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: <20250610160946.10b5fb7d@kernel.org>
Date: Tue, 10 Jun 2025 16:09:46 -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 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?
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

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

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

would be just fine.

Similar comments apply to other conditions.
-- 
pw-bot: cr