RE: [PATCH net-next v02 1/2] af_packet: allow fanout_add when socket is not RUNNING

[Gur Stavi]

> Gur Stavi wrote:
> > > Gur Stavi wrote:
> > > > >> @@ -1846,21 +1846,21 @@ static int fanout_add(struct sock *sk,
> > > struct fanout_args *args)
> > > > >>  	err = -EINVAL;
> > > > >>
> > > > >>  	spin_lock(&po->bind_lock);
> > > > >> -	if (packet_sock_flag(po, PACKET_SOCK_RUNNING) &&
> > > > >> -	    match->type == type &&
> > > > >> +	if (match->type == type &&
> > > > >>  	    match->prot_hook.type == po->prot_hook.type &&
> > > > >>  	    match->prot_hook.dev == po->prot_hook.dev) {
> > > > >
> > > > > Remaining unaddressed issue is that the socket can now be added
> > > > > before being bound. See comment in v1.
> > > >
> > > > I extended the psock_fanout test with unbound fanout test.
> > > >
> > > > As far as I understand, the easiest way to verify bind is to test
> that
> > > > po->prot_hook.dev != NULL, since we are under a bind_lock anyway.
> > > > But perhaps a more readable and direct approach to test "bind"
> would be
> > > > to test po->ifindex != -1, as ifindex is commented as "bound
> device".
> > > > However, at the moment ifindex is not initialized to -1, I can add
> such
> > > > initialization, but perhaps I do not fully understand all the
> logic.
> > > >
> > > > Any preferences?
> > >
> > > prot_hook.dev is not necessarily set if a packet socket is bound.
> > > It may be bound to any device. See dev_add_pack and ptype_head.
> > >
> > > prot_hook.type, on the other hand, must be set if bound and is only
> > > modified with the bind_lock held too.
> > >
> > > Well, and in packet_create. But setsockopt PACKET_FANOUT_ADD also
> > > succeeds in case bind() was not called explicitly first to bind to
> > > a specific device or change ptype.
> >
> > Please clarify the last paragraph? When you say "also succeeds" do you
> > mean SHOULD succeed or MAY SUCCEED by mistake if "something" happens
> ???
> 
> I mean it succeeds currently. Which behavior must then be maintained.
> 
> > Do you refer to the following scenario: socket is created with non-zero
> > protocol and becomes RUNNING "without bind" for all devices. In that
> case
> > it can be added to FANOUT without bind. Is that considered a bug or
> does
> > the bind requirement for fanout only apply for all-protocol (0)
> sockets?
> 
> I'm beginning to think that this bind requirement is not needed.

I agree with that. I think that is an historical mistake that socket
becomes implicitly bound to all interfaces if a protocol is defined
during create. Without this bind requirement would make sense.

> 
> All type and dev are valid, even if an ETH_P_NONE fanout group would
> be fairly useless.

Fanout is all about RX, I think that refusing fanout for socket that
will not receive any packet is OK. The condition can be:
if (po->ifindex == -1 || !po->num)

I realized another possible problem. We should consider adding ifindex
Field to struct packet_fanout to be used for lookup of an existing match.
There is little sense to bind sockets to different interfaces and then
put them in the same fanout group.
If you agree, I can prepare a separate patch for that.

> The type and dev must match that of the fanout group, and once added
> to a fanout group can no longer be changed (bind will fail).
> 
> I briefy considered the reason might be max_num_members accounting.
> Since f->num_members counts running sockets. But that is not used
> when tracking membership of the group, sk_ref is. Every packet socket
> whose po->rollover is increased increases this refcount.
> 
> > What about using ifindex to detect bind? Initialize it to -1 in
> > packet_create and ensure that packet_do_bind, on success, sets it
> > to device id or 0?
> >
> > psock_fanout, should probably be extended with scenarios that test
> > "all devices" and all/specific protocols. Any specific scenario
> > suggestions?
> >
> >
>