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 22:25:48 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Björn Töpel <bjorn.topel@...il.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     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()

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().

Thanks,
Daniel