| Message ID | 20210106175327.5606-1-rohitm@chelsio.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: feature check mandating HW_CSUM is wrong | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 10 maintainers not CCed: ast@kernel.org bjorn.topel@intel.com daniel@iogearbox.net andriin@fb.com tariqt@nvidia.com edumazet@google.com xiyou.wangcong@gmail.com ap420073@gmail.com jiri@mellanox.com borisp@nvidia.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 10 this patch: 10 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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.
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.
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.
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.
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.
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.
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.
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; }
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(-)