Re: [PATCH net v4 2/8] xsk: handle NULL dereference of the skb without frags issue

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/

Simon says:

Hi Jason,

I'm providing this for informational purposes.
My unedrstanding is that the problem flagged below
is not present with the full 8 patches of the series
applied.

---
xsk: handle NULL dereference of the skb without frags issue

This change guards the existing xsk_inc_num_desc()/xsk_drop_skb() calls
in xsk_build_skb()'s free_err path with an xs->skb check, and adds a new
else branch that cancels the reserved CQ slot and bumps invalid_descs
when xs->skb is NULL.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 887abed25466..54d4895fc11e 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -973,9 +973,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>  		kfree_skb(skb);
>  
>  	if (err == -EOVERFLOW) {
> -		/* Drop the packet */
> -		xsk_inc_num_desc(xs->skb);
> -		xsk_drop_skb(xs->skb);
> +		if (xs->skb) {
> +			/* Drop the packet */
> +			xsk_inc_num_desc(xs->skb);
> +			xsk_drop_skb(xs->skb);
> +		} else {
> +			xsk_cq_cancel_locked(xs->pool, 1);
> +			xs->tx->invalid_descs++;
> +		}

The changelog states:

    In this series, the skb is already freed by kfree_skb() inside
    xsk_build_skb_zerocopy for the first-descriptor case, so we only need
    to do the bookkeeping: cancel the one reserved CQ slot and account
    for the single invalid descriptor.

Is that accurate for this commit?

Looking at xsk_build_skb_zerocopy() as it stands here, neither of its
error returns actually frees the skb:

    if (!skb) {
            ...
            skb = sock_alloc_send_skb(&xs->sk, hr, 1, &err);
            if (unlikely(!skb))
                    return ERR_PTR(err);

            skb_reserve(skb, hr);

            xsk_skb_init_misc(skb, xs, desc->addr);
            if (desc->options & XDP_TX_METADATA) {
                    err = xsk_skb_metadata(skb, buffer, desc, pool, hr);
                    if (unlikely(err))
                            return ERR_PTR(err);
            }
    }
    ...
    for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
            if (unlikely(i >= MAX_SKB_FRAGS))
                    return ERR_PTR(-EOVERFLOW);

            page = pool->umem->pgs[addr >> PAGE_SHIFT];
            get_page(page);
            ...
            skb_fill_page_desc(skb, i, page, offset, copy);
            ...
    }

So on the first-descriptor -EOVERFLOW path (xs->skb == NULL,
IFF_TX_SKB_NO_LINEAR device), doesn't xsk_build_skb_zerocopy() return
with the skb still allocated, up to MAX_SKB_FRAGS pages pinned via
get_page() and attached through skb_fill_page_desc(), and an
sk_wmem_alloc charge held by xsk_destruct_skb (installed by
xsk_skb_init_misc)?

Control then returns to xsk_build_skb():

    skb = xsk_build_skb_zerocopy(xs, desc);
    if (IS_ERR(skb)) {
            err = PTR_ERR(skb);
            skb = NULL;
            goto free_err;
    }
    ...
free_err:
    if (skb && !skb_shinfo(skb)->nr_frags)
            kfree_skb(skb);

Since the local skb has been set to NULL, the kfree_skb() guard is
skipped, and the new else-branch only performs the CQ cancel and
invalid_descs bump.

Doesn't that leak the sk_buff, the attached frag pages, and the
sk_wmem_alloc charge every time a first descriptor hits the i >=
MAX_SKB_FRAGS condition (or the xsk_skb_metadata failure)?

A later patch in the series, "xsk: free the skb when hitting the upper
bound MAX_SKB_FRAGS", adds an "if (!xs->skb) kfree_skb(skb);" inside
xsk_build_skb_zerocopy() before the -EOVERFLOW return, which seems to
confirm the skb is not freed at this commit. Should the changelog's
premise be reworked, or the free added here so each patch in the series
stands on its own?