Re: [PATCH mptcp-next 5/6] mptcp: trim the duplicated skb head at receive enqueue

> >  
> >  My description was inaccurate. This only occurs in the MPTCP out-of-
> >  order scenario. It does not happen with TCP.
> > 

Hi Paolo,

Sorry for the late feedback. Geliang and I also think the trim is not a good
way to address TLS issues.

> I mean: even for plain TCP skbs can sits in the receive queue with some
> heading bytes overlapping with already received ones, and skipped at
> read time due to `copied_seq`, how comes that TLS has no problem is such
> a case?


In TLS, 'tls_strp_check_queue_ok' will check this using 'TCK_SKB_CB(skb)->seq',
like the comment said:

'''
        /* Make sure there's no duplicate data in the queue,
         * and the decrypted status matches.
         */
'''

It the check return false, the 'tls_strp_read_copy' will drop the frag_list
and to use copy_mode.

This is imported by Jakub in 0d87bbd39d7('tls: strp: make sure the TCP skbs
do not have overlapping data'). 

But I think it may missed one path in 'tls_rx_msg_size' before
'tls_strp_check_queue_ok', and that's where we met the problem in MPTCP:

'''
527         if (!strp->stm.full_len) {
528                 sz = tls_rx_msg_size(strp, strp->anchor);
...
536         }
537 
538         if (!tls_strp_check_queue_ok(strp))
539                 return tls_strp_read_copy(strp, false);
'''

In 'tls_rx_msg_size', it use 'skb_copy_bits' directly without considering overlap:


'''
2163 int tls_rx_msg_size(struct tls_strparser *strp, struct sk_buff *skb)
2164 {
...
2183         ret = skb_copy_bits(skb, strp->stm.offset, header, prot->prepend_size);
2184         if (ret < 0)
2185                 goto read_failure;
2186 
2187         strp->mark = header[0];

'''

So, Geliang and I add a new helper to replace skb_copy_bits to address
this issue:

'''
static int tls_get_header(struct sk_buff *skb, int offset, void *buf, int len) 
{
        struct sk_buff *first = skb_shinfo(skb)->frag_list;
        struct sk_buff *iter;
        int copied = 0; 
        u32 end_seq;

        if (likely(!first || first->len - offset >= len))
                return skb_copy_bits(skb, offset, buf, len);

        end_seq = TLS_SKB_CB(first)->seq + offset;

        for (iter = first; copied != len; iter = iter->next) {
                int start = end_seq - TLS_SKB_CB(iter)->seq;
                int count;

                if (start > iter->len)
                        return -EFAULT;

                count = min_t(int, iter->len - start, len - copied);
                if (skb_copy_bits(iter, start, buf + copied, count))
                        return -EFAULT;

                copied += count;
                end_seq = TLS_SKB_CB(iter)->seq + iter->len;
        }

        return 0;
}

'''

It works well without trim_head in KTLS over MPTCP, but regarding the TCP
reproducer: as the commit log in Jakub's commit, it really rare,
we cannot easily reproduce the overlap on plain TCP.

Could you please share your thoughts on our analysis and the TCP reproducer?

Looking forward for your feedback.

Thanks
Gang



/

> 
> /P
>