On 08/01/2018 04:43 PM, Björn Töpel wrote: > Den ons 1 aug. 2018 kl 16:14 skrev Jesper Dangaard Brouer <bro...@redhat.com>: >> On Mon, 23 Jul 2018 11:41:02 +0200 >> Björn Töpel <bjorn.to...@gmail.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
