netdev - Re: [PATCH net-next v01 1/4] af_packet: allow fanout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <67045bc8a4d4a_1635eb2947d@willemb.c.googlers.com.notmuch>
Date: Mon, 07 Oct 2024 18:08:08 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Gur Stavi <gur.stavi@...wei.com>, 
 Gur Stavi <gur.stavi@...wei.com>
Cc: netdev@...r.kernel.org, 
 linux-kernel@...r.kernel.org, 
 "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v01 1/4] af_packet: allow fanout_add when socket
 is not RUNNING

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@...wei.com>
> ---
>  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
>