netdev - Re: [PATCH net-next v2] xen-netfront: add basic XDP support

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] [day] [month] [year] [list]

Message-ID: <20200306182241.43uocmmodfoaf4jo@debian>
Date:   Fri, 6 Mar 2020 18:22:41 +0000
From:   Wei Liu <wei.liu@...nel.org>
To:     Jürgen Groß <jgross@...e.com>
Cc:     Denis Kirjanov <kda@...ux-powerpc.org>, netdev@...r.kernel.org,
        "ilias.apalodimas" <ilias.apalodimas@...aro.org>,
        wei.liu@...nel.org, paul@....org
Subject: Re: [PATCH net-next v2] xen-netfront: add basic XDP support

On Thu, Mar 05, 2020 at 11:33:48AM +0100, Jürgen Groß wrote:
> On 05.03.20 10:47, Denis Kirjanov wrote:
> > On 3/4/20, Jürgen Groß <jgross@...e.com> wrote:
> > > On 04.03.20 14:10, Denis Kirjanov wrote:
> > > > On 3/2/20, Jürgen Groß <jgross@...e.com> wrote:
> > > > > On 02.03.20 15:21, Denis Kirjanov wrote:
> > > > > > the patch adds a basic xdo logic to the netfront driver
> > > > > > 
> > > > > > XDP redirect is not supported yet
> > > > > > 
> > > > > > v2:
> > > > > > - avoid data copying while passing to XDP
> > > > > > - tell xen-natback that we need the headroom space
> > > > > 
> > > > > Please add the patch history below the "---" delimiter
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Denis Kirjanov <kda@...ux-powerpc.org>
> > > > > > ---
> > > > > >     drivers/net/xen-netback/common.h |   1 +
> > > > > >     drivers/net/xen-netback/rx.c     |   9 ++-
> > > > > >     drivers/net/xen-netback/xenbus.c |  21 ++++++
> > > > > >     drivers/net/xen-netfront.c       | 157
> > > > > > +++++++++++++++++++++++++++++++++++++++
> > > > > >     4 files changed, 186 insertions(+), 2 deletions(-)
> > > > > 
> > > > > You are modifying xen-netback sources. Please Cc the maintainers.
> > > > > 
> > > 
> > > ...
> > > 
> > > > > > 
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +
> > > > > > +abort_transaction:
> > > > > > +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
> > > > > > +	xenbus_transaction_end(xbt, 1);
> > > > > > +out:
> > > > > > +	return err;
> > > > > > +}
> > > > > > +
> > > > > > +static int xennet_xdp_set(struct net_device *dev, struct bpf_prog
> > > > > > *prog,
> > > > > > +			struct netlink_ext_ack *extack)
> > > > > > +{
> > > > > > +	struct netfront_info *np = netdev_priv(dev);
> > > > > > +	struct bpf_prog *old_prog;
> > > > > > +	unsigned int i, err;
> > > > > > +
> > > > > > +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
> > > > > > +	if (!old_prog && !prog)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	if (prog)
> > > > > > +		bpf_prog_add(prog, dev->real_num_tx_queues);
> > > > > > +
> > > > > > +	for (i = 0; i < dev->real_num_tx_queues; ++i)
> > > > > > +		rcu_assign_pointer(np->queues[i].xdp_prog, prog);
> > > > > > +
> > > > > > +	if (old_prog)
> > > > > > +		for (i = 0; i < dev->real_num_tx_queues; ++i)
> > > > > > +			bpf_prog_put(old_prog);
> > > > > > +
> > > > > > +	err = talk_to_netback_xdp(np, old_prog ?
> > > > > > NETBACK_XDP_HEADROOM_DISABLE:
> > > > > > +				  NETBACK_XDP_HEADROOM_ENABLE);
> > > > > > +	if (err)
> > > > > > +		return err;
> > > > > > +
> > > > > > +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
> > > > > 
> > > > > What is happening in case the backend doesn't support XDP?
> > > > Here we just ask xen-backend to make a headroom, that's it.
> > > > It's better to send xen-backend changes in a separate patch.
> > > 
> > > Okay, but what do you do if the backend doesn't support XDP (e.g. in
> > > case its an older kernel)? How do you know it is supporting XDP?
> > We can check a xenbus reply to xenbus state change.
> 
> Using the frontend state for that purpose is rather dangerous.
> 
> In case the backend doesn't support the "Reconfiguring" state you might
> end up with a broken network.
> 
> I'd rather let the backend advertise the support via a "feature-xdp"
> node in Xenstore and do the activation via another node.

Yes, that's how feature negotiation is supposed to work. If XDP somehow
doesn't fit into this model, we would like to know why at the very
least.

Wei.

> 
> 
> Juergen