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

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

[Tirthendu Sarkar]

xsk_build_skb_zerocopy() may return an error other than -EAGAIN and this
is received as skb and used later in xsk_set_destructor_arg() and
xsk_drop_skb() which must operate on a valid skb.

Set -EOVERFLOW as error when MAX_SKB_FRAGS are exceeded and packet needs
to be dropped and use this to distinguish against all other error cases
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:
	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 v2] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR_PTR()

[Stanislav Fomichev]

On 08/15, Tirthendu Sarkar wrote:
> xsk_build_skb_zerocopy() may return an error other than -EAGAIN and this
> is received as skb and used later in xsk_set_destructor_arg() and
> xsk_drop_skb() which must operate on a valid skb.
> 
> Set -EOVERFLOW as error when MAX_SKB_FRAGS are exceeded and packet needs
> to be dropped and use this to distinguish against all other error cases
> 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:
> 	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) {

Don't think this will work? We have some other error paths in xsk_build_skb
that are not -EOVERFLOW that still need kfree_skb, right?

I feel like we are trying to share some state between xsk_build_skb and
xsk_build_skb_zerocopy which we really shouldn't share. So how about
we try to have a separate cleanup path in xsk_build_skb_zerocopy?

Will something like the following (untested / uncompiled) work instead?

IOW, ideally, xsk_build_skb should look like:

	if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
		return xsk_build_skb_zerocopy(xs, desc);
	} else {
		return xsk_build_skb_copy(xs, desc);
		/* ^^ current path that should really be a separate func */
	}

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 79a96019b7ef..747dd012afdb 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -578,6 +578,19 @@ static void xsk_drop_skb(struct sk_buff *skb)
 	xsk_consume_skb(skb);
 }
 
+static struct sk_buff *xsk_cleanup_skb(int err, struct sk_buff *skb, struct xdp_sock *xs)
+{
+	if (err == -EAGAIN) {
+		xsk_cq_cancel_locked(xs, 1);
+	} else {
+		xsk_set_destructor_arg(skb);
+		xsk_drop_skb(skb);
+		xskq_cons_release(xs->tx);
+	}
+
+	return ERR_PTR(err);
+}
+
 static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
 					      struct xdp_desc *desc)
 {
@@ -593,8 +606,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
 		hr = max(NET_SKB_PAD, L1_CACHE_ALIGN(xs->dev->needed_headroom));
 
 		skb = sock_alloc_send_skb(&xs->sk, hr, 1, &err);
-		if (unlikely(!skb))
-			return ERR_PTR(err);
+		if (unlikely(!skb)) {
+			err = ERR_PTR(err);
+			goto free_err;
+		}
 
 		skb_reserve(skb, hr);
 	}
@@ -608,8 +623,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
 	addr = buffer - pool->addrs;
 
 	for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
-		if (unlikely(i >= MAX_SKB_FRAGS))
-			return ERR_PTR(-EFAULT);
+		if (unlikely(i >= MAX_SKB_FRAGS)) {
+			err = ERR_PTR(-EFAULT);
+			goto free_err;
+		}
 
 		page = pool->umem->pgs[addr >> PAGE_SHIFT];
 		get_page(page);
@@ -629,6 +646,9 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
 	refcount_add(ts, &xs->sk.sk_wmem_alloc);
 
 	return skb;
+
+free_err:
+	return xsk_cleanup_skb(err, skb, xs);
 }
 
 static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
@@ -641,11 +661,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
 	int err;
 
 	if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
-		skb = xsk_build_skb_zerocopy(xs, desc);
-		if (IS_ERR(skb)) {
-			err = PTR_ERR(skb);
-			goto free_err;
-		}
+		return xsk_build_skb_zerocopy(xs, desc);
 	} else {
 		u32 hr, tr, len;
 		void *buffer;
@@ -729,15 +745,7 @@ 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);
-		xskq_cons_release(xs->tx);
-	}
-
-	return ERR_PTR(err);
+	return xsk_cleanup_skb(err, skb, xs);
 }
 
 static int __xsk_generic_xmit(struct sock *sk)

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

[Sarkar, Tirthendu]

