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

[Jakub Kicinski <kuba@kernel.org>]

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