Commit 9d0a67b9 authored Aug 23, 2023 by Tirthendu Sarkar Committed by Daniel Borkmann Aug 30, 2023

xsk: Fix xsk_build_skb() error: 'skb' dereferencing possible ERR_PTR()

Currently, xsk_build_skb() is a function that builds skb in two possible
ways and then is ended with common error handling.

We can distinguish four possible error paths and handling in xsk_build_skb():

1. sock_alloc_send_skb fails: Retry (skb is NULL).
2. skb_store_bits fails : Free skb and retry.
3. MAX_SKB_FRAGS exceeded: Free skb, cleanup and drop packet.
4. alloc_page fails for frag: Retry page allocation w/o freeing skb

1] and 3] can happen in xsk_build_skb_zerocopy(), which is one of the
two code paths responsible for building skb. Common error path in
xsk_build_skb() assumes that in case errno != -EAGAIN, skb is a valid
pointer, which is wrong as kernel test robot reports that in
xsk_build_skb_zerocopy() other errno values are returned for skb being
NULL.

To fix this, set -EOVERFLOW as error when MAX_SKB_FRAGS are exceeded
and packet needs to be dropped in both xsk_build_skb() and
xsk_build_skb_zerocopy() and use this to distinguish against all other
error cases. Also, add explicit kfree_skb() for 3] so that handling
of 1], 2], and 3] becomes identical where allocation needs to be retried.

Fixes: cf24f5a5 ("xsk: add support for AF_XDP multi-buffer on Tx path")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
Closes: https://lore.kernel.org/r/202307210434.OjgqFcbB-lkp@intel.com
Link: https://lore.kernel.org/bpf/20230823144713.2231808-1-tirthendu.sarkar@intel.com

parent 6a8faf10

net/xdp/xsk.c

+13 −9

Original line number	Diff line number	Diff line
		@@ -602,7 +602,7 @@ static struct sk_buff xsk_build_skb_zerocopy(struct xdp_sock xs,

		for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
		if (unlikely(i >= MAX_SKB_FRAGS))
		return ERR_PTR(-EFAULT);
		return ERR_PTR(-EOVERFLOW);

		page = pool->umem->pgs[addr >> PAGE_SHIFT];
		get_page(page);
		@@ -655,15 +655,17 @@ static struct sk_buff xsk_build_skb(struct xdp_sock xs,
		skb_put(skb, len);

		err = skb_store_bits(skb, 0, buffer, len);
		if (unlikely(err))
		if (unlikely(err)) {
		kfree_skb(skb);
		goto free_err;
		}
		} else {
		int nr_frags = skb_shinfo(skb)->nr_frags;
		struct page *page;
		u8 *vaddr;

		if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) && xp_mb_desc(desc))) {
		err = -EFAULT;
		err = -EOVERFLOW;
		goto free_err;
		}

		@@ -690,12 +692,14 @@ static struct sk_buff xsk_build_skb(struct xdp_sock xs,
		return skb;

		free_err:
		if (err == -EAGAIN) {
		xsk_cq_cancel_locked(xs, 1);
		} else {
		xsk_set_destructor_arg(skb);
		xsk_drop_skb(skb);
		if (err == -EOVERFLOW) {
		/* Drop the packet */
		xsk_set_destructor_arg(xs->skb);
		xsk_drop_skb(xs->skb);
		xskq_cons_release(xs->tx);
		} else {
		/* Let application retry */
		xsk_cq_cancel_locked(xs, 1);
		}

		return ERR_PTR(err);
		@@ -738,7 +742,7 @@ static int __xsk_generic_xmit(struct sock *sk)
		skb = xsk_build_skb(xs, &desc);
		if (IS_ERR(skb)) {
		err = PTR_ERR(skb);
		if (err == -EAGAIN)
		if (err != -EOVERFLOW)
		goto out;
		err = 0;
		continue;