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

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

On 05/05/2017 04:01 PM, Alexander Duyck wrote:
> 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.

You know,  I did that and that was the patch I meant to send... I had
3 different versions of this thing trying to find the best way...

Let me repost, since some of the rest of the changes go away.

-vlad

> 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
>>