Den ons 1 aug. 2018 kl 22:25 skrev Daniel Borkmann <dan...@iogearbox.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 > > <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(). >
I'll do that (as soon as I've double-checked so that I'm not lying)! Björn > Thanks, > Daniel
