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

[Gur Stavi]

> Gur Stavi wrote:
> > PACKET socket can retain its fanout membership through link down and up
> > and leave a fanout while closed regardless of link state.
> > However, socket was forbidden from joining a fanout while it was not
> > RUNNING.
> >
> > This patch allows PACKET socket to join fanout while not RUNNING.
> >
> > The previous test for RUNNING also implicitly tested that the socket is
> > bound to a device. An explicit test of ifindex was added instead.
> >
> > Signed-off-by: Gur Stavi <gur.stavi@xxxxxxxxxx>
> > ---
> >  net/packet/af_packet.c | 35 +++++++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index f8942062f776..8137c33ab0fd 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -1843,26 +1843,29 @@ static int fanout_add(struct sock *sk, struct
> fanout_args *args)
> >  		match->prot_hook.ignore_outgoing = type_flags &
> PACKET_FANOUT_FLAG_IGNORE_OUTGOING;
> >  		list_add(&match->list, &fanout_list);
> >  	}
> > -	err = -EINVAL;
> >
> >  	spin_lock(&po->bind_lock);
> > -	if (packet_sock_flag(po, PACKET_SOCK_RUNNING) &&
> > -	    match->type == type &&
> > -	    match->prot_hook.type == po->prot_hook.type &&
> > -	    match->prot_hook.dev == po->prot_hook.dev) {
> > +	if (po->ifindex == -1 || po->num == 0) {
> 
> This patch is more complex than it needs to be.
> 
> No need to block the case of ETH_P_NONE or not bound to a socket.


ETH_P_NONE was blocked before as well.
packet_do_bind will not switch socket to RUNNING when proto is 0.

	if (proto == 0 || !need_rehook)
		goto out_unlock;

Same for packet_create.

So the old condition could only pass the RUNNING condition if proto
was non-zero.
The new condition is exactly equivalent except for allowing IFF_UP
to be cleared in the bound device.


Yes, the same result could be achieved with a FEW less line changes
but I think that the new logic is more readable where every clause
explains itself with a comment instead of constructing one large if
statement. And since the solution does add another nested if for the
RUNNING the extra indentation started to look ugly.

> 
> I would have discussed that in v2, but you already respun.
> 
> > +		/* Socket can not receive packets */
> > +		err = -ENXIO;
> > +	} else if (match->type != type ||
> > +		   match->prot_hook.type != po->prot_hook.type ||
> > +		   match->prot_hook.dev != po->prot_hook.dev) {
> > +		/* Joining an existing group, properties must be identical */
> > +		err = -EINVAL;
> > +	} else if (refcount_read(&match->sk_ref) >= match->max_num_members)
> {
> >  		err = -ENOSPC;
> > -		if (refcount_read(&match->sk_ref) < match->max_num_members) {
> > +	} else {
> > +		/* Paired with packet_setsockopt(PACKET_FANOUT_DATA) */
> > +		WRITE_ONCE(po->fanout, match);
> > +		po->rollover = rollover;
> > +		rollover = NULL;
> > +		refcount_set(&match->sk_ref, refcount_read(&match->sk_ref) +
> 1);
> > +		if (packet_sock_flag(po, PACKET_SOCK_RUNNING)) {
> >  			__dev_remove_pack(&po->prot_hook);
> > -
> > -			/* Paired with packet_setsockopt(PACKET_FANOUT_DATA) */
> > -			WRITE_ONCE(po->fanout, match);
> > -
> > -			po->rollover = rollover;
> > -			rollover = NULL;
> > -			refcount_set(&match->sk_ref, refcount_read(&match-
> >sk_ref) + 1);
> >  			__fanout_link(sk, po);
> > -			err = 0;
> >  		}
> > +		err = 0;
> >  	}
> >  	spin_unlock(&po->bind_lock);
> >
> > @@ -3452,8 +3455,12 @@ static int packet_create(struct net *net, struct
> socket *sock, int protocol,
> >  	po->prot_hook.af_packet_net = sock_net(sk);
> >
> >  	if (proto) {
> > +		/* Implicitly bind socket to "any interface" */
> > +		po->ifindex = 0;
> >  		po->prot_hook.type = proto;
> >  		__register_prot_hook(sk);
> > +	} else {
> > +		po->ifindex = -1;
> >  	}
> >
> >  	mutex_lock(&net->packet.sklist_lock);
> > --
> > 2.45.2
> >
>