[net] net: feature check mandating HW_CSUM is wrong

[net] net: feature check mandating HW_CSUM is wrong

[Rohit Maheshwari]

Mandating NETIF_F_HW_CSUM to enable TLS offload feature is wrong.
And it broke tls offload feature for the drivers, which are still
using NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM. We should use
NETIF_F_CSUM_MASK instead.

Fixes: ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled")
Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a46334906c94..b1f99287f280 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9643,7 +9643,7 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		}
 	}
 
-	if ((features & NETIF_F_HW_TLS_TX) && !(features & NETIF_F_HW_CSUM)) {
+	if ((features & NETIF_F_HW_TLS_TX) && !(features & NETIF_F_CSUM_MASK)) {
 		netdev_dbg(dev, "Dropping TLS TX HW offload feature since no CSUM feature.\n");
 		features &= ~NETIF_F_HW_TLS_TX;
 	}
-- 
2.18.1

Re: [net] net: feature check mandating HW_CSUM is wrong

[Jakub Kicinski]

On Wed,  6 Jan 2021 23:23:27 +0530 Rohit Maheshwari wrote:
> Mandating NETIF_F_HW_CSUM to enable TLS offload feature is wrong.
> And it broke tls offload feature for the drivers, which are still
> using NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM. We should use
> NETIF_F_CSUM_MASK instead.
> 
> Fixes: ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled")
> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>

Please use Tariq's suggestion.

Please learn to CC appropriate reviewers.

Re: [net] net: feature check mandating HW_CSUM is wrong

[rohit maheshwari]

On 07/01/21 12:47 AM, Jakub Kicinski wrote:
> On Wed,  6 Jan 2021 23:23:27 +0530 Rohit Maheshwari wrote:
>> Mandating NETIF_F_HW_CSUM to enable TLS offload feature is wrong.
>> And it broke tls offload feature for the drivers, which are still
>> using NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM. We should use
>> NETIF_F_CSUM_MASK instead.
>>
>> Fixes: ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled")
>> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
> Please use Tariq's suggestion.
HW_TLS_TX feature is for both IPv4/v6. And If one device is limited to
support only IPv4 checksum offload, TLS offload should be allowed for
that too. So I think putting a check of CSUM_MASK is enough.

Re: [net] net: feature check mandating HW_CSUM is wrong

[Jakub Kicinski]

On Wed, 13 Jan 2021 02:47:51 +0530 rohit maheshwari wrote:
> On 07/01/21 12:47 AM, Jakub Kicinski wrote:
> > On Wed,  6 Jan 2021 23:23:27 +0530 Rohit Maheshwari wrote:  
> >> Mandating NETIF_F_HW_CSUM to enable TLS offload feature is wrong.
> >> And it broke tls offload feature for the drivers, which are still
> >> using NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM. We should use
> >> NETIF_F_CSUM_MASK instead.
> >>
> >> Fixes: ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled")
> >> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>  
> > Please use Tariq's suggestion.  
> HW_TLS_TX feature is for both IPv4/v6. And If one device is limited to
> support only IPv4 checksum offload, TLS offload should be allowed for
> that too. So I think putting a check of CSUM_MASK is enough.

If Tariq does not disagree please repost.

Re: [net] net: feature check mandating HW_CSUM is wrong

[Alexander Duyck]

On Tue, Jan 12, 2021 at 6:43 PM rohit maheshwari <rohitm@chelsio.com> wrote:
>
>
> On 07/01/21 12:47 AM, Jakub Kicinski wrote:
> > On Wed,  6 Jan 2021 23:23:27 +0530 Rohit Maheshwari wrote:
> >> Mandating NETIF_F_HW_CSUM to enable TLS offload feature is wrong.
> >> And it broke tls offload feature for the drivers, which are still
> >> using NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM. We should use
> >> NETIF_F_CSUM_MASK instead.
> >>
> >> Fixes: ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled")
> >> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
> > Please use Tariq's suggestion.
> HW_TLS_TX feature is for both IPv4/v6. And If one device is limited to
> support only IPv4 checksum offload, TLS offload should be allowed for
> that too. So I think putting a check of CSUM_MASK is enough.

The issue is that there is no good software fallback if the packet
arrives at the NIC and it cannot handle the IPv6 TLS offload.

The problem with the earlier patch you had is that it was just
dropping frames if you couldn't handle the offload and "hoping" the
other end would catch it. That isn't a good practice for any offload.
If you have it enabled you have to have a software fallback and in
this case it sounds like you don't have that.

In order to do that you would have to alter the fast path to have to
check if the device is capable per packet which is really an
undesirable setup as it would add considerable overhead and is open to
race conditions.

Re: [net] net: feature check mandating HW_CSUM is wrong

[Tariq Toukan]

On 1/13/2021 5:35 AM, Alexander Duyck wrote:
> On Tue, Jan 12, 2021 at 6:43 PM rohit maheshwari <rohitm@chelsio.com> wrote:
>>
>>
>> On 07/01/21 12:47 AM, Jakub Kicinski wrote:
>>> On Wed,  6 Jan 2021 23:23:27 +0530 Rohit Maheshwari wrote:
>>>> Mandating NETIF_F_HW_CSUM to enable TLS offload feature is wrong.
>>>> And it broke tls offload feature for the drivers, which are still
>>>> using NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM. We should use
>>>> NETIF_F_CSUM_MASK instead.
>>>>
>>>> Fixes: ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled")
>>>> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
>>> Please use Tariq's suggestion.
>> HW_TLS_TX feature is for both IPv4/v6. And If one device is limited to
>> support only IPv4 checksum offload, TLS offload should be allowed for
>> that too. So I think putting a check of CSUM_MASK is enough.
> 
> The issue is that there is no good software fallback if the packet
> arrives at the NIC and it cannot handle the IPv6 TLS offload.
> 
> The problem with the earlier patch you had is that it was just
> dropping frames if you couldn't handle the offload and "hoping" the
> other end would catch it. That isn't a good practice for any offload.
> If you have it enabled you have to have a software fallback and in
> this case it sounds like you don't have that.
> 
> In order to do that you would have to alter the fast path to have to
> check if the device is capable per packet which is really an
> undesirable setup as it would add considerable overhead and is open to
> race conditions.
> 

+1

Are there really such modern devices with missing IPv6 csum 
capabilities, or it's just a missing SW implementation in the device driver?

IMO, it sounds reasonable for this modern TLS device offload to asks for 
a basic requirement such as IPv6 csum offload capability, to save the 
overhead.

Re: [net] net: feature check mandating HW_CSUM is wrong

[rohit maheshwari]

On 13/01/21 10:37 PM, Tariq Toukan wrote:
>
>
> On 1/13/2021 5:35 AM, Alexander Duyck wrote:
>> On Tue, Jan 12, 2021 at 6:43 PM rohit maheshwari <rohitm@chelsio.com> 
>> wrote:
>>>
>>>
>>> On 07/01/21 12:47 AM, Jakub Kicinski wrote:
>>>> On Wed,  6 Jan 2021 23:23:27 +0530 Rohit Maheshwari wrote:
>>>>> Mandating NETIF_F_HW_CSUM to enable TLS offload feature is wrong.
>>>>> And it broke tls offload feature for the drivers, which are still
>>>>> using NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM. We should use
>>>>> NETIF_F_CSUM_MASK instead.
>>>>>
>>>>> Fixes: ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM 
>>>>> is disabled")
>>>>> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
>>>> Please use Tariq's suggestion.
>>> HW_TLS_TX feature is for both IPv4/v6. And If one device is limited to
>>> support only IPv4 checksum offload, TLS offload should be allowed for
>>> that too. So I think putting a check of CSUM_MASK is enough.
>>
>> The issue is that there is no good software fallback if the packet
>> arrives at the NIC and it cannot handle the IPv6 TLS offload.
>>
>> The problem with the earlier patch you had is that it was just
>> dropping frames if you couldn't handle the offload and "hoping" the
>> other end would catch it. That isn't a good practice for any offload.
>> If you have it enabled you have to have a software fallback and in
>> this case it sounds like you don't have that.
>>
>> In order to do that you would have to alter the fast path to have to
>> check if the device is capable per packet which is really an
>> undesirable setup as it would add considerable overhead and is open to
>> race conditions.
>>
>
> +1
>
> Are there really such modern devices with missing IPv6 csum 
> capabilities, or it's just a missing SW implementation in the device 
> driver?
>
> IMO, it sounds reasonable for this modern TLS device offload to asks 
> for a basic requirement such as IPv6 csum offload capability, to save 
> the overhead.
>
I agree with you, all modern devices support V6 csum, but still if we think
logically, we can't limit TLS offload to work only if both IPV4_CSUM  and
IPV6_CSUM are configured/supported. If there is no dependency of IPV6
in running TLS offload with IPv4  and vice-versa, then why should there
be any restriction as such.

Re: [net] net: feature check mandating HW_CSUM is wrong

[Alexander Duyck]

On Thu, Jan 14, 2021 at 9:39 PM rohit maheshwari <rohitm@chelsio.com> wrote:
>
>
> On 13/01/21 10:37 PM, Tariq Toukan wrote:
> >
> >
> > On 1/13/2021 5:35 AM, Alexander Duyck wrote:
> >> On Tue, Jan 12, 2021 at 6:43 PM rohit maheshwari <rohitm@chelsio.com>
> >> wrote:
> >>>
> >>>
> >>> On 07/01/21 12:47 AM, Jakub Kicinski wrote:
> >>>> On Wed,  6 Jan 2021 23:23:27 +0530 Rohit Maheshwari wrote:
> >>>>> Mandating NETIF_F_HW_CSUM to enable TLS offload feature is wrong.
> >>>>> And it broke tls offload feature for the drivers, which are still
> >>>>> using NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM. We should use
> >>>>> NETIF_F_CSUM_MASK instead.
> >>>>>
> >>>>> Fixes: ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM
> >>>>> is disabled")
> >>>>> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
> >>>> Please use Tariq's suggestion.
> >>> HW_TLS_TX feature is for both IPv4/v6. And If one device is limited to
> >>> support only IPv4 checksum offload, TLS offload should be allowed for
> >>> that too. So I think putting a check of CSUM_MASK is enough.
> >>
> >> The issue is that there is no good software fallback if the packet
> >> arrives at the NIC and it cannot handle the IPv6 TLS offload.
> >>
> >> The problem with the earlier patch you had is that it was just
> >> dropping frames if you couldn't handle the offload and "hoping" the
> >> other end would catch it. That isn't a good practice for any offload.
> >> If you have it enabled you have to have a software fallback and in
> >> this case it sounds like you don't have that.
> >>
> >> In order to do that you would have to alter the fast path to have to
> >> check if the device is capable per packet which is really an
> >> undesirable setup as it would add considerable overhead and is open to
> >> race conditions.
> >>
> >
> > +1
> >
> > Are there really such modern devices with missing IPv6 csum
> > capabilities, or it's just a missing SW implementation in the device
> > driver?
> >
> > IMO, it sounds reasonable for this modern TLS device offload to asks
> > for a basic requirement such as IPv6 csum offload capability, to save
> > the overhead.
> >
> I agree with you, all modern devices support V6 csum, but still if we think
> logically, we can't limit TLS offload to work only if both IPV4_CSUM  and
> IPV6_CSUM are configured/supported. If there is no dependency of IPV6
> in running TLS offload with IPv4  and vice-versa, then why should there
> be any restriction as such.

The requirement is to have a software fallback for any offload that
cannot be performed by the hardware if you are going to advertise it.
So if we were to disable IPv6 checksum offload and then request TLS
offload there isn't a software fallback for the combination since the
L4 header checksum cannot be performed on the packet if we are
expecting the hardware to handle the encryption. So in such a case the
TLS offload will need to occur before software can offload the header
checksum and so the hardware offload is disabled.

The basic idea is any offload combination should be functional so that
it doesn't mangle frames or cause the receiving end to drop packets.