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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <SN7PR11MB665573AB14515B617C3D2EA69015A@SN7PR11MB6655.namprd11.prod.outlook.com>
Date: Wed, 16 Aug 2023 06:04:52 +0000
From: "Sarkar, Tirthendu" <tirthendu.sarkar@...el.com>
To: Stanislav Fomichev <sdf@...gle.com>
CC: "bpf@...r.kernel.org" <bpf@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "bjorn@...nel.org" <bjorn@...nel.org>, "Karlsson,
 Magnus" <magnus.karlsson@...el.com>, "Fijalkowski, Maciej"
	<maciej.fijalkowski@...el.com>, "jonathan.lemon@...il.com"
	<jonathan.lemon@...il.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"ast@...nel.org" <ast@...nel.org>, "daniel@...earbox.net"
	<daniel@...earbox.net>, "martin.lau@...ux.dev" <martin.lau@...ux.dev>,
	"dan.carpenter@...aro.org" <dan.carpenter@...aro.org>
Subject: RE: [PATCH bpf-next v2] xsk: fix xsk_build_skb() error: 'skb'
 dereferencing possible ERR_PTR()

> -----Original Message-----
> From: Stanislav Fomichev <sdf@...gle.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@...el.com>
> > Reported-by: kernel test robot <lkp@...el.com>
> > Reported-by: Dan Carpenter <dan.carpenter@...aro.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 */
> 	}
>