Message-ID: <683554de-5f4f-4adb-4e97-c532f514b352@grimberg.me>
Date: Tue, 12 Sep 2023 14:34:25 +0300
From: Sagi Grimberg <sagi@...mberg.me>
To: Hannes Reinecke <hare@...e.de>, Christoph Hellwig <hch@....de>
Cc: Keith Busch <kbusch@...nel.org>, linux-nvme@...ts.infradead.org,
 Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCHv11 00/18] nvme: In-kernel TLS support for TCP


> Hi all,

Hannes, I think that this is in decent shape. Assuming that
the recent reports on the tls tests are resolved, I think this
is ready for inclusion.

I also want to give it some time on the nvme tree.

> 
> with the merge of Chuck Levers handshake upcall mechanism and
> my tls_read_sock() implementation finally merged with net-next
> it's now time to restart on the actual issue, namely implementing
> in-kernel TLS support for nvme-tcp.
> 
> The patchset is based on the recent net-next git tree;
> everything after commit ba4a734e1aa0 ("net/tls: avoid TCP window
> full during ->read_sock()") should work.
> You might want to add the patch
> 'nvmet-tcp: use 'spin_lock_bh' for state_lock()'
> before applying this patchset; otherwise results might be ...
> interesting.
> 
> It also requires the 'tlshd' userspace daemon
> (https://github.com/oracle/ktls-utils)
> for the actual TLS handshake.
> Changes for nvme-cli are already included in the upstream repository.
> 
> Theory of operation:
> A dedicated '.nvme' keyring is created to hold the pre-shared keys (PSKs)
> for the TLS handshake. Keys will have to be provisioned before TLS handshake
> is attempted; that can be done with the 'nvme gen-tls-key' command for nvme-cli
> (patches are already merged upstream).
> After connection to the remote TCP port the client side will use the
> 'best' PSK (as inferred from the NVMe TCP spec) or the PSK specified
> by the '--tls_key' option to nvme-cli and call the TLS userspace daemon
> to initiate a TLS handshake.
> The server side will then invoke the TLS userspace daemon to run the TLS
> handshake.
> If the TLS handshake succeeds the userspace daemon will be activating
> kTLS on the socket, and control is passed back to the kernel.
> 
> This implementation currently does not implement the so-called
> 'secure concatenation' mode from NVMe-TCP; a TPAR is still pending
> with fixes for it, so I'll wait until it's published before
> posting patches for that.
> 
> Patchset can be found at:
> git.kernel.org/pub/scm/linux/kernel/git/hare/nvme.git
> branch tls.v16
> 
> For testing I'm using this script, running on a nvme target
> with NQN 'nqn.test' and using 127.0.0.1 as a port; the port
> has to set 'addr_tsas' to 'tls1.3':
> 
> modprobe nvmet-tcp
> nvmetcli restore
> modprobe nvme-tcp
> ./nvme gen-tls-key --subsysnqn=nqn.test -i
> ./nvme gen-tls-key --subsysnqn=nqn.2014-08.org.nvmexpress.discovery -i
> tlshd -c /etc/tlshd.conf
> 
> and then one can do a simple:
> # nvme connect -t tcp -a 127.0.0.1 -s 4420 -n nqn.test --tls
> to start the connection.
> 
> As usual, comments and reviews are welcome.
> 
> Changes to v10:
> - Include reviews from Sagi
> 
> Changes to v9:
> - Close race between done() and timeout()
> - Add logging message for icreq/icresp failure
> - Sparse fixes
> - Restrict TREQ setting to 'not required' or 'required'
>    when TLS is enabled
> 
> Changes to v8:
> - Simplify reference counting as suggested by Sagi
> - Implement nvmf_parse_key() to simplify options parsing
> - Add patch to peek at icreq to figure out whether TLS
>    should be started
> 
> Changes to v7:
> - Include reviews from Simon
> - Do not call sock_release() when sock_alloc_file() fails
> 
> Changes to v6:
> - More reviews from Sagi
> - Fixup non-tls connections
> - Fixup nvme options handling
> - Add reference counting to nvmet-tcp queues
> 
> Changes to v5:
> - Include reviews from Sagi
> - Split off nvmet tsas/treq handling
> - Sanitize sock_file handling
> 
> Changes to v4:
> - Split off network patches into a separate
>    patchset
>      - Handle TLS Alert notifications
> 
> Changes to v3:
> - Really handle MSG_EOR for TLS
> - Fixup MSG_SENDPAGE_NOTLAST handling
> - Conditionally disable fabric option
> 
> Changes to v2:
> - Included reviews from Sagi
> - Removed MSG_SENDPAGE_NOTLAST
> - Improved MSG_EOR handling for TLS
> - Add config options NVME_TCP_TLS
>    and NVME_TARGET_TCP_TLS
> 
> Changes to the original RFC:
> - Add a CONFIG_NVME_TLS config option
> - Use a single PSK for the TLS handshake
> - Make TLS connections mandatory
> - Do not peek messages for the server
> - Simplify data_ready callback
> - Implement read_sock() for TLS
> 
> Hannes Reinecke (18):
>    nvme-keyring: register '.nvme' keyring
>    nvme-keyring: define a 'psk' keytype
>    nvme: add TCP TSAS definitions
>    nvme-tcp: add definitions for TLS cipher suites
>    nvme-keyring: implement nvme_tls_psk_default()
>    security/keys: export key_lookup()
>    nvme-tcp: allocate socket file
>    nvme-tcp: enable TLS handshake upcall
>    nvme-tcp: control message handling for recvmsg()
>    nvme-tcp: improve icreq/icresp logging
>    nvme-fabrics: parse options 'keyring' and 'tls_key'
>    nvmet: make TCP sectype settable via configfs
>    nvmet-tcp: make nvmet_tcp_alloc_queue() a void function
>    nvmet-tcp: allocate socket file
>    nvmet: Set 'TREQ' to 'required' when TLS is enabled
>    nvmet-tcp: enable TLS handshake upcall
>    nvmet-tcp: control messages for recvmsg()
>    nvmet-tcp: peek icreq before starting TLS
> 
>   drivers/nvme/common/Kconfig    |   4 +
>   drivers/nvme/common/Makefile   |   3 +-
>   drivers/nvme/common/keyring.c  | 182 ++++++++++++++++++
>   drivers/nvme/host/Kconfig      |  15 ++
>   drivers/nvme/host/core.c       |  12 +-
>   drivers/nvme/host/fabrics.c    |  67 ++++++-
>   drivers/nvme/host/fabrics.h    |   9 +
>   drivers/nvme/host/nvme.h       |   1 +
>   drivers/nvme/host/sysfs.c      |  21 +++
>   drivers/nvme/host/tcp.c        | 184 ++++++++++++++++--
>   drivers/nvme/target/Kconfig    |  15 ++
>   drivers/nvme/target/configfs.c | 128 ++++++++++++-
>   drivers/nvme/target/nvmet.h    |  11 ++
>   drivers/nvme/target/tcp.c      | 334 ++++++++++++++++++++++++++++++---
>   include/linux/key.h            |   1 +
>   include/linux/nvme-keyring.h   |  36 ++++
>   include/linux/nvme-tcp.h       |   6 +
>   include/linux/nvme.h           |  10 +
>   security/keys/key.c            |   1 +
>   19 files changed, 991 insertions(+), 49 deletions(-)
>   create mode 100644 drivers/nvme/common/keyring.c
>   create mode 100644 include/linux/nvme-keyring.h
> 

Powered by blists - more mailing lists