netdev - Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 10 Aug 2018 17:15:07 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Netdev <netdev@...r.kernel.org>, Taehee Yoo <ap420073@...il.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>, kafai@...com,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"

Den fre 10 aug. 2018 kl 12:18 skrev Jesper Dangaard Brouer <brouer@...hat.com>:
>
> On Fri, 10 Aug 2018 11:28:02 +0200
> Björn Töpel <bjorn.topel@...il.com> wrote:
>
> > From: Björn Töpel <bjorn.topel@...el.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@...hat.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.
>
>

Thanks for putting your visions/ideas here! I agree with both of your
last sections, and this is what we're working towards. AF_XDP ZC has
to play nicer with XDP.  The current (well, the soon-to-be-published
[1] ;-)) ZC scheme is just a first step, and should be seen as a
starting point so people can start playing using AF_XDP. Jakub also
mentioned these issues a couple of threads ago, so there are
definitely more people feeling the ZC allocator pains. Mid-term a
sophisticated/proper and generic (for inter-driver reuse) ZC allocator
is needed; Converting xdp_buffs to xdp_frames cheaply for multi-CPU
completion, and hopefully dito for the XDP_PASS/kernel stack path. But
let's start with something simple that works, and take it from there.

Björn

[1] WIP: https://github.com/bjoto/linux/tree/af-xdp-i40e-zc

> > Suggested-by: Jesper Dangaard Brouer <brouer@...hat.com>
> > Signed-off-by: Björn Töpel <bjorn.topel@...el.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