netdev - Re: [PATCH net-next 0/2] TLS TX HW offload for Bond

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: <20201119083811.6b68bfa8@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Thu, 19 Nov 2020 08:38:11 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Tariq Toukan <ttoukan.linux@...il.com>,
        Jarod Wilson <jarod@...hat.com>
Cc:     Tariq Toukan <tariqt@...dia.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Saeed Mahameed <saeedm@...dia.com>,
        Moshe Shemesh <moshe@...dia.com>
Subject: Re: [PATCH net-next 0/2] TLS TX HW offload for Bond

On Thu, 19 Nov 2020 17:59:38 +0200 Tariq Toukan wrote:
> On 11/19/2020 2:02 AM, Jakub Kicinski wrote:
> > On Sun, 15 Nov 2020 15:42:49 +0200 Tariq Toukan wrote:  
> >> This series opens TLS TX HW offload for bond interfaces.
> >> This allows bond interfaces to benefit from capable slave devices.
> >>
> >> The first patch adds real_dev field in TLS context structure, and aligns
> >> usages in TLS module and supporting drivers.
> >> The second patch opens the offload for bond interfaces.
> >>
> >> For the configuration above, SW kTLS keeps picking the same slave
> >> To keep simple track of the HW and SW TLS contexts, we bind each socket to
> >> a specific slave for the socket's whole lifetime. This is logically valid
> >> (and similar to the SW kTLS behavior) in the following bond configuration,
> >> so we restrict the offload support to it:
> >>
> >> ((mode == balance-xor) or (mode == 802.3ad))
> >> and xmit_hash_policy == layer3+4.  
> > 
> > This does not feel extremely clean, maybe you can convince me otherwise.
> > 
> > Can we extend netdev_get_xmit_slave() and figure out the output dev
> > (and if it's "stable") in a more generic way? And just feed that dev
> > into TLS handling?   
> 
> I don't see we go through netdev_get_xmit_slave(), but through 
> .ndo_start_xmit (bond_start_xmit).

I may be misunderstanding the purpose of netdev_get_xmit_slave(),
please correct me if I'm wrong. AFAIU it's supposed to return a
lower netdev that the skb should then be xmited on.

So what I was thinking was either construct an skb or somehow reshuffle
the netdev_get_xmit_slave() code to take a flow dissector output or
${insert other ideas}. Then add a helper in the core that would drill
down from the socket netdev to the "egress" netdev. Have TLS call
that helper, and talk to the "egress" netdev from the start, rather
than the socket's netdev. Then loosen the checks on software devices.

I'm probably missing the problem you're trying to explain to me :S

Side note - Jarod, I'd be happy to take a patch renaming
netdev_get_xmit_slave() and the ndo, if you have the cycles to send 
a patch. It's a recent addition, and in the core we should make more 
of an effort to avoid sensitive terms.

> Currently I have my check there to 
> catch all skbs belonging to offloaded TLS sockets.
> 
> The TLS offload get_slave() logic decision is per socket, so the result 
> cannot be saved in the bond memory. Currently I save the real_dev field 
> in the TLS context structure.

Right, but we could just have ctx->netdev be the "egress" netdev
always, right? Do we expect somewhere that it's going to be matching
the socket's dst?

> One way to make it more generic is to save it on the sock structure. I 
> agree that this replaces the TLS-specific logic, but demands increasing 
> the sock struct, and has larger impact on all other flows...