From: Jesper Dangaard Brouer <brouer@redhat•com>
To: "Björn Töpel" <bjorn.topel@gmail•com>
Cc: ast@fb•com, daniel@iogearbox•net, netdev@vger•kernel.org,
ap420073@gmail•com, "Björn Töpel" <bjorn.topel@intel•com>,
magnus.karlsson@intel•com, magnus.karlsson@gmail•com,
kafai@fb•com, brouer@redhat•com
Subject: Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"
Date: Fri, 10 Aug 2018 12:18:04 +0200 [thread overview]
Message-ID: <20180810121804.22aec831@redhat.com> (raw)
In-Reply-To: <20180810092802.5948-1-bjorn.topel@gmail.com>
On Fri, 10 Aug 2018 11:28:02 +0200
Björn Töpel <bjorn.topel@gmail•com> wrote:
> From: Björn Töpel <bjorn.topel@intel•com>
>
> 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 <brouer@redhat•com>
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 <brouer@redhat•com>
> Signed-off-by: Björn Töpel <bjorn.topel@intel•com>
> ---
> 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
next prev parent reply other threads:[~2018-08-10 12:47 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-10 9:28 [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()" Björn Töpel
2018-08-10 10:18 ` Jesper Dangaard Brouer [this message]
2018-08-10 15:15 ` Björn Töpel
2018-08-10 16:23 ` Jakub Kicinski
2018-08-10 14:10 ` Daniel Borkmann
2018-08-10 15:16 ` Björn Töpel
2018-08-10 16:26 ` Jakub Kicinski
2018-08-14 12:02 ` Björn Töpel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180810121804.22aec831@redhat.com \
--to=brouer@redhat$(echo .)com \
--cc=ap420073@gmail$(echo .)com \
--cc=ast@fb$(echo .)com \
--cc=bjorn.topel@gmail$(echo .)com \
--cc=bjorn.topel@intel$(echo .)com \
--cc=daniel@iogearbox$(echo .)net \
--cc=kafai@fb$(echo .)com \
--cc=magnus.karlsson@gmail$(echo .)com \
--cc=magnus.karlsson@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox