netdev - Re: [PATCH v7 02/11] net: add ndo to setup/query xdp prog in adapter rx

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: <20160712063457.GB20499@gmail.com>
Date:	Mon, 11 Jul 2016 23:35:00 -0700
From:	Brenden Blanco <bblanco@...mgrid.com>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, jhs@...atatu.com,
	saeedm@....mellanox.co.il, kafai@...com, brouer@...hat.com,
	as754m@....com, alexei.starovoitov@...il.com, gerlitz.or@...il.com,
	john.fastabend@...il.com, hannes@...essinduktion.org,
	tgraf@...g.ch, tom@...bertland.com, daniel@...earbox.net
Subject: Re: [PATCH v7 02/11] net: add ndo to setup/query xdp prog in adapter
 rx

On Mon, Jul 11, 2016 at 11:12:24PM -0700, David Miller wrote:
> From: Brenden Blanco <bblanco@...mgrid.com>
> Date: Mon, 11 Jul 2016 14:29:49 -0700
> 
> > +	if (fd >= 0) {
> > +		prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP);
> > +		if (IS_ERR(prog))
> > +			return PTR_ERR(prog);
> > +	}
> > +
> > +	xdp.command = XDP_SETUP_PROG;
> > +	xdp.prog = prog;
> > +	err = ops->ndo_xdp(dev, &xdp);
> > +	if (err < 0 && prog)
> > +		bpf_prog_put(prog);
> 
> I don't understand the reference counting on 'prog' here.
> 
> The enumeration documentation states that no matter what, the passed
> in prog doesn't need to be mangaged by the ->ndo_xdp() method.
> 
> Therefore, if that is true, we must always put the 'prog' here if it
> is non-NULL.  Regardless of the 'err' value.
> 
The documentation is unclear then. What I _meant_ to say is that the callee
is not responsible for putting the program on error, but on success it
takes ownership of the reference. In context of that, does the code make
sense? Is there a more conventional way of handling this?

Thanks for your review.