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: <49E77C4C.5060508@cosmosbay.com>
Date:	Thu, 16 Apr 2009 20:43:24 +0200
From:	Eric Dumazet <dada1@...mosbay.com>
To:	paulmck@...ux.vnet.ibm.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()

Paul E. McKenney a écrit :
> 
> Please add a comment where the synchronize_rcu() used to be explaining why
> it is not needed.  The poor slob who copies your code isn't going to read
> theh Documentation/RCU, he is just going to expect it to magically work.
> 
> With the synchronize_rcu(), it does just magically work.  Without the
> synchronize_rcu(), you have to be careful.  Therefore, please add the
> comment saying that care is required.
> 

Sorry Paul, I dont understand why I should put a comment to say :

/*
 * Dont need to use synchronize_net() or call_rcu() or msleep(100) or
 * whatever function here because bla bla ...
 */

We could add this comment in about 99% of all functions in linux kernel ;)

I checked inet6_register_protosw(struct inet_protosw *p)
and it doesnt have this synchronize_rcu() neither the comment you advise...

Following construct is obvious and should not be commented in code itself.

spin_lock_bh(&somelock);
list_for_each(..., ...) {
	if (some_condition) {
		list_add_rcu(..., ...)
or		rcu_assign_pointer(...) 
		break;
	}
}
spin_unlock_bh(&somelock);

If it is not obvious, then it should be documented once in Documentation/RCU, since
we find hundred of similar code in kernel.

On the contrary, places where we *use* synchronize_{rcu|net}() should get a comment
to explain why this is really necessary since this function can be a real problem.

Thanks

--
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