Re: [PATCH net] inet: frags: bound the wmem charge of the socket completing a reassembly

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In net/ipv4/inet_fragment.c, inet_frag_wmem_ok() (lines 533-540)
  reads sk->sk_wmem_alloc without any socket-wide lock or reservation…

--

inet: frags: bound the wmem charge of the socket completing a reassembly

This patch adds inet_frag_wmem_ok(). The new helper refuses a reassembly
when charging the queue's truesize to the completing socket would push
sk_wmem_alloc above 2 * sk_sndbuf + 4MB, capped at INET_FRAG_MAX_TRUESIZE.
The aim is to keep sk_wmem_alloc from saturating while several reassembled
skbs owned by the same socket are alive at the same time.

Closes: https://lore.kernel.org/netdev/24feda7d-ae49-45af-9596-2cc5f149f28e@redhat.com/

> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index 0b8d22ffafca6..c11d7e976dfa5 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c

[ ... ]

> @@ -519,6 +520,25 @@ int inet_frag_queue_insert(struct inet_frag_queue *q, struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL(inet_frag_queue_insert);
>  
> +/* When a socket owned fragment completes a queue, the truesize of all
> + * other fragments is charged to its sk_wmem_alloc, although they were
> + * orphaned when queued and possibly sent by someone else.
> + * Bound this like ip_append_data() bounds the memory a socket can
> + * allocate (2 * sk_sndbuf), plus slack for fragments inflated by a
> + * refragmentation using a small MTU. Otherwise, several reassembled skbs
> + * alive at the same time can saturate sk_wmem_alloc, and leak the socket.
> + */
> +#define INET_FRAG_WMEM_SLACK	SZ_4M
> +
> +static bool inet_frag_wmem_ok(const struct inet_frag_queue *q,
> +			      const struct sock *sk)
> +{
> +	u64 limit = 2ULL * READ_ONCE(sk->sk_sndbuf) + INET_FRAG_WMEM_SLACK;
> +	u64 wmem = (u64)refcount_read(&sk->sk_wmem_alloc) + q->truesize;
> +
> +	return wmem <= min_t(u64, limit, INET_FRAG_MAX_TRUESIZE);
> +}

[Severity: High]
Is this check safe when other reassemblies charge the same socket at the
same time?

inet_frag_wmem_ok() reads sk_wmem_alloc without a lock. The charge it
approves is applied later, in two steps:

inet_frag_reasm_prepare()
    refcount_add(ts_delta, &sk->sk_wmem_alloc);

inet_frag_reasm_finish()
    /* after the loop over every fragment */
    refcount_add(sum_truesize - head_truesize, &sk->sk_wmem_alloc);

The only lock held across this is the per-queue lock, for example
spin_lock(&qp->q.lock) in ip_defrag(). The IPv6, nf_conntrack and 6lowpan
callers follow the same pattern. That lock serializes one queue, not all
the queues charging the same socket.

Suppose several CPUs complete different queues for the same socket. Can
they all read the same sk_wmem_alloc value W, all see
W + q->truesize <= limit, and then all add their charge?

The limit is at most INET_FRAG_MAX_TRUESIZE (INT_MAX / 2). So three such
completions racing would seem to be enough to push sk_wmem_alloc past
INT_MAX. That is the saturation and socket leak the commit message
describes.

One possible path from an unprivileged user/net namespace:

tun_chr_open() sets tfile->sk.sk_sndbuf = INT_MAX, so the limit works out
to min(2 * INT_MAX + 4M, INT_MAX / 2) = INT_MAX / 2.

tun_build_skb() and the sock_alloc_send_pskb() path leave the skbs owned
by the tun socket via skb_set_owner_w(), so is_skb_wmem() is true. These
skbs are not orphaned before tc ingress, and tun_get_user() takes no
per-write lock outside the napi_frags mode.

Add clsact plus act_ct on the tun device and raise ipfrag_high_thresh.
Then several threads writing the final fragments of different datagrams
to the same tun fd would each reach this path on their own CPU, each
under a different q->lock:

netif_receive_skb()
  tcf_ct_handle_fragments()
    nf_ct_handle_fragments()
      ip_defrag()
        inet_frag_reasm_prepare()
          inet_frag_wmem_ok()

The window between the check and the second refcount_add() covers
skb_clone(), skb_morph(), skb_unclone() and the whole fragment loop in
inet_frag_reasm_finish().

Would this need an atomic reservation on sk_wmem_alloc instead? For
example, add the charge first and then recheck and roll back, or use a
cmpxchg loop.

> +
>  void *inet_frag_reasm_prepare(struct inet_frag_queue *q, struct sk_buff *skb,
>  			      struct sk_buff *parent)
>  {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103092957.4164530-1-edumazet%40kernel.org