Re: [PATCH net 1/2] net: restrict SO_RESERVE_MEM to TCP sockets and cap max value

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] getsockopt and setsockopt for SO_RESERVE_MEM now disagree on the
  valid range.

--

net: restrict SO_RESERVE_MEM to TCP sockets and cap max value

This patch limits sock_reserve_memory() to TCP sockets. UDP protects
sk_forward_alloc with a different lock, and udp_rmem_release() does not
honor the reserved memory. It also makes sk_setsockopt(SO_RESERVE_MEM)
reject values above INT_MAX >> 1, so that the page conversions in
sock_reserve_memory() cannot overflow.

Closes: https://lore.kernel.org/netdev/5a88421d-10ef-4fca-9acb-85a27a3c1173@huawei.com/

> diff --git a/net/core/sock.c b/net/core/sock.c
> index d23333bb4f3fa..cfc6700a62e6f 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c

[ ... ]

> @@ -1661,7 +1661,7 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
>  	{
>  		int delta;
>  
> -		if (val < 0) {
> +		if (val < 0 || val > INT_MAX >> 1) {

[Severity: Low]
Could this cap leave getsockopt and setsockopt disagreeing about the
valid range for SO_RESERVE_MEM?

The check is made on the raw byte count. sock_reserve_memory() then rounds
the request up to whole pages before it stores the value:

net/core/sock.c:sock_reserve_memory() {
    ...
	pages = sk_mem_pages(bytes);
    ...
	WRITE_ONCE(sk->sk_reserved_mem,
		   sk->sk_reserved_mem + (pages << PAGE_SHIFT));
    ...
}

If val is 0x3FFFFFFF, the largest value this check accepts, sk_mem_pages()
returns 0x40000 pages with 4K pages (0x4000 with 64K pages). So
sk_reserved_mem becomes 0x40000000. The same holds for any val within one
page below the cap.

sk_getsockopt() then reports that rounded value back to userspace:

	case SO_RESERVE_MEM:
		v.val = READ_ONCE(sk->sk_reserved_mem);

If that value (1073741824) is passed back to setsockopt(SO_RESERVE_MEM), as
a save/restore or checkpoint/restore flow would, it is now above
INT_MAX >> 1 and fails with -EINVAL. Before this patch, setting the value
returned by getsockopt always worked.

Nothing overflows here, since 0x40000000 still fits in an int. The only
effect is an unexpected -EINVAL on that round trip.

Would it make sense to apply the cap after page rounding? For example,
reject when PAGE_ALIGN(val) > INT_MAX >> 1, or state the cap in
page-aligned terms.

>  			ret = -EINVAL;
>  			break;
>  		}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924123602.1979090-1-edumazet%40google.com