[PATCH 07/18] net/tls: sanitize MSG_EOR handling

[Hannes Reinecke]

On 4/18/23 12:43, Sagi Grimberg wrote:
> 
> 
> On 4/18/23 13:39, Hannes Reinecke wrote:
>> On 4/18/23 12:24, Hannes Reinecke wrote:
>>> On 4/18/23 12:07, Sagi Grimberg wrote:
>>>>
>>>>>>> The TLS stack is using MSG_EOR internally, so the flag cannot be
>>>>>>> set for sendmsg()/sendpage(). But to avoid having the caller to
>>>>>>> check whether TLS is active modify the code to clear the MSG_EOR
>>>>>>> flag. And blank out MSG_MORE / MSG_SENDPAGE_NOTLAST, too, as they
>>>>>>> conflict with MSG_EOR anyway.
>>>>>>
>>>>>> This looks like a temporary workaround to me.
>>>>>>
>>>>>> The networking folks really need to be CC'd on this (same for 
>>>>>> patch 6).
>>>>>
>>>>> Thanks, when I said "we can support EOR" I obviously meant support
>>>>> not ignore it :(  No ack.
>>>>
>>>> Obviously neither Hannes or I have sufficient tls knowledge to properly
>>>> support it... It needs to be done by someone who knows the
>>>> implementation.
>>>>
>>>> Is this a large scope to add support for it? Because if it is, I'd
>>>> simply change nvme to clear MSG_EOR when tls is used (despite my
>>>> preference to not do special things for tls).
>>>
>>> There's a rather simple patch for tls_sw. It already paces the data 
>>> internally by an 'eor' variable, which is currently set to !MSG_MORE.
>>> So evaluating MSG_EOR here is really trivial, and doesn't seem to 
>>> cause any issues.
>>>
>>> Or, at least, none which would show up with NVMe-over-TLS :-)
>>>
>> Suggestion is to change it like this (sendpage follows suit):
>>
>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
>> index 827292e29f99..7b28b11ff611 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_EOR)
> 
> You probably meant if (msg->msg_flags & MSG_EOR)...
> 
Of course.

Cheers,

Hannes