> -----Original Message-----
> From: Stanislav Fomichev <sdf@google.com>
> Sent: Tuesday, August 15, 2023 11:51 PM
> On 08/15, Tirthendu Sarkar wrote:
> > xsk_build_skb_zerocopy() may return an error other than -EAGAIN and
> this
> > is received as skb and used later in xsk_set_destructor_arg() and
> > xsk_drop_skb() which must operate on a valid skb.
> >
> > Set -EOVERFLOW as error when MAX_SKB_FRAGS are exceeded and
> packet needs
> > to be dropped and use this to distinguish against all other error cases
> > 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:
> > 	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) {
> 
> Don't think this will work? We have some other error paths in xsk_build_skb
> that are not -EOVERFLOW that still need kfree_skb, right?
> 

There are 4 possible error paths in xsk_build_skb():
1. sock_alloc_send_skb:  skb is NULL; retry
2. skb_store_bits : free skb and retry
3. MAX_SKB_FRAGS exceeded: Free skb, cleanup and drop packet
4. alloc_page fails for frag: retry page allocation for frag w/o freeing skb

Of these 1] and 3] can also happen in xsk_build_skb_zerocopy() and the 
error returned is either -EOVERFLOW or something else and the same
error handling needs to be done.

> I feel like we are trying to share some state between xsk_build_skb and
> xsk_build_skb_zerocopy which we really shouldn't share. So how about
> we try to have a separate cleanup path in xsk_build_skb_zerocopy?
> 

The only things that are common between *copy and *zerocopy  paths are 
setting  the skb headers (destructor/args, mark, priority) and error handling.

While we can potentially split out the paths into independent functions, the
same skb header settings and error handling will still need to be duplicated in
both functions.

> Will something like the following (untested / uncompiled) work instead?
> 
> IOW, ideally, xsk_build_skb should look like:
> 
> 	if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
> 		return xsk_build_skb_zerocopy(xs, desc);
> 	} else {
> 		return xsk_build_skb_copy(xs, desc);
> 		/* ^^ current path that should really be a separate func */
> 	}
> 

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

[Maciej Fijalkowski]

On Wed, Aug 16, 2023 at 08:04:52AM +0200, Sarkar, Tirthendu wrote:
> > -----Original Message-----
> > From: Stanislav Fomichev <sdf@google.com>
> > Sent: Tuesday, August 15, 2023 11:51 PM
> > On 08/15, Tirthendu Sarkar wrote:
> > > xsk_build_skb_zerocopy() may return an error other than -EAGAIN and
> > this
> > > is received as skb and used later in xsk_set_destructor_arg() and
> > > xsk_drop_skb() which must operate on a valid skb.
> > >
> > > Set -EOVERFLOW as error when MAX_SKB_FRAGS are exceeded and
> > packet needs
> > > to be dropped and use this to distinguish against all other error cases
> > > where allocation needs to be retried.

Please be explicit - say that you're changing this error code
in xsk_build_skb_zerocopy() otherwise it's not clear.

You're not saying anything about kfree_skb() that is added when
skb_store_bits() failed. This code is non-trivial so all of the changes
need to be described.

Also did we test this patch? I believe it would require us to run xdpsock
against virtio net device?

> > >
> > > 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:
> > > 	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) {
> > 
> > Don't think this will work? We have some other error paths in xsk_build_skb
> > that are not -EOVERFLOW that still need kfree_skb, right?
> > 
> 
> There are 4 possible error paths in xsk_build_skb():
> 1. sock_alloc_send_skb:  skb is NULL; retry
> 2. skb_store_bits : free skb and retry
> 3. MAX_SKB_FRAGS exceeded: Free skb, cleanup and drop packet
> 4. alloc_page fails for frag: retry page allocation for frag w/o freeing skb
> 
> Of these 1] and 3] can also happen in xsk_build_skb_zerocopy() and the 
> error returned is either -EOVERFLOW or something else and the same
> error handling needs to be done.

That would be helpful to have it included in commit message.

> 
> > I feel like we are trying to share some state between xsk_build_skb and
> > xsk_build_skb_zerocopy which we really shouldn't share. So how about
> > we try to have a separate cleanup path in xsk_build_skb_zerocopy?
> > 
> 
> The only things that are common between *copy and *zerocopy  paths are 
> setting  the skb headers (destructor/args, mark, priority) and error handling.
> 
> While we can potentially split out the paths into independent functions, the
> same skb header settings and error handling will still need to be duplicated in
> both functions.
> 
> > Will something like the following (untested / uncompiled) work instead?
> > 
> > IOW, ideally, xsk_build_skb should look like:
> > 
> > 	if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
> > 		return xsk_build_skb_zerocopy(xs, desc);
> > 	} else {
> > 		return xsk_build_skb_copy(xs, desc);
> > 		/* ^^ current path that should really be a separate func */
> > 	}

IMHO this is way out of the scope for a fix. We can think later on about
cleaning up these paths.

> >