netdev - Re: [PATCH net-next] netlink: Fix wraparounds of sk->sk_rmem

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] [day] [month] [year] [list]

Message-ID: <20250610174550.54c0d594@kernel.org>
Date: Tue, 10 Jun 2025 17:45:50 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jason Baron <jbaron@...mai.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
 pabeni@...hat.com, horms@...nel.org, kuniyu@...zon.com
Subject: Re: [PATCH net-next] netlink: Fix wraparounds of sk->sk_rmem_alloc

On Tue, 10 Jun 2025 19:36:00 -0400 Jason Baron wrote:
> >>   	nlk = nlk_sk(sk);
> >> +	rmem = atomic_read(&sk->sk_rmem_alloc);
> >> +	rcvbuf = READ_ONCE(sk->sk_rcvbuf);
> >> +	size = skb->truesize;  
> > 
> > I don't see a reason to store skb->truesize to a temp variable, is
> > there one?  
> 
> 
> I only stored skb->truesize to 'size' in the first bit of the patch 
> where skb->truesize is not re-read and used a 2nd time. The other cases 
> I did use skb->truesize. So if you'd prefer skb->truesize twice even in 
> this first case, let me know and I can update it.

Weak preference towards not using a temp variable.
Fewer indirections make the code easier to grok IMHO.

> > Actually rcvbuf gets re-read every time, we probably don't need a temp
> > for it either. Just rmem to shorten the lines.
> >   
> >> -	if ((atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf ||
> >> -	     test_bit(NETLINK_S_CONGESTED, &nlk->state))) {
> >> +	if (((rmem + size) > rcvbuf) ||  
> > 
> > too many brackets:
> > 
> > 	if (rmem + skb->truesize > READ_ONCE(sk->sk_rcvbuf) ||
> >  
> 
> So the local variables function to type cast sk->sk_rmem_alloc and 
> sk->sk_rcvbuf to 'unsigned int' instead of their native type of 'int'. I 
> did that so that the comparison was all among the same types and didn't 
> have messy explicit casts to avoid potential compiler warnings. It 
> seemed more consistent with the style of the below patch I referenced in 
> the commit:
> 
> 5a465a0da13e ("udp: Fix multiple wraparounds of sk->sk_rmem_alloc.")

Ah, I see. Missed that when looking at the quoted commit as the temp
variables already existed there. Maybe add a comment like:

	/* READ_ONCE() and implicitly cast to unsigned int */

? Up to you.