netdev - Re: [PATCH net] net: Disable NETIF_F_HW_TLS_TX when HW

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: <20201105080019.24bd43fe@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 5 Nov 2020 08:00:19 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Tariq Toukan <ttoukan.linux@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Saeed Mahameed <saeedm@...dia.com>,
        Moshe Shemesh <moshe@...dia.com>,
        Boris Pismenny <borisp@...dia.com>
Subject: Re: [PATCH net] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is
 disabled

On Thu, 5 Nov 2020 15:22:53 +0200 Tariq Toukan wrote:
> On 11/4/2020 11:25 PM, Jakub Kicinski wrote:
> > On Wed,  4 Nov 2020 12:21:41 +0200 Tariq Toukan wrote:  
> >> With NETIF_F_HW_TLS_TX packets are encrypted in HW. This cannot be
> >> logically done when HW_CSUM offload is off.  
> > 
> > Right. Do you expect drivers to nack clearing NETIF_F_HW_TLS_TX when
> > there are active connections, then?  I don't think NFP does.  We either
> > gotta return -EBUSY when there are offloaded connections, or at least
> > clearly document the expected behavior.
> 
> As I see from code, today drivers and TLS stack allow clearing 
> NETIF_F_HW_TLS_TX without doing anything to change behavior in existing 
> sockets, so they continue to do HW offload. Only new sockets will be 
> affected.

Right, we want to let users turn off the offload when it misbehaves,
and we don't have a way of un-offloading connections.

> I think the same behavior should apply when NETIF_F_HW_TLS_TX is cleared 
> implicitly (due to clearing HW_CSUM).

Right, I don't mind either way. My thinking with the tls feature was
that offload is likely to be broken, at least initially. Checksum
offload should work, one would hope, so its less of a risk.

The question is perhaps - do we care more about consistency with the
behavior of the TLS feature, or current expectation that csum offload
off will actually turn it off.

> If the existing behavior is not expected, and we should force fallback 
> to SW kTLS for existing sockets, then I think this should be fixed 
> independently to this patch, as it introduces no new regression.
> 
> What do you think?

The current behavior of the TLS features is documented in
tls-offload.rst, you can do what you're doing in this patch, 
but you need to document it there.