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

[Jakub Kicinski <kuba@kernel.org>]

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.