netdev - Re: [RFC net-next 5/8] nfp: bpf: take a reference on offloaded programs

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]

Message-ID: <20170619163441.17c29f77@cakuba.netronome.com>
Date:   Mon, 19 Jun 2017 16:34:41 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kafai@...com,
        brouer@...hat.com, oss-drivers@...ronome.com
Subject: Re: [RFC net-next 5/8] nfp: bpf: take a reference on offloaded
 programs

On Tue, 20 Jun 2017 01:23:05 +0200, Daniel Borkmann wrote:
> On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
> > The xdp_prog member of the adapter's data path structure is used
> > for XDP in driver mode.  In case a XDP program is loaded with in
> > HW-only mode, we need to store it somewhere else.  Add a new XDP
> > prog pointer in the main structure and use that when we need to
> > know whether any XDP program is loaded, not only a driver mode
> > one.  Only release our reference on adapter free instead of
> > immediately after netdev unregister to allow offload to be disabled
> > first.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>  
> [...]
> > @@ -3327,6 +3323,10 @@ nfp_net_xdp_setup(struct nfp_net *nn, struct bpf_prog 
> >   		return err;
> >
> >   	nfp_app_xdp_offload(nn->app, nn, offload_prog);
> > +
> > +	if (nn->xdp_prog)
> > +		bpf_prog_put(nn->xdp_prog);
> > +	nn->xdp_prog = prog;
> >   	nn->xdp_flags = flags;
> >
> >   	return 0;  
> 
> Can you elaborate on the extra reference on the prog? 

Sorry, this patch went through a few revisions and the subject doesn't
express the intent too well any more :S  Originally it was about making
sure we have a reference on the program when it's offloaded but not
loaded in the driver, but I realized the we have the reference from 
dev_change_xdp_fd() already, so now the patch just releases the
reference on the offloaded program.

> So in nfp_net_xdp_setup(), assuming a prog was already loaded on
> driver side: after your set, nfp_net_xdp_setup_drv() will then
> do the xchg() on nn->dp.xdp_prog, bpf_prog_put() this one and
> later back in nfp_net_xdp_setup() we check nn->xdp_prog and
> bpf_prog_put() it if it existed before and update nn->xdp_prog
> to the current prog. So you end up with two puts on the same
> program, but I don't see where you take the one additional ref
> aside from the ref that you already get from dev_change_xdp_fd().
> What am I missing?

You are right, I missed there were two spots where I was doing a
bpf_prog_put() in nfp_net_xdp_setup_drv(), thanks!