From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return() Date: Thu, 26 Jul 2018 11:47:32 -0700 Message-ID: <20180726114732.2d5ae343@cakuba.netronome.com> References: <20180720160445.17475-1-ap420073@gmail.com> <20180720171821.ivtpo6obzx2v737c@kafai-mbp.dhcp.thefacebook.com> <20180720130538.10c22c61@cakuba.netronome.com> <20180723125843.7431eb3a@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: Daniel Borkmann , =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= , Jesper Dangaard Brouer , kafai@fb.com, Taehee Yoo , ast@kernel.org, Netdev , "Karlsson, Magnus" To: =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= Return-path: Received: from mail-qk0-f194.google.com ([209.85.220.194]:46181 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729801AbeGZUFn (ORCPT ); Thu, 26 Jul 2018 16:05:43 -0400 Received: by mail-qk0-f194.google.com with SMTP id o2-v6so1679761qkc.13 for ; Thu, 26 Jul 2018 11:47:37 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 26 Jul 2018 14:13:20 +0200, Bj=C3=B6rn T=C3=B6pel wrote: > Den m=C3=A5n 23 juli 2018 kl 21:58 skrev Jakub Kicinski: > > On Mon, 23 Jul 2018 11:39:36 +0200, Bj=C3=B6rn T=C3=B6pel wrote: =20 > > > Den fre 20 juli 2018 kl 22:08 skrev Jakub Kicinski: =20 > > > > On Fri, 20 Jul 2018 10:18:21 -0700, Martin KaFai Lau wrote: =20 > > > > > On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote: =20 > > > > > > rhashtable_lookup() can return NULL. so that NULL pointer > > > > > > check routine should be added. > > > > > > > > > > > > Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY") > > > > > > Signed-off-by: Taehee Yoo > > > > > > --- > > > > > > net/core/xdp.c | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/net/core/xdp.c b/net/core/xdp.c > > > > > > index 9d1f220..1c12bc7 100644 > > > > > > --- a/net/core/xdp.c > > > > > > +++ b/net/core/xdp.c > > > > > > @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct= xdp_mem_info *mem, bool napi_direct, > > > > > > rcu_read_lock(); > > > > > > /* mem->id is valid, checked in xdp_rxq_info_reg_me= m_model() */ > > > > > > xa =3D rhashtable_lookup(mem_id_ht, &mem->id, mem_i= d_rht_params); > > > > > > - xa->zc_alloc->free(xa->zc_alloc, handle); > > > > > > + if (xa) > > > > > > + xa->zc_alloc->free(xa->zc_alloc, handle); = =20 > > > > > hmm...It is not clear to me the "!xa" case don't have to be handl= ed? =20 > > > > > > > > Actually I have a more fundamental question about this interface I'= ve > > > > been meaning to ask. > > > > > > > > IIUC free() can happen on any CPU at any time, when whatever device, > > > > socket or CPU this got redirected to completed the TX. IOW there m= ay > > > > be multiple producers. Drivers would need to create spin lock a'la= the > > > > a9744f7ca200 ("xsk: fix potential race in SKB TX completion code") = fix? > > > > =20 > > > > > > Jakub, apologies for the slow response. I'm still in > > > "holiday/hammock&beer mode", but will be back in a week. :-P =20 > > > > Ah, sorry to interrupt! :) > > =20 >=20 > Don't make it a habit! ;-P :-D > > > The idea with the xdp_return_* functions are that an xdp_buff and > > > xdp_frame can have custom allocations schemes. The difference beween > > > struct xdp_buff and struct xdp_frame is lifetime. The xdp_buff > > > lifetime is within the napi context, whereas xdp_frame can have a > > > lifetime longer/outside the napi context. E.g. for a XDP_REDIRECT > > > scenario an xdp_buff is converted to a xdp_frame. The conversion is > > > done in include/net/xdp.h:convert_to_xdp_frame. > > > > > > Currently, the zero-copy MEM_TYPE_ZERO_COPY memtype can *only* be used > > > for xdp_buff, meaning that the lifetime is constrained to a napi > > > context. Further, given an xdp_buff with memtype MEM_TYPE_ZERO_COPY, > > > doing XDP_REDIRECT to a target that is *not* an AF_XDP socket would > > > mean converting the xdp_buff to an xdp_frame. The xdp_frame can then > > > be free'd on any CPU. > > > > > > Note that the xsk_rcv* functions is always called from an napi > > > context, and therefore is using the xdp_return_buff calls. > > > > > > To answer your question -- no, this fix is *not* needed, because the > > > xdp_buff napi constrained, and the xdp_buff will only be free'd on one > > > CPU. =20 > > > > Oh, thanks, I missed the check in convert_to_xdp_frame(), so the only > > frames which can come back via the free path are out of the error path > > in __xsk_rcv_zc()? > > > > That path looks a little surprising too, isn't the expectation that if > > xdp_do_redirect() returns an error the driver retains the ownership of > > the buffer? > > > > static int __xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 = len) > > { > > int err =3D xskq_produce_batch_desc(xs->rx, (u64)xdp->handle, l= en); > > > > if (err) { > > xdp_return_buff(xdp); > > xs->rx_dropped++; > > } > > > > return err; > > } > > > > This seems to call xdp_return_buff() *and* return an error. > > =20 >=20 > Ugh, this is indeed an error. The xdp_return buff should be removed. > Thanks for spotting this! >=20 > So, yes, the way to get the buffer back (in ZC) to the driver is via > the error path (recycling) or via the UMEM fill queue. Okay, I'm gonna test and submit this later today for bpf tree: diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c index 72335c2e8108..4e937cd7c17d 100644 --- a/net/xdp/xsk.c +++ b/net/xdp/xsk.c @@ -84,10 +84,8 @@ static int __xsk_rcv_zc(struct xdp_sock *xs, struct xdp_= buff *xdp, u32 len) { int err =3D xskq_produce_batch_desc(xs->rx, (u64)xdp->handle, len); =20 - if (err) { - xdp_return_buff(xdp); + if (err) xs->rx_dropped++; - } =20 return err; } Now the frame return/ZC allocator ->free path is all dead code :S