mptcp: fix divide error in mptcp_subflow_get_send

[PATCH mptcp-next] mptcp: fix divide error in mptcp_subflow_get_send

Posted by Geliang Tang 3 years, 3 months ago

Fix this divide error:

----
divide error: 0000 [#1] PREEMPT SMP KASAN NOPTI
CPU: 0 PID: 14336 Comm: syz-executor.6 Not tainted 6.1.0-rc1-00215-g47aa7f23f440 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
RIP: 0010:div_u64_rem include/linux/math64.h:29 [inline]
RIP: 0010:div_u64 include/linux/math64.h:128 [inline]
RIP: 0010:mptcp_subflow_get_send+0xa87/0x1200 net/mptcp/protocol.c:1486
----

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/314
Reported-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ddeb8b36a677..3a07c0d197d4 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1475,13 +1475,14 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 	if (!ssk || !sk_stream_memory_free(ssk))
 		return NULL;
 
-	burst = min_t(int, MPTCP_SEND_BURST_SIZE, mptcp_wnd_end(msk) - msk->snd_nxt);
-	wmem = READ_ONCE(ssk->sk_wmem_queued);
-	if (!burst) {
+	if (mptcp_wnd_end(msk) <= msk->snd_nxt) {
 		msk->last_snd = NULL;
 		return ssk;
 	}
 
+	burst = min_t(u32, MPTCP_SEND_BURST_SIZE, mptcp_wnd_end(msk) - msk->snd_nxt);
+	wmem = READ_ONCE(ssk->sk_wmem_queued);
+
 	subflow = mptcp_subflow_ctx(ssk);
 	subflow->avg_pacing_rate = div_u64((u64)subflow->avg_pacing_rate * wmem +
 					   READ_ONCE(ssk->sk_pacing_rate) * burst,
-- 
2.35.3

Re: [PATCH mptcp-next] mptcp: fix divide error in mptcp_subflow_get_send

Posted by Mat Martineau 3 years, 3 months ago

On Sat, 22 Oct 2022, Geliang Tang wrote:

> Fix this divide error:
>
> ----
> divide error: 0000 [#1] PREEMPT SMP KASAN NOPTI
> CPU: 0 PID: 14336 Comm: syz-executor.6 Not tainted 6.1.0-rc1-00215-g47aa7f23f440 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> RIP: 0010:div_u64_rem include/linux/math64.h:29 [inline]
> RIP: 0010:div_u64 include/linux/math64.h:128 [inline]
> RIP: 0010:mptcp_subflow_get_send+0xa87/0x1200 net/mptcp/protocol.c:1486
> ----
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/314
> Reported-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/protocol.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index ddeb8b36a677..3a07c0d197d4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1475,13 +1475,14 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> 	if (!ssk || !sk_stream_memory_free(ssk))
> 		return NULL;
>
> -	burst = min_t(int, MPTCP_SEND_BURST_SIZE, mptcp_wnd_end(msk) - msk->snd_nxt);
> -	wmem = READ_ONCE(ssk->sk_wmem_queued);
> -	if (!burst) {
> +	if (mptcp_wnd_end(msk) <= msk->snd_nxt) {
> 		msk->last_snd = NULL;
> 		return ssk;
> 	}
>
> +	burst = min_t(u32, MPTCP_SEND_BURST_SIZE, mptcp_wnd_end(msk) - msk->snd_nxt);
> +	wmem = READ_ONCE(ssk->sk_wmem_queued);
> +
> 	subflow = mptcp_subflow_ctx(ssk);
> 	subflow->avg_pacing_rate = div_u64((u64)subflow->avg_pacing_rate * wmem +
> 					   READ_ONCE(ssk->sk_pacing_rate) * burst,
> -- 
> 2.35.3

Hi Geliang -

I haven't been able to reproduce the problem using commit id c3917837f483 
(export/20221024T065054 without the "features other trees" patches), and 
the only change in mptcp_subflow_get_send() is the removal of the fallback 
check in "mptcp: add get_subflow wrappers". The stack trace shows that 
mptcp_subflow_get_send() is called directly by __mptcp_push_pending():

RIP: 0010:mptcp_subflow_get_send+0xa87/0x1200 net/mptcp/protocol.c:1486
...
Call Trace:
  <TASK>
  __mptcp_push_pending+0x19e/0x770 net/mptcp/protocol.c:1550
  mptcp_sendmsg+0x694/0x19d0 net/mptcp/protocol.c:1813

So I think the divide by zero is due to the missing fallback check. Seems 
like the fallback check should be moved only when mptcp_subflow_get_send() 
is only called using the wrapper? WDYT?

--
Mat Martineau
Intel

Re: mptcp: fix divide error in mptcp_subflow_get_send: Tests Results

Posted by MPTCP CI 3 years, 3 months ago

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 1 failed test(s): packetdrill_add_addr 🔴:
  - Task: https://cirrus-ci.com/task/5728997552685056
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5728997552685056/summary/summary.txt

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/5166047599263744
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5166047599263744/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/c5b3f2e01f96


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

[PATCH mptcp-next] Squash to "mptcp: delay updating already_sent"

Posted by Geliang Tang 3 years, 3 months ago

No need to update info->limit, drop it.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 958c7fd6da2b..a21bf43472f6 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1244,8 +1244,8 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	pr_debug("msk=%p ssk=%p sending dfrag at seq=%llu len=%u already sent=%u",
 		 msk, ssk, dfrag->data_seq, dfrag->data_len, info->sent);
 
-	if (info->sent > info->limit ||
-	    info->limit > dfrag->data_len)
+	if (WARN_ON_ONCE(info->sent > info->limit ||
+			 info->limit > dfrag->data_len))
 		return 0;
 
 	if (unlikely(!__tcp_can_send(ssk)))
@@ -1573,7 +1573,6 @@ static int __subflow_push_pending(struct sock *sk, struct sock *ssk,
 			}
 
 			info->sent += ret;
-			info->limit -= ret;
 			copied += ret;
 			len -= ret;
 
-- 
2.35.3

Re: [PATCH mptcp-next] Squash to "mptcp: delay updating already_sent"

Posted by Mat Martineau 3 years, 3 months ago

On Sat, 22 Oct 2022, Geliang Tang wrote:

> No need to update info->limit, drop it.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>

Hi Geliang -

Please include this in the next revision of the "BPF redundant scheduler" 
series.

Thanks,
Mat

> ---
> net/mptcp/protocol.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 958c7fd6da2b..a21bf43472f6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1244,8 +1244,8 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
> 	pr_debug("msk=%p ssk=%p sending dfrag at seq=%llu len=%u already sent=%u",
> 		 msk, ssk, dfrag->data_seq, dfrag->data_len, info->sent);
>
> -	if (info->sent > info->limit ||
> -	    info->limit > dfrag->data_len)
> +	if (WARN_ON_ONCE(info->sent > info->limit ||
> +			 info->limit > dfrag->data_len))
> 		return 0;
>
> 	if (unlikely(!__tcp_can_send(ssk)))
> @@ -1573,7 +1573,6 @@ static int __subflow_push_pending(struct sock *sk, struct sock *ssk,
> 			}
>
> 			info->sent += ret;
> -			info->limit -= ret;
> 			copied += ret;
> 			len -= ret;
>
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel