netdev - Re: [PATCH bpf] 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:   Wed, 1 Aug 2018 16:43:32 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Taehee Yoo <ap420073@...il.com>, kafai@...com,
        Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org,
        Björn Töpel <bjorn.topel@...el.com>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

Den ons 1 aug. 2018 kl 16:14 skrev Jesper Dangaard Brouer <brouer@...hat.com>:
>
> On Mon, 23 Jul 2018 11:41:02 +0200
> Björn Töpel <bjorn.topel@...il.com> wrote:
>
> > > >> 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_mem_model() */
> > > >>               xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> > > >> -             xa->zc_alloc->free(xa->zc_alloc, handle);
> > > >> +             if (xa)
> > > >> +                     xa->zc_alloc->free(xa->zc_alloc, handle);
> > > > hmm...It is not clear to me the "!xa" case don't have to be handled?
> > >
> > > Thank you for reviewing!
> > >
> > > Returning NULL pointer is bug case such as calling after use
> > > xdp_rxq_info_unreg().
> > > so that, I think it can't handle at that moment.
> > > we can make __xdp_return to add WARN_ON_ONCE() or
> > > add return error code to driver.
> > > But I'm not sure if these is useful information.
> > >
> > > I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
> > > because there is no use case of MEM_TYPE_ZERO_COPY yet.
> > >
> >
> > Taehee, again, sorry for the slow response and thanks for patch!
> >
> > If xa is NULL, the driver has a buggy/broken implementation. What
> > would be a proper way of dealing with this? BUG?
>
> Hmm... I don't like these kind of changes to the hot-path code!
>
> You might not realize this, but adding BUG() and WARN_ON() to the code
> affect performance in ways you might not realize!  These macros gets
> compiled and uses an asm instruction called "ud2".  Seeing the "ud2"
> instruction causes the CPUs instruction cache prefetcher to stop.
> Thus, if some code ends up below this instruction, this will cause more
> i-cache-misses.
>
> I don't know if xa==NULL is even possible, but if it is, then I think
> this is a result of a driver mem_reg API usage bug.  And the mem-reg
> API is full of WARN's and error messages, exactly to push these kind of
> checks out of the fast-path.  There is no need for a BUG() call, as
> deref a NULL pointer will case an OOPS, that is easy to read and
> understand.
>

Jesper, thanks for having a look! So, you're right that if xa==NULL
the driver is "broken/buggy" (as stated earlier!). I agree that
OOPSing on a NULL pointer is as good as a BUG!

The applied patch adds a WARN_ON_ONCE, and I thought best practice was
that a buggy driver shouldn't crash the kernel... What is considered
best practices in these scenarios? *I'd* prefer an OOPS instead of
WARN_ON_ONCE, to catch that buggy driver. Again, that's me. I thought
that most people prefer not crashing, hence the patch. :-)


Björn

> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer