netdev - Re: [bpf-next V1-RFC PATCH 01/14] xdp: base API for new XDP rx-queue info concept

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: <20171218115501.3f1fcf36@redhat.com>
Date:   Mon, 18 Dec 2017 11:55:01 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     David Ahern <dsahern@...il.com>
Cc:     Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        netdev@...r.kernel.org, gospo@...adcom.com, bjorn.topel@...el.com,
        michael.chan@...adcom.com, brouer@...hat.com
Subject: Re: [bpf-next V1-RFC PATCH 01/14] xdp: base API for new XDP
 rx-queue info concept

On Wed, 13 Dec 2017 19:34:40 -0700
David Ahern <dsahern@...il.com> wrote:

> On 12/13/17 4:19 AM, Jesper Dangaard Brouer wrote:
> > +
> > +void xdp_rxq_info_unreg(struct xdp_rxq_info *xdp_rxq)
> > +{
> > +	xdp_rxq->reg_state = REG_STATE_UNREGISTRED;
> > +}
> > +EXPORT_SYMBOL_GPL(xdp_rxq_info_unreg);
> > +
> > +void xdp_rxq_info_init(struct xdp_rxq_info *xdp_rxq)
> > +{
> > +	if (xdp_rxq->reg_state == REG_STATE_REGISTRED) {
> > +		WARN(1, "Missing unregister, handled but fix driver\n");
> > +		xdp_rxq_info_unreg(xdp_rxq);
> > +	}
> > +	memset(xdp_rxq, 0, sizeof(*xdp_rxq));
> > +	xdp_rxq->queue_index = U32_MAX;
> > +	xdp_rxq->reg_state = REG_STATE_NEW;
> > +}
> > +EXPORT_SYMBOL_GPL(xdp_rxq_info_init);
> > +
> > +void xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq)
> > +{
> > +	WARN(!xdp_rxq->dev, "Missing net_device from driver");
> > +	WARN(xdp_rxq->queue_index == U32_MAX, "Miss queue_index from driver");
> > +	WARN(!(xdp_rxq->reg_state == REG_STATE_NEW),"API violation, miss init");
> > +	xdp_rxq->reg_state = REG_STATE_REGISTRED;
> > +}
> > +EXPORT_SYMBOL_GPL(xdp_rxq_info_reg);
> >   
> 
> Rather than WARN()'s why not make the _reg and _init functions return an
> int that indicates an error? For example you don't want to continue if
> the dev is expected but missing.

Handling return-errors in the drivers complicated the driver code, as it
involves unraveling and deallocating other RX-rings etc (that were
already allocated) if the reg fails. (Also notice next patch will allow
dev == NULL, if right ptype is set).

I'm not completely rejecting you idea, as this is a good optimization
trick, which is to move validation checks to setup-time, thus allowing
less validation checks at runtime.  I sort-of actually already did
this, as I allow bpf to deref dev without NULL check.  I would argue
this is good enough, as we will crash in a predictable way, as above
WARN will point to which driver violated the API.

If people think it is valuable I can change this API to return an err?

I guess, it would be more future-proof to do this, as we (Bjørn,
Michael, Andy) want to extend this to implement a XDP frame/mem return
code-path.  And the register call will likely have to allocate some
resource that could fail, which need to be handled...

If we do this, we might as well (slab) alloc the xdp_rxq_info
structure to reduce the bloat in the drivers RX-rings to a single
pointer (and a pointer to xdp_rxq_info is what xdp_buff.rxq need).

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