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

Message-ID: <f7c398f4-ea06-495c-b310-cae3c731cb4b@akamai.com>
Date: Tue, 10 Jun 2025 19:36:00 -0400
From: Jason Baron <jbaron@...mai.com>
To: Jakub Kicinski <kuba@...nel.org>
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 6/10/25 7:09 PM, Jakub Kicinski wrote:
> !-------------------------------------------------------------------|
>    This Message Is From an External Sender
>    This message came from outside your organization.
> |-------------------------------------------------------------------!
> 
> On Mon,  9 Jun 2025 12:12:44 -0400 Jason Baron wrote:
>> As noted in that fix, if there are multiple threads writing to a
>> netlink socket it's possible to slightly exceed rcvbuf value. But as
>> noted this avoids an expensive 'atomic_add_return()' for the common
>> case. I've confirmed that with the fix the modified program from
>> SOCK_DIAG(7) can no longer fill memory and the sk->sk_rcvbuf limit
>> is enforced.
> 
> Looks good in general, could you add a Fixes tag?

Sure I can add this for v2.

> A few coding style nit picks..
> 
>> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
>> index e8972a857e51..607e5d72de39 100644
>> --- a/net/netlink/af_netlink.c
>> +++ b/net/netlink/af_netlink.c
>> @@ -1213,11 +1213,15 @@ int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
>>   		      long *timeo, struct sock *ssk)
>>   {
>>   	struct netlink_sock *nlk;
>> +	unsigned int rmem, rcvbuf, size;
> 
> Please try to short variable declaration lines longest to shortest
> 

ok sure, will fix for v2.

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

> 
> 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.")


Thanks,

-Jason

> would be just fine.
> 
> Similar comments apply to other conditions.