Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

[Sagi Grimberg]

> kTLS does not support MSG_EOR flag for sendmsg(), and in general
> is really picky about invalid MSG_XXX flags.

CC'ing TLS folks.

Can't tls simply ignore MSG_EOR instead of consumers having to be 
careful over it?

> So ensure that the MSG_EOR flags is blanked out for TLS, and that
> the MSG_SENDPAGE_LAST is only set if we actually do sendpage().

You mean MSG_SENDPAGE_NOTLAST.

It is also a bit annoying that a tls socket dictates different behavior
than a normal socket.

The current logic is rather simple:
if more data comming:
	flags = MSG_MORE | MSG_SENDPAGE_NOTLAST
else:
	flags = MSG_EOR

Would like to keep it that way for tls as well. Can someone
explain why this is a problem with tls?

Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

[Hannes Reinecke]

On 3/30/23 17:24, Sagi Grimberg wrote:
> 
>> kTLS does not support MSG_EOR flag for sendmsg(), and in general
>> is really picky about invalid MSG_XXX flags.
> 
> CC'ing TLS folks.
> 
> Can't tls simply ignore MSG_EOR instead of consumers having to be 
> careful over it?
> 
>> So ensure that the MSG_EOR flags is blanked out for TLS, and that
>> the MSG_SENDPAGE_LAST is only set if we actually do sendpage().
> 
> You mean MSG_SENDPAGE_NOTLAST.
> 
> It is also a bit annoying that a tls socket dictates different behavior
> than a normal socket.
> 
> The current logic is rather simple:
> if more data comming:
>      flags = MSG_MORE | MSG_SENDPAGE_NOTLAST
> else:
>      flags = MSG_EOR
> 
> Would like to keep it that way for tls as well. Can someone
> explain why this is a problem with tls?

TLS is using MSG_EOR internally (to control the flow of the underlying 
tcp stream), so it's meaningless for the ULP.
I've no idea about SENDPAGE_NOTLAST; that one is particularly annoying
as we have to check whether we do sendpage() or sendmsg().

But TLS really should blank out invalid flags, as it has different rules 
for them than anyone else. And the caller really shouldn't be burdened
with checking whether TLS is enabled or not.

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

Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

[Jakub Kicinski]

On Thu, 30 Mar 2023 18:24:04 +0300 Sagi Grimberg wrote:
> > kTLS does not support MSG_EOR flag for sendmsg(), and in general
> > is really picky about invalid MSG_XXX flags.  
> 
> CC'ing TLS folks.
>
> Can't tls simply ignore MSG_EOR instead of consumers having to be 
> careful over it?

I think we can support EOR, I don't see any fundamental problem there.

> > So ensure that the MSG_EOR flags is blanked out for TLS, and that
> > the MSG_SENDPAGE_LAST is only set if we actually do sendpage().  
> 
> You mean MSG_SENDPAGE_NOTLAST.
> 
> It is also a bit annoying that a tls socket dictates different behavior
> than a normal socket.
> 
> The current logic is rather simple:
> if more data comming:
> 	flags = MSG_MORE | MSG_SENDPAGE_NOTLAST
> else:
> 	flags = MSG_EOR
> 
> Would like to keep it that way for tls as well. Can someone
> explain why this is a problem with tls?

