netdev - Re: [RFC PATCH 0/6] BOND TLS flags fixes

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: <8182c05b-03ab-1052-79b8-3cdf7ab467b5@gmail.com>
Date:   Thu, 27 May 2021 17:07:06 +0300
From:   Tariq Toukan <ttoukan.linux@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>, Tariq Toukan <tariqt@...dia.com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Moshe Shemesh <moshe@...dia.com>,
        Boris Pismenny <borisp@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Maxim Mikityanskiy <maximmi@...dia.com>
Subject: Re: [RFC PATCH 0/6] BOND TLS flags fixes



On 5/27/2021 3:47 AM, Jakub Kicinski wrote:
> On Wed, 26 May 2021 12:57:41 +0300 Tariq Toukan wrote:
>> This RFC series suggests a solution for the following problem:
>>
>> Bond interface and lower interface are both up with TLS RX/TX offloads on.
>> TX/RX csum offload is turned off for the upper, hence RX/TX TLS is turned off
>> for it as well.
>> Yet, although it indicates that feature is disabled, new connections are still
>> offloaded by the lower, as Bond has no way to impact that:
>> Return value of bond_sk_get_lower_dev() is agnostic to this change.
>>
>> One way to solve this issue, is to bring back the Bond TLS operations callbacks,
>> i.e. provide implementation for struct tlsdev_ops in Bond.
>> This gives full control for the Bond over its features, making it aware of every
>> new TLS connection offload request.
>> This direction was proposed in the original Bond TLS implementation, but dropped
>> during ML review. Probably it's right to re-consider now.
>>
>> Here I suggest another solution, which requires generic changes out of the bond
>> driver.
>>
>> Fixes in patches 1 and 4 are needed anyway, independently to which solution
>> we choose. I'll probably submit them separately soon.
> 
> No opinions here, semantics of bond features were always clear
> as mud to me. What does it mean that bond survived 20 years without
> rx-csum? And it so why would TLS offload be different from what one
> may presume the semantics of rx-csum are today? 🤷🏻‍♂️
> 

Hi Jakub,

Advanced device offloads have basic logical dependencies, that are 
applied for all kind of netdevs, agnostic to internal details of each 
netdev.

Nothing special with TLS really.
TLS device offload behaves similarly to TSO (needs HW_CSUM), and GRO_HW 
(needs RXCSUM).

For TLS TX:
Dependency problem doesn't exist for bond because HW_CSUM is already 
supported. That's why TSO is available on bond.

Currently, TLS RX is blocked, as RXCSUM is cleared.
Why wouldn't RX side of bond act in a symmetric way to TX?

Moreover:
Today, bond *does* support NETIF_F_LRO (find it in BOND_VLAN_FEATURES).
It should mean that GRO_HW can be easily enabled as well. But no, it's 
blocked by the missing RXCSUM.

Actually, I think that this code below that blocks GRO_HW if no RXCSUM 
should be extended to block LRO as well. But this would make a 
degradation in bond, unless RXCSUM.

if (!(features & NETIF_F_RXCSUM)) {
         /* NETIF_F_GRO_HW implies doing RXCSUM since every packet
          * successfully merged by hardware must also have the
          * checksum verified by hardware.  If the user does not
          * want to enable RXCSUM, logically, we should disable GRO_HW.
          */
         if (features & NETIF_F_GRO_HW) {
                 netdev_dbg(dev, "Dropping NETIF_F_GRO_HW since no 
RXCSUM feature.\n");
                 features &= ~NETIF_F_GRO_HW;
         }
}

More relevant code from netdev_fix_features():

if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
                                 !(features & NETIF_F_IP_CSUM)) {
         netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
         features &= ~NETIF_F_TSO;
         features &= ~NETIF_F_TSO_ECN;
}

if ((features & NETIF_F_TSO6) && !(features & NETIF_F_HW_CSUM) &&
                                  !(features & NETIF_F_IPV6_CSUM)) {
         netdev_dbg(dev, "Dropping TSO6 features since no CSUM feature.\n");
         features &= ~NETIF_F_TSO6;
}

Thanks,
Tariq