Re: [PATCH] mptcp: add MSG_PEEK support

[Paolo Abeni <pabeni@redhat.com>]

Hello,

On Tue, 2021-04-13 at 15:24 +0800, Yonglong Li wrote:
> This patch adds support for MSG_PEEK flag. Packets are not removed
> from the receive_queue if MSG_PEEK set in recv() system call.
> 
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
>  net/mptcp/protocol.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2d895c3..65448e1 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1710,12 +1710,13 @@ static void mptcp_wait_data(struct sock *sk, long *timeo)
>  
>  static int __mptcp_recvmsg_mskq(struct mptcp_sock *msk,
>  				struct msghdr *msg,
> -				size_t len)
> +				size_t len, int flags)
>  {
>  	struct sk_buff *skb;
>  	int copied = 0;
>  
> -	while ((skb = skb_peek(&msk->receive_queue)) != NULL) {
> +	skb = skb_peek(&msk->receive_queue);
> +	while (!(skb == NULL || skb == (struct sk_buff *)(&msk->receive_queue))) {

I think you can replace the above construct with skb_queue_walk_safe().
Will be cleaner and will avoid the skb_peek()/skb = skb->next later.

>  		u32 offset = MPTCP_SKB_CB(skb)->offset;
>  		u32 data_len = skb->len - offset;
>  		u32 count = min_t(size_t, len - copied, data_len);
> @@ -1731,15 +1732,21 @@ static int __mptcp_recvmsg_mskq(struct mptcp_sock *msk,
>  		copied += count;
>  
>  		if (count < data_len) {
> -			MPTCP_SKB_CB(skb)->offset += count;
> +			if (!(flags & MSG_PEEK))
> +				MPTCP_SKB_CB(skb)->offset += count;
>  			break;
>  		}
>  
> -		/* we will bulk release the skb memory later */
> -		skb->destructor = NULL;
> -		msk->rmem_released += skb->truesize;
> -		__skb_unlink(skb, &msk->receive_queue);
> -		__kfree_skb(skb);
> +		if (!(flags & MSG_PEEK))
> +			/* we will bulk release the skb memory later */

It looks like there is a missing bracket here.

> +			skb->destructor = NULL;
> +			msk->rmem_released += skb->truesize;
> +			__skb_unlink(skb, &msk->receive_queue);
> +			__kfree_skb(skb);
> +			skb = skb_peek(&msk->receive_queue);
> +		}else{

Uhm... this causes a compiler error due to missing paired opening
bracket.

Additional minor hint, a space is needed before and after the 'else'

> +		    skb = skb->next;
> +		}
>  
>  		if (copied >= len)
>  			break;
> @@ -1933,7 +1940,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  	while (copied < len) {
>  		int bytes_read;
>  
> -		bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied);
> +		bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied, flags);
>  		if (unlikely(bytes_read < 0)) {
>  			if (!copied)
>  				copied = bytes_read;
> @@ -2017,7 +2024,8 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  	pr_debug("msk=%p data_ready=%d rx queue empty=%d copied=%d",
>  		 msk, test_bit(MPTCP_DATA_READY, &msk->flags),
>  		 skb_queue_empty_lockless(&sk->sk_receive_queue), copied);
> -	mptcp_rcv_space_adjust(msk, copied);
> +	if (!(flags & MSG_PEEK))
> +		mptcp_rcv_space_adjust(msk, copied);
>  
>  	release_sock(sk);
>  	return copied;

This lacks some other changes in mptcp_recvmsg().
Currently mptcp_recvmsg() fails with -EOPNOTSUPP if an unsupported flag
is provided as an argument, and MSG_PEEK is not yet in the supported
list.

Side note: we need to change the above: mptcp_recvmsg() should silently
ignore unsupported flags.

Finally you should add some self-tests for the new feature (in a
separate patch). You could extend the 'mptcp_connect.c' program and add
some new script in the 'mptcp' self-test directory. That will allow you
to test your code, as needed.

If you are interested in larger contribution to the MPTCP protocol, I
suggest to try to join the public mtg we have every week:

https://github.com/multipath-tcp/mptcp_net-next/wiki/Meetings

The current timeslot is likely unfortunate, but we could change that.

Cheers,

Paolo