Re: [PATCH mptcp-next 3/6] mptcp: implement peek_len for proto_ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mptcp: mptcp_inq() under-reports readable bytes by 1 without DATA_FIN
- [Medium] mptcp: mptcp_inq() returns bogus byte count for TFO data after 3rd ACK
- [Medium] mptcp: mptcp_inq() and mptcp_ioctl() mismatch commit message
--

commit 2cfec08fb299c62052c815a2a207f3aacada8176
Author: Geliang Tang <geliang@kernel.org>

mptcp: implement peek_len for proto_ops

This commit adds mptcp_inq() and mptcp_peek_len() to report readable bytes on
an MPTCP socket, wires .peek_len into mptcp_stream_ops and
mptcp_v6_stream_ops, and switches SIOCINQ in mptcp_ioctl() to use mptcp_inq().

>     mptcp: implement peek_len for proto_ops
>     
>     Add mptcp_inq() to compute the number of readable bytes at the MPTCP
>     level. It derives the count from ack_seq - copied_seq, returns 0 while
>     the connection is still handshaking (TCP_SYN_SENT/TCP_SYN_RECV), returns
>     0 when the connection is closed (TCP_CLOSE or RCV_SHUTDOWN), and
>     subtracts 1 once a FIN has been received, mirroring tcp_inq().
>     
>     READ_ONCE() is used instead of mptcp_data_lock() for the u64 reads of
>     ack_seq and copied_seq because mptcp_inq() will be called from TLS
>     contexts with the socket lock held - taking the spinlock would deadlock.
>     
>     On 32-bit systems a torn read can produce a wrapped subtraction (close to
>     2^64). When hint_val >= INT_MAX, we return 0 instead of INT_MAX to avoid
>     reporting 2GB of readable data when there may be none. This is likely
>     caused by a torn read rather than a legitimate large receive window, as
>     normal receive queues should not exceed INT_MAX bytes.
>     
>     Wire mptcp_inq() into mptcp_peek_len() and assign .peek_len in both
>     mptcp_stream_ops and mptcp_v6_stream_ops. This is preparatory work for
>     an upcoming kTLS-over-MPTCP series.
>     
>     Note: mptcp_inq() intentionally differs from mptcp_inq_hint() when the
>     calculated value is 0. mptcp_inq_hint() returns 1 to signal EOF to
>     ioctl/cmsg users (mirroring tcp_inq_hint()), while mptcp_inq() returns
>     0 because peek_len should report the actual readable byte count
>     (mirroring tcp_inq()).
>     
>     Co-developed-by: Zqiang <qiang.zhang@linux.dev>
>     Signed-off-by: Zqiang <qiang.zhang@linux.dev>
>     Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0caa31525b907..71a0a28b0d3c9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -4188,7 +4189,7 @@ static int mptcp_ioctl(struct sock *sk, int cmd, int *karg)
>  		lock_sock(sk);
>  		if (mptcp_move_skbs(sk))
>  			mptcp_cleanup_rbuf(msk, 0);
> -		*karg = mptcp_inq_hint(sk);
> +		*karg = mptcp_inq(sk);
>  		release_sock(sk);
>  		break;
>  	case SIOCOUTQ:

[Severity: Medium]
Does this change in mptcp_ioctl() contradict the commit message?

The commit message notes that mptcp_inq_hint() returns 1 to signal EOF to
ioctl/cmsg users, while mptcp_inq() returns 0 for peek_len. However, switching
SIOCINQ in mptcp_ioctl() from mptcp_inq_hint(sk) to mptcp_inq(sk) causes
ioctl(SIOCINQ) to return 0 instead of 1 at EOF when sk->sk_receive_queue is
empty and TCP_CLOSE or RCV_SHUTDOWN is set.

In addition, the commit message describes mptcp_inq() as reading a scalar
copied_seq with READ_ONCE(), returning 0 when TCP_CLOSE or RCV_SHUTDOWN is
set, and avoiding mptcp_data_lock() because taking it under the socket lock
would deadlock. In the implementation below, struct mptcp_sock has no
copied_seq field; mptcp_inq() instead peeks sk->sk_receive_queue and reads
MPTCP_SKB_CB(skb)->map_seq without READ_ONCE(), decrements answ rather than
returning 0 on TCP_CLOSE or RCV_SHUTDOWN when an skb is queued, and
mptcp_move_skbs() right above takes mptcp_data_lock() while holding
lock_sock(sk).

Should SIOCINQ in mptcp_ioctl() keep calling mptcp_inq_hint(sk), and could the
commit message be updated to match the implementation?

