[PATCHv8,0/6] net/tls: fixes for NVMe-over-TLS

Message

Hannes Reinecke July 21, 2023, 2:35 p.m. UTC

Hi all,

here are some small fixes to get NVMe-over-TLS up and running.
The first set are just minor modifications to have MSG_EOR handled
for TLS, but the second set implements the ->read_sock() callback for tls_sw
which I guess could do with some reviews.

As usual, comments and reviews are welcome.

Changes to the original submission:
- Add a testcase for MSG_EOR handling

Changes to v2:
- Bail out on conflicting message flags
- Rework flag handling

Changes to v3:
- Return -EINVAL on conflicting flags
- Rebase on top of net-next

Changes to v4:
- Add tlx_rx_reader_lock() to read_sock
- Add MSG_EOR handling to tls_sw_readpages()

Changes to v5:
- Rebase to latest upstream
- Split tls_rx_reader_lock() as suggested by Sagi

Changes to v6:
- Fixup tls_strp_read_copyin() to avoid infinite recursion
  in tls_read_sock()
- Rework tls_read_sock() to read all available data

Changes to v7:
- Include reviews from Jakub

Hannes Reinecke (6):
  net/tls: handle MSG_EOR for tls_sw TX flow
  net/tls: handle MSG_EOR for tls_device TX flow
  selftests/net/tls: add test for MSG_EOR
  net/tls: Use tcp_read_sock() instead of ops->read_sock()
  net/tls: split tls_rx_reader_lock
  net/tls: implement ->read_sock()

 net/tls/tls.h                     |   2 +
 net/tls/tls_device.c              |   6 +-
 net/tls/tls_main.c                |   2 +
 net/tls/tls_strp.c                |   3 +-
 net/tls/tls_sw.c                  | 132 ++++++++++++++++++++++++++----
 tools/testing/selftests/net/tls.c |  11 +++
 6 files changed, 136 insertions(+), 20 deletions(-)

Comments

Jakub Kicinski July 22, 2023, 2 a.m. UTC | #1

On Fri, 21 Jul 2023 16:35:17 +0200 Hannes Reinecke wrote:
> here are some small fixes to get NVMe-over-TLS up and running.
> The first set are just minor modifications to have MSG_EOR handled
> for TLS, but the second set implements the ->read_sock() callback for tls_sw
> which I guess could do with some reviews.

Reviewed-by: Jakub Kicinski <[e-mail]>

Sagi, I _think_ a stable branch with this should be doable,
would you like one, or no rush?

Hannes Reinecke July 24, 2023, 7:10 a.m. UTC | #2

On 7/22/23 04:00, Jakub Kicinski wrote:
> On Fri, 21 Jul 2023 16:35:17 +0200 Hannes Reinecke wrote:
>> here are some small fixes to get NVMe-over-TLS up and running.
>> The first set are just minor modifications to have MSG_EOR handled
>> for TLS, but the second set implements the ->read_sock() callback for tls_sw
>> which I guess could do with some reviews.
> 
> Reviewed-by: Jakub Kicinski <[e-mail]>
> 
> Sagi, I _think_ a stable branch with this should be doable,
> would you like one, or no rush?

I guess a stable branch would not be too bad; I've got another
set of patches for the NVMe side, too.
Sagi?

Cheers,

Hannes

Sagi Grimberg July 24, 2023, 7:21 a.m. UTC | #3

>>> here are some small fixes to get NVMe-over-TLS up and running.
>>> The first set are just minor modifications to have MSG_EOR handled
>>> for TLS, but the second set implements the ->read_sock() callback for 
>>> tls_sw
>>> which I guess could do with some reviews.
>>
>> Reviewed-by: Jakub Kicinski <[e-mail]>
>>
>> Sagi, I _think_ a stable branch with this should be doable,
>> would you like one, or no rush?
> 
> I guess a stable branch would not be too bad; I've got another
> set of patches for the NVMe side, too.
> Sagi?

I don't think there is a real need for this to go to stable, nothing
is using it. Perhaps the MSG_EOR patches can go to stable in case
there is some userspace code that wants to rely on it.

