| Message ID | 20230823144713.2231808-1-tirthendu.sarkar@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9d0a67b9d42c630d5013ef81587335d975a7a4a9 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v3] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR_PTR() | expand |
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 > >
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!
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;
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(-)