netdev - Re: [PATCH] vlan: Keep NETIF_F_HW_CSUM similar to other software devices

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]

Date:   Fri, 5 May 2017 13:01:03 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Vladislav Yasevich <vyasevich@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>, Michal Kubecek <mkubecek@...e.cz>,
        avagin@...nvz.org, Vladislav Yasevich <vyasevic@...hat.com>
Subject: Re: [PATCH] vlan: Keep NETIF_F_HW_CSUM similar to other software devices

On Fri, May 5, 2017 at 12:20 PM, Vladislav Yasevich <vyasevich@...il.com> wrote:
> Vlan devices, like all other software devices, enable
> NETIF_F_HW_CSUM feature.  However, unlike all the othe other
> software devices, vlans will switch to using IP|IPV6_CSUM
> features, if the underlying devices uses them.  In these situations,
> checksum offload features on the vlan device can't be controlled
> via ethtool.
>
> This patch makes vlans keep HW_CSUM feature if the underlying
> device supports checksum offloading.  This makes vlan devices
> behave like other software devices, and restores control to the
> user.
>
> A side-effect is that some offload settings (typically UFO)
> may be enabled on the vlan device while being disabled on the HW.
> However, the GSO code will correctly process the packets. This
> actually results in slightly better raw throughput.
>
> Signed-off-by: Vladislav Yasevich <vyasevic@...hat.com>
> ---
>  net/8021q/vlan_dev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 9ee5787..ffc8167 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -626,10 +626,16 @@ static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
>  {
>         struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
>         netdev_features_t old_features = features;
> +       netdev_features_t real_dev_features = real_dev->features;
>
> -       features = netdev_intersect_features(features, real_dev->vlan_features);
> +       features = netdev_intersect_features(features,
> +                                            (real_dev->vlan_features |
> +                                             NETIF_F_HW_CSUM));

You might want to change the ordering on all this.

You could start out with a value based on the intersection of
real_dev->features and real_dev->vlan_features. Then you don't need to
mess around with this extra piece where you are having OR in HW_CSUM.
That way you don't risk losing track of the state of the hardware
checksum offload in terms of vlan_features as it should completely
clear all of the checksums if none of them are supported in hardware.

>         features |= NETIF_F_RXCSUM;

This line would probably need to be changed to OR NETIF_F_RXCSUM with
the real_dev->vlan_features when we perform the first intersect test.
That way we are guaranteed to report RXCSUM if the underlying device
supports it. It might just be worth while to force this into the
vlan_features for all devices in register_netdevice() then we wouldn't
need this line at all and it probably makes sense since it would allow
us to save a few extra cycles/instructions by combining it with the
line that was adding high dma support.

> -       features = netdev_intersect_features(features, real_dev->features);
> +       if (real_dev_features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> +               real_dev_features |= NETIF_F_HW_CSUM;
> +
> +       features = netdev_intersect_features(features, real_dev_features);

This part all looks good.

My only advice like I said would be to record the vlan_features
intersection first based on the real_dev. That way you don't risk
losing state data from real device if for some reason it doesn't
support checksum offload with VLAN tagged frames.

>         features |= old_features & (NETIF_F_SOFT_FEATURES | NETIF_F_GSO_SOFTWARE);
>         features |= NETIF_F_LLTX;
> --
> 2.7.4
>