netdev - Re: [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for 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] [day] [month] [year] [list]

Date:   Mon, 17 Apr 2023 13:20:21 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
Cc:     Daniel Borkmann <daniel@...earbox.net>, bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Jussi Maki <joamaki@...il.com>
Subject: Re: [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for
 xdp_bonding selftest

On Sat, Apr 15, 2023 at 4:06 AM Lorenzo Bianconi <lorenzo@...nel.org> wrote:
>
> > On 4/15/23 12:10 AM, Lorenzo Bianconi wrote:
> > > > On 4/14/23 11:21 PM, Lorenzo Bianconi wrote:
> > > > > NETDEV_XDP_ACT_NDO_XMIT is not enabled by default for veth driver but it
> > > > > depends on the device configuration. Fix XDP_REDIRECT xdp-features in
> > > > > xdp_bonding selftest loading a dummy XDP program on veth2_2 device.
> > > > >
> > > > > Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag")
> > > >
> > > > Hm, does that mean we're changing^breaking existing user behavior iff after
> > > > fccca038f300 you can only make it work by loading dummy prog?
> > >
> > > nope, even before in order to enable ndo_xdp_xmit for veth you should load a dummy
> > > program on the device peer or enable gro on the device peer:
> > >
> > > https://github.com/torvalds/linux/blob/master/drivers/net/veth.c#L477
> > >
> > > we are just reflecting this behaviour in the xdp_features flag.
> >
> > Ok, I'm confused then why it passed before?
>
> ack, you are right. I guess the issue is in veth driver code. In order to
> enable NETDEV_XDP_ACT_NDO_XMIT for device "veth0", we need to check the peer
> veth1 configuration since the check in veth_xdp_xmit() is on the peer rx queue.
> Something like:
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index e1b38fbf1dd9..4b3c6647edc6 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -1262,11 +1262,12 @@ static void veth_set_xdp_features(struct net_device *dev)
>
>         peer = rtnl_dereference(priv->peer);
>         if (peer && peer->real_num_tx_queues <= dev->real_num_rx_queues) {
> +               struct veth_priv *priv_peer = netdev_priv(peer);
>                 xdp_features_t val = NETDEV_XDP_ACT_BASIC |
>                                      NETDEV_XDP_ACT_REDIRECT |
>                                      NETDEV_XDP_ACT_RX_SG;
>
> -               if (priv->_xdp_prog || veth_gro_requested(dev))
> +               if (priv_peer->_xdp_prog || veth_gro_requested(peer))
>                         val |= NETDEV_XDP_ACT_NDO_XMIT |
>                                NETDEV_XDP_ACT_NDO_XMIT_SG;
>                 xdp_set_features_flag(dev, val);
> @@ -1504,19 +1505,23 @@ static int veth_set_features(struct net_device *dev,
>  {
>         netdev_features_t changed = features ^ dev->features;
>         struct veth_priv *priv = netdev_priv(dev);
> +       struct net_device *peer;
>         int err;
>
>         if (!(changed & NETIF_F_GRO) || !(dev->flags & IFF_UP) || priv->_xdp_prog)
>                 return 0;
>
> +       peer = rtnl_dereference(priv->peer);
>         if (features & NETIF_F_GRO) {
>                 err = veth_napi_enable(dev);
>                 if (err)
>                         return err;
>
> -               xdp_features_set_redirect_target(dev, true);
> +               if (peer)
> +                       xdp_features_set_redirect_target(peer, true);
>         } else {
> -               xdp_features_clear_redirect_target(dev);
> +               if (peer)
> +                       xdp_features_clear_redirect_target(peer);
>                 veth_napi_del(dev);
>         }
>         return 0;
> @@ -1598,13 +1603,13 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>                         peer->max_mtu = max_mtu;
>                 }
>
> -               xdp_features_set_redirect_target(dev, true);
> +               xdp_features_set_redirect_target(peer, true);
>         }
>
>         if (old_prog) {
>                 if (!prog) {
> -                       if (!veth_gro_requested(dev))
> -                               xdp_features_clear_redirect_target(dev);
> +                       if (peer && !veth_gro_requested(dev))
> +                               xdp_features_clear_redirect_target(peer);
>
>                         if (dev->flags & IFF_UP)
>                                 veth_disable_xdp(dev);
>
> What do you think?

Please send an official patch.
We need to fix this regression asap.