Jakub Kicinski July 24, 2023, 7:35 p.m. UTC | #4

On Mon, 24 Jul 2023 10:21:52 +0300 Sagi Grimberg wrote:
> >> Reviewed-by: Jakub Kicinski <[e-mail]>
> >>
> >> Sagi, I _think_ a stable branch with this should be doable,
> >> would you like one, or no rush?  
> > 
> > I guess a stable branch would not be too bad; I've got another
> > set of patches for the NVMe side, too.
> > Sagi?  
> 
> I don't think there is a real need for this to go to stable, nothing
> is using it. Perhaps the MSG_EOR patches can go to stable in case
> there is some userspace code that wants to rely on it.

I'm probably using the wrong word. I mean a branch based on -rc3 that's
not going to get rebased so the commits IDs match and we can both pull
it in. Not stable as in Greg KH.

Sagi Grimberg July 24, 2023, 7:44 p.m. UTC | #5

>>>> Reviewed-by: Jakub Kicinski <[e-mail]>
>>>>
>>>> Sagi, I _think_ a stable branch with this should be doable,
>>>> would you like one, or no rush?
>>>
>>> I guess a stable branch would not be too bad; I've got another
>>> set of patches for the NVMe side, too.
>>> Sagi?
>>
>> I don't think there is a real need for this to go to stable, nothing
>> is using it. Perhaps the MSG_EOR patches can go to stable in case
>> there is some userspace code that wants to rely on it.
> 
> I'm probably using the wrong word. I mean a branch based on -rc3 that's
> not going to get rebased so the commits IDs match and we can both pull
> it in. Not stable as in Greg KH.

Are you aiming this for 6.5 ? We are unlikely to get the nvme bits in
this round. I also don't think there is a conflict so the nvme bits
can go in for 6.6 and later the nvme tree will pull the tls updates.

Jakub Kicinski July 24, 2023, 7:52 p.m. UTC | #6

On Mon, 24 Jul 2023 22:44:49 +0300 Sagi Grimberg wrote:
> > I'm probably using the wrong word. I mean a branch based on -rc3 that's
> > not going to get rebased so the commits IDs match and we can both pull
> > it in. Not stable as in Greg KH.  
> 
> Are you aiming this for 6.5 ? We are unlikely to get the nvme bits in
> this round. I also don't think there is a conflict so the nvme bits
> can go in for 6.6 and later the nvme tree will pull the tls updates.

Great, less work :) 

Let's see a v9 with the flushing improved and we'll apply to net-next
directly.

Hannes Reinecke July 25, 2023, 6:55 a.m. UTC | #7

On 7/24/23 21:44, Sagi Grimberg wrote:
> 
>>>>> Reviewed-by: Jakub Kicinski <[e-mail]>
>>>>>
>>>>> Sagi, I _think_ a stable branch with this should be doable,
>>>>> would you like one, or no rush?
>>>>
>>>> I guess a stable branch would not be too bad; I've got another
>>>> set of patches for the NVMe side, too.
>>>> Sagi?
>>>
>>> I don't think there is a real need for this to go to stable, nothing
>>> is using it. Perhaps the MSG_EOR patches can go to stable in case
>>> there is some userspace code that wants to rely on it.
>>
>> I'm probably using the wrong word. I mean a branch based on -rc3 that's
>> not going to get rebased so the commits IDs match and we can both pull
>> it in. Not stable as in Greg KH.
> 
> Are you aiming this for 6.5 ? We are unlikely to get the nvme bits in
> this round. I also don't think there is a conflict so the nvme bits
> can go in for 6.6 and later the nvme tree will pull the tls updates.

I really would love to get this into 6.5, as then we can do a clean 
rebase of the NVMe development tree off 6.5, knowing that we won't have 
to touch the networking. Otherwise you have to be sure to fork the 6.6 
development branch off the right point.

But then I'm not doing the forking, so really all I care is that the 
networking bits are present in the 6.6 NVMe development branch ...

Cheers,

Hannes