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:   Fri, 20 Jul 2018 13:05:38 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     <daniel@...earbox.net>, <bjorn.topel@...el.com>,
        <brouer@...hat.com>
Cc:     Martin KaFai Lau <kafai@...com>, Taehee Yoo <ap420073@...il.com>,
        <ast@...nel.org>, <netdev@...r.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>
Subject: Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

On Fri, 20 Jul 2018 10:18:21 -0700, Martin KaFai Lau wrote:
> On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
> > rhashtable_lookup() can return NULL. so that NULL pointer
> > check routine should be added.
> > 
> > Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
> > Signed-off-by: Taehee Yoo <ap420073@...il.com>
> > ---
> >  net/core/xdp.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > 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?

Actually I have a more fundamental question about this interface I've
been meaning to ask.  

IIUC free() can happen on any CPU at any time, when whatever device,
socket or CPU this got redirected to completed the TX.  IOW there may
be multiple producers.  Drivers would need to create spin lock a'la the
a9744f7ca200 ("xsk: fix potential race in SKB TX completion code") fix?

We need some form of internal kernel circulation which would be MPSC.
I'm currently hacking up the XSK code to tell me whether the frame was
consumed by the correct XSK, and always clone the frame otherwise
(claiming to be the "traditional" MEM_TYPE_PAGE_ORDER0).

I feel like I'm missing something about the code.  Is redirect of
ZC/UMEM frame outside the xsk not possible and the only returns we will
see are from net/xdp/xsk.c?  That would work, but I don't see such a
check.  Help would be appreciated.

Also the fact that XSK bufs can't be freed, only completed, adds to the
pain of implementing AF_XDP, we'd certainly need some form of "give
back the frame, but I may need it later" SPSC mechanism, otherwise
driver writers will have tough time.  Unless, again, I'm missing
something about the code :)

> >  		rcu_read_unlock();
> >  	default:
> >  		/* Not possible, checked in xdp_rxq_info_reg_mem_model() */