From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesper Dangaard Brouer Subject: Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()" Date: Fri, 10 Aug 2018 12:18:04 +0200 Message-ID: <20180810121804.22aec831@redhat.com> References: <20180810092802.5948-1-bjorn.topel@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Cc: ast@fb.com, daniel@iogearbox.net, netdev@vger.kernel.org, ap420073@gmail.com, =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= , magnus.karlsson@intel.com, magnus.karlsson@gmail.com, kafai@fb.com, brouer@redhat.com To: =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= Return-path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:60740 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727258AbeHJMrZ (ORCPT ); Fri, 10 Aug 2018 08:47:25 -0400 In-Reply-To: <20180810092802.5948-1-bjorn.topel@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, 10 Aug 2018 11:28:02 +0200 Björn Töpel wrote: > From: Björn Töpel > > This reverts commit 36e0f12bbfd3016f495904b35e41c5711707509f. > > The reverted commit adds a WARN to check against NULL entries in the > mem_id_ht rhashtable. Any kernel path implementing the XDP (generic or > driver) fast path is required to make a paired > xdp_rxq_info_reg/xdp_rxq_info_unreg call for proper function. In > addition, a driver using a different allocation scheme than the > default MEM_TYPE_PAGE_SHARED is required to additionally call > xdp_rxq_info_reg_mem_model. > > For MEM_TYPE_ZERO_COPY, an xdp_rxq_info_reg_mem_model call ensures > that the mem_id_ht rhashtable has a properly inserted allocator id. If > not, this would be a driver bug. A NULL pointer kernel OOPS is > preferred to the WARN. Acked-by: Jesper Dangaard Brouer As a comment says in the code: /* NB! Only valid from an xdp_buff! */ Which is (currently) guarded by the return/exit in convert_to_xdp_frame(). This means that this code path can only be invoked while the driver is still running under the RX NAPI process. Thus, there is no chance that the allocator-id is gone (via calling xdp_rxq_info_unreg) for this code path. But I really hope we at somepoint can convert a MEM_TYPE_ZERO_COPY into a form of xdp_frame, that can travel further into the redirect-core. In which case, we likely need to handle the NULL case (but also need other code to handle what to do with the memory backing the frame) (I'm my vision here:) I really dislike that the current Zero-Copy mode steal ALL packets, when ZC is enabled on a RX-queue. This is not better than the existing bypass solutions, which have ugly ways of re-injecting packet back into the network stack. With the integration with XDP, we have the flexibility of selecting frames, that we don't want to be "bypassed" into AF_XDP, and want the kernel process these. (The most common use-case is letting the kernel handle the arptable). IHMO this is what will/would make AF_XDP superior to other bypass solutions. > Suggested-by: Jesper Dangaard Brouer > Signed-off-by: Björn Töpel > --- > net/core/xdp.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/core/xdp.c b/net/core/xdp.c > index 6771f1855b96..9d1f22072d5d 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -345,8 +345,7 @@ 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_mem_model() */ > xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params); > - if (!WARN_ON_ONCE(!xa)) > - xa->zc_alloc->free(xa->zc_alloc, handle); > + xa->zc_alloc->free(xa->zc_alloc, handle); > rcu_read_unlock(); > default: > /* Not possible, checked in xdp_rxq_info_reg_mem_model() */ -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer