netdev - Re: [PATCH] net: remove superfluous call to synchronize

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: <20090415215454.GU6766@linux.vnet.ibm.com>
Date:	Wed, 15 Apr 2009 14:54:54 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Eric Dumazet <dada1@...mosbay.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: remove superfluous call to synchronize_net()

On Wed, Apr 15, 2009 at 05:38:06PM +0200, Eric Dumazet wrote:
> inet_register_protosw() is adding inet_protosw to inetsw[] with appropriate
> locking section and rcu variant. No need to call synchronize_net() to wait
> for a RCU grace period. Changes are immediatly visible to other cpus anyway.

I agree with the conclusion (that this change is safe), but not with
the reasoning process.  ;-)

The reason that this change is safe is that any inter-process
communication mechanism used to tell other CPUs that this protocol has
been registered must contain relevant memory barriers, otherwise, that
mechanism won't be reliable.

If an unreliable mechanism was to be used, the other CPU might not yet see
the protocol.  For example, if the caller did a simple non-atomic store
to a variable that the other CPU accessed with a simple non-atomic load,
then that other CPU could potentially see the inetsw[] without the new
protocol, given that inet_create() is lockless.  Unlikely, but possible.

But if a proper inter-process communication mechanism is used to inform
the other CPU, then the first CPU's memory operations will be seen.

So I suggest a comment to this effect.

> This saves about 13 ms on boot time on a HZ=1000 8 cpus machine ;)
> (4 calls to inet_register_protosw(), and about 3200 us per call)
> 
> But more seriously, we should audit all synchronize_{rcu|net}() calls
> to make sure we dont waste time and hide some bugs because of artificial
> delays.

Good point!

							Thanx, Paul

> Signed-off-by: Eric Dumazet <dada1@...mosbay.com>
> 
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 7f03373..1706896 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1003,8 +1003,6 @@ void inet_register_protosw(struct inet_protosw *p)
>  out:
>  	spin_unlock_bh(&inetsw_lock);
> 
> -	synchronize_net();
> -
>  	return;
> 
>  out_permanent:
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html