> @@ -4797,6 +4798,39 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
>  	return ret;
>  }
>  
> +static int mptcp_inq(struct sock *sk)
> +{
> +	const struct mptcp_sock *msk = mptcp_sk(sk);
> +	const struct sk_buff *skb;
> +	u64 hint_val;
> +	int answ;
> +
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> +		return 0;
> +
> +	skb = skb_peek(&sk->sk_receive_queue);
> +	if (!skb)
> +		return 0;
> +
> +	hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> +	if (hint_val >= INT_MAX)
> +		return 0;

[Severity: Medium]
Can this sequence subtraction return 0 (or a bogus byte count) when unread TCP
Fast Open data is still at the head of sk->sk_receive_queue after the 3rd ACK
arrives?

On a passive MPTCP Fast Open connection,
mptcp_fastopen_subflow_synack_set_params() in net/mptcp/fastopen.c queues the
SYN data skb onto sk->sk_receive_queue while msk->ack_seq is 0:

    /* Only the sequence delta is relevant */
    MPTCP_SKB_CB(skb)->map_seq = -skb->len;
    MPTCP_SKB_CB(skb)->end_seq = 0;
    MPTCP_SKB_CB(skb)->offset = 0;

Commit f03afb3aeb9d ("mptcp: drop __mptcp_fastopen_gen_msk_ackseq()") removed
the helper that rebased the queued TFO skb's map_seq when the 3rd ACK arrives.
When the 3rd ACK carrying MP_CAPABLE is processed, subflow_set_remote_key() in
net/mptcp/subflow.c sets msk->ack_seq to the 64-bit initial data sequence
number:

    WRITE_ONCE(msk->remote_key, subflow->remote_key);
    WRITE_ONCE(msk->ack_seq, subflow->iasn);

If the TFO skb at the head of sk->sk_receive_queue has not yet been fully
consumed when mptcp_inq() runs, READ_ONCE(msk->ack_seq) -
MPTCP_SKB_CB(skb)->map_seq evaluates to subflow->iasn + remaining_tfo_len
(plus any subsequent data). Because subflow->iasn is a 64-bit hash of the
remote key, hint_val will almost always be >= INT_MAX and cause mptcp_inq() to
return 0 (or a bogus value if it wraps below INT_MAX) even though unread TFO
data is queued.

Would using mptcp_data_avail(msk), i.e. READ_ONCE(msk->bytes_received) -
READ_ONCE(msk->bytes_consumed), avoid this issue?

> +
> +	answ = (unsigned int)hint_val;
> +
> +	/* Subtract 1, if FIN was received. Mirror tcp_inq() */
> +	if (answ &&
> +	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
> +		answ--;
> +
> +	return answ;
> +}

[Severity: Medium]
Won't subtracting 1 whenever sk->sk_state == TCP_CLOSE or RCV_SHUTDOWN is set
under-report the readable byte count by 1 whenever the socket enters those
states without processing an in-sequence DATA_FIN?

In TCP, tcp_inq() decrements answ only when sock_flag(sk, SOCK_DONE) is set,
which happens in tcp_fin() when an in-sequence FIN has been received and
tp->rcv_nxt has been incremented by 1.

In MPTCP, payload skbs queued onto sk->sk_receive_queue advance msk->ack_seq
by the exact payload length, and the only place that increments msk->ack_seq
by an extra 1 for DATA_FIN is mptcp_check_data_fin() in net/mptcp/protocol.c:

    if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
        WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
        WRITE_ONCE(msk->rcv_data_fin, 0);

        WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);

However, sk->sk_state == TCP_CLOSE or (sk->sk_shutdown & RCV_SHUTDOWN) can be
set while unread data remains in sk->sk_receive_queue without
mptcp_check_data_fin() ever incrementing msk->ack_seq:

1. A local user calls shutdown(fd, SHUT_RD) or shutdown(fd, SHUT_RDWR), where
   inet_shutdown() in net/ipv4/af_inet.c sets RCV_SHUTDOWN on sk->sk_shutdown
   without modifying msk->ack_seq or purging sk->sk_receive_queue:

    default:
        WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | how);
        if (sk->sk_prot->shutdown)
            sk->sk_prot->shutdown(sk, how);
        break;

2. The peer aborts the connection with MP_FASTCLOSE, where
   mptcp_check_fastclose() in net/mptcp/protocol.c sets TCP_CLOSE and
   SHUTDOWN_MASK without incrementing msk->ack_seq:

    mptcp_set_state(sk, TCP_CLOSE);
    WRITE_ONCE(sk->sk_shutdown, SHUTDOWN_MASK);

3. A fallback subflow receives a TCP RST after data, where
   __mptcp_subflow_error_report() in net/mptcp/protocol.c transitions the
   MPTCP socket to TCP_CLOSE without incrementing msk->ack_seq (and commit
   f3589041bbf5, "mptcp: prevent dummy DATA_FIN on RST in fallback mode",
   explicitly avoided synthesizing a dummy DATA_FIN on RST):

    ssk_state = inet_sk_state_load(ssk);
    if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
        mptcp_set_state(sk, ssk_state);

4. All subflows close without DATA_FIN or time out in __mptcp_close_ssk() /
   mptcp_do_fastclose(), setting sk->sk_state to TCP_CLOSE without
   incrementing msk->ack_seq.

In all of these cases, READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq is
the exact number of unread payload bytes N in sk->sk_receive_queue, so
decrementing answ causes mptcp_inq(), mptcp_peek_len(), and
mptcp_ioctl(SIOCINQ) to return N - 1 (and 0 when 1 byte of unread data
remains), which can also stall strp_data_ready() when waiting for
strp_peek_len() >= strp->need_bytes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792999988.git.tanggeliang@kylinos.cn?part=3