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] [day] [month] [year] [list]

Message-ID: <CAJ+HfNifXEvZnPVdNEO6xbRTcLV+8MHuOa1qJTYB5J_56xCO2Q@mail.gmail.com>
Date:   Thu, 2 Aug 2018 12:59:29 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Taehee Yoo <ap420073@...il.com>, kafai@...com, 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 22:25 skrev Daniel Borkmann <daniel@...earbox.net>:
>
> On 08/01/2018 04:43 PM, Björn Töpel wrote:
> > 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. :-)
>
> In that case, lets send a revert for the patch with a proper analysis
> of why it is safe to omit the NULL check which should be placed as a
> comment right near the rhashtable_lookup().
>

I'll do that (as soon as I've double-checked so that I'm not lying)!


Björn

> Thanks,
> Daniel