netdev - Re: [PATCH net-next 2/2] net/packet: use synchronize_net

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: <99b9647a7c3fd8328a78c4a1944d1f41c4606676.camel@redhat.com>
Date:   Tue, 15 Mar 2022 10:37:32 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     David Lamparter <equinox@...c24.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net/packet: use synchronize_net_expedited()

Hello,

On Sun, 2022-03-13 at 11:00 +0100, David Lamparter wrote:
> Since these locations don't have RTNL held, synchronize_net() uses
> synchronize_rcu(), which takes its time.  Unfortunately, this is user
> visible on bind() and close() calls from userspace.  With a good amount
> of network interfaces, this sums up to Wireshark (dumpcap) taking
> several seconds to start for no good reason.
> 
> Signed-off-by: David Lamparter <equinox@...c24.net>
> ---
>  net/packet/af_packet.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 1b93ce1a5600..559e72149110 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -379,7 +379,7 @@ static void __unregister_prot_hook(struct sock *sk, bool sync)
>  
>  	if (sync) {
>  		spin_unlock(&po->bind_lock);
> -		synchronize_net();
> +		synchronize_net_expedited();
>  		spin_lock(&po->bind_lock);
>  	}
>  }

I *think* synchronize_net_expedited could be useful in some (likely
limited) circumstances, but here it looks like a bit too much.
Creating, deleteing or setting up a packet socket will hammer all the
CPUs significanly, while e.g. starting tcpdump on system is supposed
realitively safe.

I *think* you can speed up your test case replacing  synchronize_net 
with call_rcu() in __fanout_set_data_bpf() and in packet_release(). In
packet_set_ring() I guess synchronize_net() is needed only if an older
ring was running. Finally __fanout_set_data_bpf() should matter only
when replacing an existing filter, it likely should not impact your use
-case

Thanks!

Paolo