netdev - Re: [PATCH v2 net] bonding: add 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]

Message-ID: <5a1c7de53daaa6180b207ff42d1736f50b5d90b9.camel@redhat.com>
Date:   Tue, 02 May 2023 11:28:53 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org
Cc:     lorenzo.bianconi@...hat.com, j.vosburgh@...il.com,
        andy@...yhouse.net, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, bpf@...r.kernel.org, andrii@...nel.org,
        mykolal@...com, ast@...nel.org, martin.lau@...ux.dev,
        alardam@...il.com, memxor@...il.com, sdf@...gle.com,
        brouer@...hat.com, toke@...hat.com, Jussi Maki <joamaki@...il.com>
Subject: Re: [PATCH v2 net] bonding: add xdp_features support

On Mon, 2023-05-01 at 15:33 +0200, Daniel Borkmann wrote:
> On 4/30/23 12:02 PM, Lorenzo Bianconi wrote:
> > Introduce xdp_features support for bonding driver according to the slave
> > devices attached to the master one. xdp_features is required whenever we
> > want to xdp_redirect traffic into a bond device and then into selected
> > slaves attached to it.
> > 
> > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
> > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> 
> Please also keep Jussi in Cc for bonding + XDP reviews [added here].

Perhaps worth adding such info to the maintainer file for future
memory?

> > ---
> > Change since v1:
> > - remove bpf self-test patch from the series
> 
> Given you targeted net tree, was this patch run against BPF CI locally from
> your side to avoid breakage again?
> 
> Thanks,
> Daniel
> 
> > ---
> >   drivers/net/bonding/bond_main.c    | 48 ++++++++++++++++++++++++++++++
> >   drivers/net/bonding/bond_options.c |  2 ++
> >   include/net/bonding.h              |  1 +
> >   3 files changed, 51 insertions(+)
> > 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 710548dbd0c1..c98121b426a4 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -1789,6 +1789,45 @@ static void bond_ether_setup(struct net_device *bond_dev)
> >   	bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> >   }
> >   
> > +void bond_xdp_set_features(struct net_device *bond_dev)
> > +{
> > +	struct bonding *bond = netdev_priv(bond_dev);
> > +	xdp_features_t val = NETDEV_XDP_ACT_MASK;
> > +	struct list_head *iter;
> > +	struct slave *slave;
> > +
> > +	ASSERT_RTNL();
> > +
> > +	if (!bond_xdp_check(bond)) {
> > +		xdp_clear_features_flag(bond_dev);
> > +		return;
> > +	}
> > +
> > +	bond_for_each_slave(bond, slave, iter) {
> > +		struct net_device *dev = slave->dev;
> > +
> > +		if (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) {
> > +			xdp_clear_features_flag(bond_dev);
> > +			return;
> > +		}
> > +
> > +		if (!(dev->xdp_features & NETDEV_XDP_ACT_REDIRECT))
> > +			val &= ~NETDEV_XDP_ACT_REDIRECT;
> > +		if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT))
> > +			val &= ~NETDEV_XDP_ACT_NDO_XMIT;
> > +		if (!(dev->xdp_features & NETDEV_XDP_ACT_XSK_ZEROCOPY))
> > +			val &= ~NETDEV_XDP_ACT_XSK_ZEROCOPY;
> > +		if (!(dev->xdp_features & NETDEV_XDP_ACT_HW_OFFLOAD))
> > +			val &= ~NETDEV_XDP_ACT_HW_OFFLOAD;
> > +		if (!(dev->xdp_features & NETDEV_XDP_ACT_RX_SG))
> > +			val &= ~NETDEV_XDP_ACT_RX_SG;
> > +		if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG))
> > +			val &= ~NETDEV_XDP_ACT_NDO_XMIT_SG;

Can we expect NETDEV_XDP_ACT_MASK changing in the future (e.g. new
features to be added)? If so the above code will break silently, as the
new features will be unconditionally enabled. What about adding a
BUILD_BUG() to catch such situation? 

> 
Cheers,

Paolo