Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Mon, 23 Jul 2018 11:41:02 +0200
Björn Töpel <bjorn.topel@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.

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