Re: [PATCH net-next v01 1/4] af_packet: allow fanout_add when socket is not RUNNING

[Willem de Bruijn]

Gur Stavi wrote:
> PACKET socket can retain its fanout membership through link down and up
> and (obviously) leave a fanout while it is not RUNNING (link down).

Probably just semantics, but a socket cannot leave a fanout group.
__fanout_unlink does remove it from the fanout group on link down
(__unregister_prot_hook). But a subsequent link up will always add it
back.

> However, socket was forbidden from joining a fanout while it was not
> RUNNING.

Your change looks safe to me.

I'm trying to understand where this check came from. Originally it was
this at the start of fanout_add:

+       if (!po->running)
+               return -EINVAL;

Perhaps as indicator of this requirement

    An AF_PACKET socket must be fully bound before it tries to add itself
    to a fanout.  All AF_PACKET sockets trying to join the same fanout
    must all have the same bind settings.

Probably because for the first socket in the fanout group, the group
inherits the relevant bound fields from the socket:

                match->prot_hook.type = po->prot_hook.type;
                match->prot_hook.dev = po->prot_hook.dev;

> This patch allows PACKET socket to join fanout while not RUNNING.
> 
> Signed-off-by: Gur Stavi <gur.stavi@xxxxxxxxxx>
> ---
>  net/packet/af_packet.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index a705ec214254..c28eee7f6ce0 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -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) {
>  		err = -ENOSPC;
>  		if (refcount_read(&match->sk_ref) < match->max_num_members) {
> -			__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);
> +			if (packet_sock_flag(po, PACKET_SOCK_RUNNING)) {
> +				__dev_remove_pack(&po->prot_hook);
> +				__fanout_link(sk, po);
> +			}
>  			err = 0;
>  		}
>  	}
> -- 
> 2.45.2
>