Re: [PATCH 07/17] net/tls: handle MSG_EOR for tls_sw TX flow

Re: [PATCH 07/17] net/tls: handle MSG_EOR for tls_sw TX flow

[Hannes Reinecke]

On 5/9/23 11:19, Max Gurtovoy wrote:
> 
> 
> On 19/04/2023 9:57, Hannes Reinecke wrote:
>> tls_sw_sendmsg() / tls_do_sw_sendpage() already checks for an
>> 'end of record' by evaluating the 'MSG_MORE' / 'MSG_SENDPAGE_NOTLAST'
>> setting. So MSG_EOR can easily be handled by treating it
>> as the opposite of MSG_MORE / MSG_SENDPAGE_NOTLAST.
>>
> 
> This seems like a nice optimization but seems not mandatory for the 
> acceptance of TLS support in nvme/tcp.
> 
> I wonder if this can go to net/tls as a standalone patch ?
> 
Errm. Without this NVMe/TLS will not work as sendmsg/sendpage will
bail out.
So yes, surely it can be applied as a standalone patch, but that
only makes sense if it will be applied _before_ the rest of the
nvme/tls patches.

Not sure how best to coordinate this.

> 
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> ---
>>   net/tls/tls_sw.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
>> index 827292e29f99..9bee2dcd55bf 100644
>> --- a/net/tls/tls_sw.c
>> +++ b/net/tls/tls_sw.c
>> @@ -953,9 +953,12 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr 
>> *msg, size_t size)
>>       int pending;
>>       if (msg->msg_flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
>> -                   MSG_CMSG_COMPAT))
>> +                   MSG_EOR | MSG_CMSG_COMPAT))
>>           return -EOPNOTSUPP;
>> +    if (msg->msg_flags & MSG_EOR)
>> +        eor = true;
>> +
>>       ret = mutex_lock_interruptible(&tls_ctx->tx_lock);
>>       if (ret)
>>           return ret;
>> @@ -1173,6 +1176,8 @@ static int tls_sw_do_sendpage(struct sock *sk, 
>> struct page *page,
>>       bool eor;
>>       eor = !(flags & MSG_SENDPAGE_NOTLAST);
>> +    if (flags & MSG_EOR)
>> +        eor = true;
>>       sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
>>       /* Call the sk_stream functions to manage the sndbuf mem. */
>> @@ -1274,7 +1279,7 @@ static int tls_sw_do_sendpage(struct sock *sk, 
>> struct page *page,
>>   int tls_sw_sendpage_locked(struct sock *sk, struct page *page,
>>                  int offset, size_t size, int flags)
>>   {
>> -    if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
>> +    if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_EOR |
>>                 MSG_SENDPAGE_NOTLAST | MSG_SENDPAGE_NOPOLICY |
>>                 MSG_NO_SHARED_FRAGS))
>>           return -EOPNOTSUPP;
>> @@ -1288,7 +1293,7 @@ int tls_sw_sendpage(struct sock *sk, struct page 
>> *page,
>>       struct tls_context *tls_ctx = tls_get_ctx(sk);
>>       int ret;
>> -    if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
>> +    if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_EOR |
>>                 MSG_SENDPAGE_NOTLAST | MSG_SENDPAGE_NOPOLICY))
>>           return -EOPNOTSUPP;

-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH 07/17] net/tls: handle MSG_EOR for tls_sw TX flow

[Jakub Kicinski]

On Tue, 9 May 2023 16:18:30 +0200 Hannes Reinecke wrote:
> > This seems like a nice optimization but seems not mandatory for the 
> > acceptance of TLS support in nvme/tcp.
> > 
> > I wonder if this can go to net/tls as a standalone patch ?
>
> Errm. Without this NVMe/TLS will not work as sendmsg/sendpage will
> bail out.
> So yes, surely it can be applied as a standalone patch, but that
> only makes sense if it will be applied _before_ the rest of the
> nvme/tls patches.
> 
> Not sure how best to coordinate this.

You should apply it on a branch based on -rc1 and then both us and
$appropriate-nvme-maintainer can pull it in.

Re: [PATCH 07/17] net/tls: handle MSG_EOR for tls_sw TX flow

[Max Gurtovoy]

On 09/05/2023 18:13, Jakub Kicinski wrote:
> On Tue, 9 May 2023 16:18:30 +0200 Hannes Reinecke wrote:
>>> This seems like a nice optimization but seems not mandatory for the
>>> acceptance of TLS support in nvme/tcp.
>>>
>>> I wonder if this can go to net/tls as a standalone patch ?
>>
>> Errm. Without this NVMe/TLS will not work as sendmsg/sendpage will
>> bail out.
>> So yes, surely it can be applied as a standalone patch, but that
>> only makes sense if it will be applied _before_ the rest of the
>> nvme/tls patches.

how come it will bail out only for NVMe/TLS and not for Other_user/TLS ?

>>
>> Not sure how best to coordinate this.
> 
> You should apply it on a branch based on -rc1 and then both us and
> $appropriate-nvme-maintainer can pull it in.

Re: [PATCH 07/17] net/tls: handle MSG_EOR for tls_sw TX flow

[Hannes Reinecke]

On 5/10/23 01:02, Max Gurtovoy wrote:
> 
> 
> On 09/05/2023 18:13, Jakub Kicinski wrote:
>> On Tue, 9 May 2023 16:18:30 +0200 Hannes Reinecke wrote:
>>>> This seems like a nice optimization but seems not mandatory for the
>>>> acceptance of TLS support in nvme/tcp.
>>>>
>>>> I wonder if this can go to net/tls as a standalone patch ?
>>>
>>> Errm. Without this NVMe/TLS will not work as sendmsg/sendpage will
>>> bail out.
>>> So yes, surely it can be applied as a standalone patch, but that
>>> only makes sense if it will be applied _before_ the rest of the
>>> nvme/tls patches.
> 
> how come it will bail out only for NVMe/TLS and not for Other_user/TLS ?
> 
Oh, it will bail for other TLS users as well.
Point is it will not bail for _non_ TLS connections, causing the issue.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman