netdev - Re: [PATCH net-next] net: skbuff: Skip early return in skb

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Zqi/SL/icYA9IwjH@gmail.com>
Date: Tue, 30 Jul 2024 03:24:08 -0700
From: Breno Leitao <leitao@...ian.org>
To: Paolo Abeni <pabeni@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, leit@...a.com,
	Chris Mason <clm@...com>,
	"open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next] net: skbuff: Skip early return in skb_unref
 when debugging

Hello Paolo,

On Tue, Jul 30, 2024 at 11:38:38AM +0200, Paolo Abeni wrote:
> On 7/29/24 12:47, Breno Leitao wrote:

> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -1225,7 +1225,7 @@ static inline bool skb_unref(struct sk_buff *skb)
> >   {
> >   	if (unlikely(!skb))
> >   		return false;
> > -	if (likely(refcount_read(&skb->users) == 1))
> > +	if (!IS_ENABLED(CONFIG_DEBUG_NET) && likely(refcount_read(&skb->users) == 1))
> >   		smp_rmb();
> >   	else if (likely(!refcount_dec_and_test(&skb->users)))
> >   		return false;
 
> I think one assumption behind CONFIG_DEBUG_NET is that enabling such config
> should not have any measurable impact on performances.
> 
> I suspect the above could indeed cause some measurable impact, e.g. under
> UDP flood, when the user-space receiver and the BH runs on different cores,
> as this will increase pressure on the CPU cache. Could you please benchmark
> such scenario before and after this patch?

Sure, I am more than happy to do so. I will be back with it soon.

Assuming there is some performance overhead, isn't it a worthwhile
trade-off for those who are debugging the network? In other words,
wouldn't it be better to prioritize correctness over optimization in
this the CONFIG_DEBUG_NET case, even if it means sacrificing some
performance?

Thanks for reviewing,
--breno