Re: [tipc-discussion] [PATCH net-next] tipc: keep the skb in rcv queue until the whole data is read

[Jon M. <jm...@re...>]

On 29/06/2021 17:41, Xin Long wrote:
> On Tue, Jun 29, 2021 at 3:57 PM Jon Maloy <jm...@re...> wrote:
>>
[...]
> Yes, Jon, I mean the opposite.
>
> when MSG_EOR is set, we will go with what this patch does,
> but to delete MSG_EOR if this is not the last part of the data,
> and keep MSG_EOR if this is the last part of the data.
>
> when MSG_EOR is not set, the msg will be truncated as before.

Yes, that would be a safe behavior. Is SCTP doing this?

///jon

>
>> In the first case, we don't solve any compatibility issue, if that is
>> the purpose. The programmer still has to add code to get the current
>> behavior.
>>
>> In the latter case we would be on the 100% safe side, although I have a
>> real hard time to see that this could be a real issue. Why would anybody
>> deliberately design an application for having messages truncated.
>>
>> ///jon
>>
>>
>>>> to indicate we don't want the truncating msg.
>>>>
>>>> When the msg flag returns with no MSG_EOR, it means there's more data to read.
>>>>
>>>> Thanks.
>>>>> - Erin
>>>>>
>>>>>> Signed-off-by: Xin Long <luc...@gm...>
>>>>>> ---
>>>>>>    net/tipc/socket.c | 30 +++++++++++++++++++++---------
>>>>>>    1 file changed, 21 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
>>>>>> index 34a97ea36cc8..504e59838b8b 100644
>>>>>> --- a/net/tipc/socket.c
>>>>>> +++ b/net/tipc/socket.c
>>>>>> @@ -1880,6 +1880,7 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
>>>>>>         bool connected = !tipc_sk_type_connectionless(sk);
>>>>>>         struct tipc_sock *tsk = tipc_sk(sk);
>>>>>>         int rc, err, hlen, dlen, copy;
>>>>>> +     struct tipc_skb_cb *skb_cb;
>>>>>>         struct sk_buff_head xmitq;
>>>>>>         struct tipc_msg *hdr;
>>>>>>         struct sk_buff *skb;
>>>>>> @@ -1903,6 +1904,7 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
>>>>>>                 if (unlikely(rc))
>>>>>>                         goto exit;
>>>>>>                 skb = skb_peek(&sk->sk_receive_queue);
>>>>>> +             skb_cb = TIPC_SKB_CB(skb);
>>>>>>                 hdr = buf_msg(skb);
>>>>>>                 dlen = msg_data_sz(hdr);
>>>>>>                 hlen = msg_hdr_sz(hdr);
>>>>>> @@ -1922,18 +1924,27 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
>>>>>>
>>>>>>         /* Capture data if non-error msg, otherwise just set return value */
>>>>>>         if (likely(!err)) {
>>>>>> -             copy = min_t(int, dlen, buflen);
>>>>>> -             if (unlikely(copy != dlen))
>>>>>> -                     m->msg_flags |= MSG_TRUNC;
>>>>>> -             rc = skb_copy_datagram_msg(skb, hlen, m, copy);
>>>>>> +             int offset = skb_cb->bytes_read;
>>>>>> +
>>>>>> +             copy = min_t(int, dlen - offset, buflen);
>>>>>> +             rc = skb_copy_datagram_msg(skb, hlen + offset, m, copy);
>>>>>> +             if (unlikely(rc))
>>>>>> +                     goto exit;
>>>>>> +             if (unlikely(offset + copy < dlen)) {
>>>>>> +                     if (!(flags & MSG_PEEK))
>>>>>> +                             skb_cb->bytes_read = offset + copy;
>>>>>> +             } else {
>>>>>> +                     m->msg_flags |= MSG_EOR;
>>>>>> +                     skb_cb->bytes_read = 0;
>>>>>> +             }
>>>>>>         } else {
>>>>>>                 copy = 0;
>>>>>>                 rc = 0;
>>>>>> -             if (err != TIPC_CONN_SHUTDOWN && connected && !m->msg_control)
>>>>>> +             if (err != TIPC_CONN_SHUTDOWN && connected && !m->msg_control) {
>>>>>>                         rc = -ECONNRESET;
>>>>>> +                     goto exit;
>>>>>> +             }
>>>>>>         }
>>>>>> -     if (unlikely(rc))
>>>>>> -             goto exit;
>>>>>>
>>>>>>         /* Mark message as group event if applicable */
>>>>>>         if (unlikely(grp_evt)) {
>>>>>> @@ -1956,9 +1967,10 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
>>>>>>                 tipc_node_distr_xmit(sock_net(sk), &xmitq);
>>>>>>         }
>>>>>>
>>>>>> -     tsk_advance_rx_queue(sk);
>>>>>> +     if (!skb_cb->bytes_read)
>>>>>> +             tsk_advance_rx_queue(sk);
>>>>>>
>>>>>> -     if (likely(!connected))
>>>>>> +     if (likely(!connected) || skb_cb->bytes_read)
>>>>>>                 goto exit;
>>>>>>
>>>>>>         /* Send connection flow control advertisement when applicable */
>>>>>> --
>>>>>> 2.27.0
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> tipc-discussion mailing list
>>>>>> tip...@li...
>>>>>> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>>> _______________________________________________
>>> tipc-discussion mailing list
>>> tip...@li...
>>> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>>>