Some of the flags are call specific, others may be internal to the
networking stack (e.g. the DECRYPTED flag). Old protocols didn't do 
any validation because people coded more haphazardly in the 90s.
This lack of validation is a major source of technical debt :(

Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

[Hannes Reinecke]

On 3/31/23 07:49, Jakub Kicinski wrote:
> On Thu, 30 Mar 2023 18:24:04 +0300 Sagi Grimberg wrote:
>>> kTLS does not support MSG_EOR flag for sendmsg(), and in general
>>> is really picky about invalid MSG_XXX flags.
>>
>> CC'ing TLS folks.
>>
>> Can't tls simply ignore MSG_EOR instead of consumers having to be
>> careful over it?
> 
> I think we can support EOR, I don't see any fundamental problem there.
> 
>>> So ensure that the MSG_EOR flags is blanked out for TLS, and that
>>> the MSG_SENDPAGE_LAST is only set if we actually do sendpage().
>>
>> You mean MSG_SENDPAGE_NOTLAST.
>>
>> It is also a bit annoying that a tls socket dictates different behavior
>> than a normal socket.
>>
>> The current logic is rather simple:
>> if more data comming:
>> 	flags = MSG_MORE | MSG_SENDPAGE_NOTLAST
>> else:
>> 	flags = MSG_EOR
>>
>> Would like to keep it that way for tls as well. Can someone
>> explain why this is a problem with tls?
> 
> Some of the flags are call specific, others may be internal to the
> networking stack (e.g. the DECRYPTED flag). Old protocols didn't do
> any validation because people coded more haphazardly in the 90s.
> This lack of validation is a major source of technical debt :(

A-ha. So what is the plan?
Should the stack validate flags?
And should the rules for validating be the same for all protocols?

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

Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

[Sagi Grimberg]

Hey Jakub, Hannes.

>>>> kTLS does not support MSG_EOR flag for sendmsg(), and in general
>>>> is really picky about invalid MSG_XXX flags.
>>>
>>> CC'ing TLS folks.
>>>
>>> Can't tls simply ignore MSG_EOR instead of consumers having to be
>>> careful over it?
>>
>> I think we can support EOR, I don't see any fundamental problem there.

It would help at least one consumer (nvme-tcp) to not change its
behavior between tls and non-tls. At the very minimum don't fail
the send operation (just do the same as if it wasn't passed).

>>>> So ensure that the MSG_EOR flags is blanked out for TLS, and that
>>>> the MSG_SENDPAGE_LAST is only set if we actually do sendpage().
>>>
>>> You mean MSG_SENDPAGE_NOTLAST.
>>>
>>> It is also a bit annoying that a tls socket dictates different behavior
>>> than a normal socket.
>>>
>>> The current logic is rather simple:
>>> if more data comming:
>>>     flags = MSG_MORE | MSG_SENDPAGE_NOTLAST
>>> else:
>>>     flags = MSG_EOR
>>>
>>> Would like to keep it that way for tls as well. Can someone
>>> explain why this is a problem with tls?
>>
>> Some of the flags are call specific, others may be internal to the
>> networking stack (e.g. the DECRYPTED flag). Old protocols didn't do
>> any validation because people coded more haphazardly in the 90s.
>> This lack of validation is a major source of technical debt :(
> 
> A-ha. So what is the plan?
> Should the stack validate flags?
> And should the rules for validating be the same for all protocols?

MSG_SENDPAGE_NOTLAST is not an internal flag, I thought it was
essentially similar semantics to MSG_MORE but for sendpage. It'd
be great if this can be allowed in tls (again, at the very least
don't fail but continue as if it wasn't passed).

If this turns out to be a big project, I would prefer to change
nvme-tcp for now in order not to block nvme tls support (although it is
a hidden capability interface, which is always bad).

Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

[Jakub Kicinski]

On Mon, 3 Apr 2023 15:20:13 +0300 Sagi Grimberg wrote:
> >> Some of the flags are call specific, others may be internal to the
> >> networking stack (e.g. the DECRYPTED flag). Old protocols didn't do
> >> any validation because people coded more haphazardly in the 90s.
> >> This lack of validation is a major source of technical debt :(  
> > 
> > A-ha. So what is the plan?
> > Should the stack validate flags?
> > And should the rules for validating be the same for all protocols?  
> 
> MSG_SENDPAGE_NOTLAST is not an internal flag, I thought it was
> essentially similar semantics to MSG_MORE but for sendpage. It'd
> be great if this can be allowed in tls (again, at the very least
> don't fail but continue as if it wasn't passed).

.. but.. MSG_SENDPAGE_NOTLAST is supported in TLS, isn't it?
Why are we talking about it?

Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

[Sagi Grimberg]

>>>> Some of the flags are call specific, others may be internal to the
>>>> networking stack (e.g. the DECRYPTED flag). Old protocols didn't do
>>>> any validation because people coded more haphazardly in the 90s.
>>>> This lack of validation is a major source of technical debt :(
>>>
>>> A-ha. So what is the plan?
>>> Should the stack validate flags?
>>> And should the rules for validating be the same for all protocols?
>>
>> MSG_SENDPAGE_NOTLAST is not an internal flag, I thought it was
>> essentially similar semantics to MSG_MORE but for sendpage. It'd
>> be great if this can be allowed in tls (again, at the very least
>> don't fail but continue as if it wasn't passed).
> 
> .. but.. MSG_SENDPAGE_NOTLAST is supported in TLS, isn't it?
> Why are we talking about it?

Ah, right.

What I'm assuming that Hannes is tripping on is that tls does
not accept when this flag is sent to sock_no_sendpage, which
is simply calling sendmsg. TLS will not accept this flag when
passed to sendmsg IIUC.

Today the rough logic in nvme send path is:

	if (more_coming(queue)) {
		flags = MSG_MORE | MSG_SENDPAGE_NOTLAST;
	} else {
		flags = MSG_EOR;
	}

	if (!sendpage_ok(page)) {
		kernel_sendpage();
	} else {
		sock_no_sendpage();
	}

This pattern (note that sock_no_sednpage was added later following bug
reports where nvme attempted to sendpage a slab allocated page), is
perfectly acceptable with normal sockets, but not with TLS.

So there are two options:
1. have tls accept MSG_SENDPAGE_NOTLAST in sendmsg (called from
    sock_no_sendpage)
2. Make nvme set MSG_SENDPAGE_NOTLAST only when calling
    kernel_sendpage and clear it when calling sock_no_sendpage

If you say that MSG_SENDPAGE_NOTLAST must be cleared when calling
sock_no_sendpage and it is a bug that it isn't enforced for normal tcp
sockets, then we need to change nvme, but I did not find
any documentation that indicates it, and right now, normal sockets
behave differently than tls sockets (wrt this flag in particular).

Hope this clarifies.

Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

[Jakub Kicinski]

On Mon, 3 Apr 2023 18:51:09 +0300 Sagi Grimberg wrote:
> What I'm assuming that Hannes is tripping on is that tls does
> not accept when this flag is sent to sock_no_sendpage, which
> is simply calling sendmsg. TLS will not accept this flag when
> passed to sendmsg IIUC.
> 
> Today the rough logic in nvme send path is:
> 
> 	if (more_coming(queue)) {
> 		flags = MSG_MORE | MSG_SENDPAGE_NOTLAST;
> 	} else {
> 		flags = MSG_EOR;
> 	}
> 
> 	if (!sendpage_ok(page)) {
> 		kernel_sendpage();
> 	} else {
> 		sock_no_sendpage();
> 	}
> 
> This pattern (note that sock_no_sednpage was added later following bug
> reports where nvme attempted to sendpage a slab allocated page), is
> perfectly acceptable with normal sockets, but not with TLS.
> 
> So there are two options:
> 1. have tls accept MSG_SENDPAGE_NOTLAST in sendmsg (called from
>     sock_no_sendpage)
> 2. Make nvme set MSG_SENDPAGE_NOTLAST only when calling
>     kernel_sendpage and clear it when calling sock_no_sendpage
> 
> If you say that MSG_SENDPAGE_NOTLAST must be cleared when calling
> sock_no_sendpage and it is a bug that it isn't enforced for normal tcp
> sockets, then we need to change nvme, but I did not find
> any documentation that indicates it, and right now, normal sockets
> behave differently than tls sockets (wrt this flag in particular).
> 
> Hope this clarifies.

Oh right, it does, the context evaporated from my head over the weekend.

IMHO it's best if the caller passes the right flags. The semantics of
MSG_MORE vs NOTLAST are quite murky and had already caused bugs in the
past :(

See commit d452d48b9f8b ("tls: prevent oversized sendfile() hangs by
ignoring MSG_MORE")

Alternatively we could have sock_no_sendpage drop NOTLAST to help
all protos. But if we consider sendfile behavior as the standard
simply clearing it isn't right, it should be a:

	more = (flags & (MORE | NOTLAST)) == MORE | NOTLAST
	flags &= ~(MORE | NOTLAST)
	if (more)
		flags |= MORE

Re: [PATCH 10/18] nvme-tcp: fixup send workflow for kTLS

[Sagi Grimberg]

On 4/3/23 21:48, Jakub Kicinski wrote:
> On Mon, 3 Apr 2023 18:51:09 +0300 Sagi Grimberg wrote:
>> What I'm assuming that Hannes is tripping on is that tls does
>> not accept when this flag is sent to sock_no_sendpage, which
>> is simply calling sendmsg. TLS will not accept this flag when
>> passed to sendmsg IIUC.
>>
>> Today the rough logic in nvme send path is:
>>
>> 	if (more_coming(queue)) {
>> 		flags = MSG_MORE | MSG_SENDPAGE_NOTLAST;
>> 	} else {
>> 		flags = MSG_EOR;
>> 	}
>>
>> 	if (!sendpage_ok(page)) {
>> 		kernel_sendpage();
>> 	} else {
>> 		sock_no_sendpage();
>> 	}
>>
>> This pattern (note that sock_no_sednpage was added later following bug
>> reports where nvme attempted to sendpage a slab allocated page), is
>> perfectly acceptable with normal sockets, but not with TLS.
>>
>> So there are two options:
>> 1. have tls accept MSG_SENDPAGE_NOTLAST in sendmsg (called from
>>      sock_no_sendpage)
>> 2. Make nvme set MSG_SENDPAGE_NOTLAST only when calling
>>      kernel_sendpage and clear it when calling sock_no_sendpage
>>
>> If you say that MSG_SENDPAGE_NOTLAST must be cleared when calling
>> sock_no_sendpage and it is a bug that it isn't enforced for normal tcp
>> sockets, then we need to change nvme, but I did not find
>> any documentation that indicates it, and right now, normal sockets
>> behave differently than tls sockets (wrt this flag in particular).
>>
>> Hope this clarifies.
> 
> Oh right, it does, the context evaporated from my head over the weekend.
> 
> IMHO it's best if the caller passes the right flags. The semantics of
> MSG_MORE vs NOTLAST are quite murky and had already caused bugs in the
> past :(
> 
> See commit d452d48b9f8b ("tls: prevent oversized sendfile() hangs by
> ignoring MSG_MORE")

Well, that is fine with me. This may change anyways with
the new MSG_SPLICE_PAGES from David.

> Alternatively we could have sock_no_sendpage drop NOTLAST to help
> all protos. But if we consider sendfile behavior as the standard
> simply clearing it isn't right, it should be a:
> 
> 	more = (flags & (MORE | NOTLAST)) == MORE | NOTLAST
> 	flags &= ~(MORE | NOTLAST)
> 	if (more)
> 		flags |= MORE

I don't think this would be the best option. Requiring callers
to clear NOTLAST if not calling sendpages is reasonable, but we
need to have this consistent. And also fix this pattern for the
rest of the kernel socket consumers that use this flag.