[PATCH bpf-next v3] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR_PTR()

[PATCH bpf-next v3] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR_PTR()

[Tirthendu Sarkar]

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.

Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202307210434.OjgqFcbB-lkp@intel.com/
Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
---
Changelog:
	v2 -> v3:
	- Added further details in commit message as asked by Maciej
	v1 -> v2:
	- Removed err as a parameter to xsk_build_skb_zerocopy()
	[Stanislav Fomichev]
	- use explicit error to distinguish packet drop vs retry

 net/xdp/xsk.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index fcfc8472f73d..55f8b9b0e06d 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -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;
-- 
2.34.1

Re: [PATCH bpf-next v3] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR_PTR()

[Magnus Karlsson]

On Wed, 23 Aug 2023 at 17:05, Tirthendu Sarkar
<tirthendu.sarkar@intel.com> wrote:
>
> 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.

Thanks Tirtha for the fix.

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

> Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202307210434.OjgqFcbB-lkp@intel.com/
> Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
> ---
> Changelog:
>         v2 -> v3:
>         - Added further details in commit message as asked by Maciej
>         v1 -> v2:
>         - Removed err as a parameter to xsk_build_skb_zerocopy()
>         [Stanislav Fomichev]
>         - use explicit error to distinguish packet drop vs retry
>
>  net/xdp/xsk.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index fcfc8472f73d..55f8b9b0e06d 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -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;
> --
> 2.34.1
>
>

Re: [PATCH bpf-next v3] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR_PTR()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Wed, 23 Aug 2023 20:17:13 +0530 you wrote:
> 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
> 
> [...]

Here is the summary with links:
  - [bpf-next,v3] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR_PTR()
    https://git.kernel.org/bpf/bpf/c/9d0a67b9d42c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html