netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
To: Ilya Lesokhin <ilyal@mellanox.com>
Cc: "netdev\@vger.kernel.org" <netdev@vger.kernel.org>,
	"davem\@davemloft.net" <davem@davemloft.net>,
	"davejwatson\@fb.com" <davejwatson@fb.com>,
	"tom\@herbertland.com" <tom@herbertland.com>,
	Boris Pismenny <borisp@mellanox.com>,
	Aviad Yehezkel <aviadye@mellanox.com>,
	Liran Liss <liranl@mellanox.com>
Subject: Re: [PATCH net-next 5/5] tls: Add generic NIC offload infrastructure.
Date: Tue, 19 Sep 2017 11:40:29 +0200	[thread overview]
Message-ID: <87wp4vt4aa.fsf@stressinduktion.org> (raw)
In-Reply-To: <AM4PR0501MB2723F6F5A22955102CDC2886D4600@AM4PR0501MB2723.eurprd05.prod.outlook.com> (Ilya Lesokhin's message of "Tue, 19 Sep 2017 07:36:28 +0000")

Hello,

Ilya Lesokhin <ilyal@mellanox.com> writes:

> Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
>
>> The user should be aware of that they can't migrate the socket to another
>> interface if they got hw offloaded. This is not the case for software offload.
>> Thus I think the user has to opt in and it shouldn't be a heuristic until we can
>> switch back to sw offload path.
>> 
>> Maybe change flowi_oif to sk_bound_dev_if and somwhow lock it against
>> further changes if hw tls is in use?
>> 
>
> I'm not sure I follow.
> We do set sk->sk_bound_dev_if to prevent further changes.
>
> Do you recommend we enable TLS offload only if SO_BINDTODEVICE	
> was previously used on that socket?
> and prevent even users with CAP_NET_RAW from unbinding it?
>
> I would rather avoid requiring CAP_NET_RAW to use TLS offload. 
> But admittedly I'm not sure setting sk->sk_bound_dev_if 
> without CAP_NET_RAW like we do is legit either.
>
> Finally, the reason we made HW offload the default is that the user
> can use sudo ethtool -K enp0s4 tls-hw-tx-offload off to opt out of HW offload
> and we currently don't have anything equivalent for opting out of SW KTLS.

IMHO the decision if a TCP flow should be bounded to hw and thus never
push traffic to another interface should a decision the administrator
and the application should opt in. You might have your management
application which is accessible over multiple interfaces and your
production application which might want to use hw offloaded tls. Thus I
don't think only a single ethtool knob will do it.

I agree that SO_BINDTODEVICE is bad for this use case. First, the
CAP_NET_RAW limitation seems annoying and we don't want to enforce TLS
apps to have this capability. Second, the user space application doesn't
care which interface it should talk to (maybe?) but leave the routing
decision to the kernel and just opt in to TLS. SO_BINDTODEVICE doesn't
allow this.

sk_bound_dev_if can be rebound later with CAP_NET_RAW privileges, will
this be a problem?

Have you thought how the user space will configure the various
offloading features (sw, hw, none)? Will it in e.g. OpenSSL be part of
the Cipher Spec or will there be new functions around SSL_CTX to do so?

Maybe an enhancement of the TLS_TX setsockopt with a boolean for hw
offload is a solution?

Another question:

How is the dependency management done between socket layer and driver
layer? It seems a bit cyclic but judging from this code you don't hold
references to the device (dev_hold) (which is good, you don't want to
have users creating refs to devices). OTOH you somehow need to match
sockets from the device layer up to the socket. Will those be reference
counted or does that work without?

Thanks,
Hannes

  reply	other threads:[~2017-09-19  9:40 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-14 10:46 [PATCH net-next 0/5] tls: Add generic NIC offload infrastructure Ilya Lesokhin
2017-09-14 10:46 ` [PATCH net-next 1/5] tls: Move release of tls_ctx into tls_sw_free_resources Ilya Lesokhin
2017-09-14 10:46 ` [PATCH net-next 2/5] tcp: Add clean acked data hook Ilya Lesokhin
2017-09-14 10:46 ` [PATCH net-next 3/5] net: Add TLS offload netdev ops Ilya Lesokhin
2017-09-14 10:46 ` [PATCH net-next 4/5] net: Add TLS TX offload features Ilya Lesokhin
2017-09-14 10:46 ` [PATCH net-next 5/5] tls: Add generic NIC offload infrastructure Ilya Lesokhin
2017-09-18 14:52   ` Hannes Frederic Sowa
2017-09-19  7:36     ` Ilya Lesokhin
2017-09-19  9:40       ` Hannes Frederic Sowa [this message]
2017-09-19 14:02         ` Boris Pismenny
2017-09-20 15:16           ` Hannes Frederic Sowa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87wp4vt4aa.fsf@stressinduktion.org \
    --to=hannes@stressinduktion.org \
    --cc=aviadye@mellanox.com \
    --cc=borisp@mellanox.com \
    --cc=davejwatson@fb.com \
    --cc=davem@davemloft.net \
    --cc=ilyal@mellanox.com \
    --cc=liranl@mellanox.com \
    --cc=netdev@vger.kernel.org \
    --cc=tom@herbertland.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).