[PATCH mptcp-next v2] Revert "mptcp: add get_subflow wrappers" - fix divide error in mptcp_subflow_get_send

Geliang Tang posted 1 patch 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/95f77f38e54f9564608e844f507701c04745475b.1666668425.git.geliang.tang@suse.com
Maintainers: Mat Martineau <mathew.j.martineau@linux.intel.com>, Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
net/mptcp/protocol.c | 18 ++++++++--
net/mptcp/protocol.h |  4 ---
net/mptcp/sched.c    | 82 --------------------------------------------
3 files changed, 16 insertions(+), 88 deletions(-)
[PATCH mptcp-next v2] Revert "mptcp: add get_subflow wrappers" - fix divide error in mptcp_subflow_get_send
Posted by Geliang Tang 1 month, 2 weeks ago
This reverts commit 8ae8437eb930b887219dc50b159322f6f9331bfb.

The wrapper mptcp_sched_get_send() will be added in the later patch
"mptcp: use get_send wrapper", and the wrapper mptcp_sched_get_retrans()
will be added in the later patch "mptcp: use get_retrans wrapper".

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 | 18 ++++++++--
 net/mptcp/protocol.h |  4 ---
 net/mptcp/sched.c    | 82 --------------------------------------------
 3 files changed, 16 insertions(+), 88 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2419d7be542b..3cf44b5ef937 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1406,7 +1406,7 @@ bool mptcp_subflow_active(struct mptcp_subflow_context *subflow)
  * returns the subflow that will transmit the next DSS
  * additionally updates the rtx timeout
  */
-struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
+static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 {
 	struct subflow_send_info send_info[SSK_MODE_MAX];
 	struct mptcp_subflow_context *subflow;
@@ -1417,6 +1417,15 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 	u64 linger_time;
 	long tout = 0;
 
+	sock_owned_by_me(sk);
+
+	if (__mptcp_check_fallback(msk)) {
+		if (!msk->first)
+			return NULL;
+		return __tcp_can_send(msk->first) &&
+		       sk_stream_memory_free(msk->first) ? msk->first : NULL;
+	}
+
 	/* re-use last subflow, if the burst allow that */
 	if (msk->last_snd && msk->snd_burst > 0 &&
 	    sk_stream_memory_free(msk->last_snd) &&
@@ -2210,12 +2219,17 @@ static void mptcp_timeout_timer(struct timer_list *t)
  *
  * A backup subflow is returned only if that is the only kind available.
  */
-struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk)
+static struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk)
 {
 	struct sock *backup = NULL, *pick = NULL;
 	struct mptcp_subflow_context *subflow;
 	int min_stale_count = INT_MAX;
 
+	sock_owned_by_me((const struct sock *)msk);
+
+	if (__mptcp_check_fallback(msk))
+		return NULL;
+
 	mptcp_for_each_subflow(msk, subflow) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 8f48f881adf8..270c187c9001 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -641,10 +641,6 @@ int mptcp_init_sched(struct mptcp_sock *msk,
 void mptcp_release_sched(struct mptcp_sock *msk);
 void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 				 bool scheduled);
-struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk);
-struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk);
-int mptcp_sched_get_send(struct mptcp_sock *msk);
-int mptcp_sched_get_retrans(struct mptcp_sock *msk);
 
 static inline bool __tcp_can_send(const struct sock *ssk)
 {
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 9b128714055a..d295b92a5789 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -93,85 +93,3 @@ void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 {
 	WRITE_ONCE(subflow->scheduled, scheduled);
 }
-
-static int mptcp_sched_data_init(struct mptcp_sock *msk, bool reinject,
-				 struct mptcp_sched_data *data)
-{
-	struct mptcp_subflow_context *subflow;
-	int i = 0;
-
-	data->reinject = reinject;
-
-	mptcp_for_each_subflow(msk, subflow) {
-		if (i == MPTCP_SUBFLOWS_MAX) {
-			pr_warn_once("too many subflows");
-			break;
-		}
-		mptcp_subflow_set_scheduled(subflow, false);
-		data->contexts[i++] = subflow;
-	}
-
-	for (; i < MPTCP_SUBFLOWS_MAX; i++)
-		data->contexts[i] = NULL;
-
-	msk->snd_burst = 0;
-
-	return 0;
-}
-
-int mptcp_sched_get_send(struct mptcp_sock *msk)
-{
-	struct mptcp_sched_data data;
-	struct sock *ssk = NULL;
-
-	sock_owned_by_me((const struct sock *)msk);
-
-	/* the following check is moved out of mptcp_subflow_get_send */
-	if (__mptcp_check_fallback(msk)) {
-		if (msk->first &&
-		    __tcp_can_send(msk->first) &&
-		    sk_stream_memory_free(msk->first)) {
-			mptcp_subflow_set_scheduled(mptcp_subflow_ctx(msk->first), true);
-			return 0;
-		}
-		return -EINVAL;
-	}
-
-	if (!msk->sched) {
-		ssk = mptcp_subflow_get_send(msk);
-		if (!ssk)
-			return -EINVAL;
-		mptcp_subflow_set_scheduled(mptcp_subflow_ctx(ssk), true);
-		return 0;
-	}
-
-	mptcp_sched_data_init(msk, false, &data);
-	msk->sched->get_subflow(msk, &data);
-
-	return 0;
-}
-
-int mptcp_sched_get_retrans(struct mptcp_sock *msk)
-{
-	struct mptcp_sched_data data;
-	struct sock *ssk = NULL;
-
-	sock_owned_by_me((const struct sock *)msk);
-
-	/* the following check is moved out of mptcp_subflow_get_retrans */
-	if (__mptcp_check_fallback(msk))
-		return -EINVAL;
-
-	if (!msk->sched) {
-		ssk = mptcp_subflow_get_retrans(msk);
-		if (!ssk)
-			return -EINVAL;
-		mptcp_subflow_set_scheduled(mptcp_subflow_ctx(ssk), true);
-		return 0;
-	}
-
-	mptcp_sched_data_init(msk, true, &data);
-	msk->sched->get_subflow(msk, &data);
-
-	return 0;
-}
-- 
2.35.3
Re: [PATCH mptcp-next v2] Revert "mptcp: add get_subflow wrappers" - fix divide error in mptcp_subflow_get_send
Posted by Matthieu Baerts 1 month, 1 week ago
Hi Geliang, Mat,

On 25/10/2022 05:27, Geliang Tang wrote:
> This reverts commit 8ae8437eb930b887219dc50b159322f6f9331bfb.
> 
> The wrapper mptcp_sched_get_send() will be added in the later patch
> "mptcp: use get_send wrapper", and the wrapper mptcp_sched_get_retrans()
> will be added in the later patch "mptcp: use get_retrans wrapper".
> 
> 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>

Thank you for the patch and the review!

Now in our tree:

- dce423f5bbb7: "squashed" in "mptcp: add get_subflow wrappers"
- Results: 9bcf275fb027..d708694301f4 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20221031T151210

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v2] Revert "mptcp: add get_subflow wrappers" - fix divide error in mptcp_subflow_get_send
Posted by Mat Martineau 1 month, 2 weeks ago
On Tue, 25 Oct 2022, Geliang Tang wrote:

> This reverts commit 8ae8437eb930b887219dc50b159322f6f9331bfb.
>

Yeah, this seems like the best thing to do for now. Thanks!

(Not marked as RvB since the patch will be dropped from topgit rather than 
adding a revert commit. Marked as "queued" in patchwork.)

- Mat

> The wrapper mptcp_sched_get_send() will be added in the later patch
> "mptcp: use get_send wrapper", and the wrapper mptcp_sched_get_retrans()
> will be added in the later patch "mptcp: use get_retrans wrapper".
>
> 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 | 18 ++++++++--
> net/mptcp/protocol.h |  4 ---
> net/mptcp/sched.c    | 82 --------------------------------------------
> 3 files changed, 16 insertions(+), 88 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2419d7be542b..3cf44b5ef937 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1406,7 +1406,7 @@ bool mptcp_subflow_active(struct mptcp_subflow_context *subflow)
>  * returns the subflow that will transmit the next DSS
>  * additionally updates the rtx timeout
>  */
> -struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> +static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> {
> 	struct subflow_send_info send_info[SSK_MODE_MAX];
> 	struct mptcp_subflow_context *subflow;
> @@ -1417,6 +1417,15 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> 	u64 linger_time;
> 	long tout = 0;
>
> +	sock_owned_by_me(sk);
> +
> +	if (__mptcp_check_fallback(msk)) {
> +		if (!msk->first)
> +			return NULL;
> +		return __tcp_can_send(msk->first) &&
> +		       sk_stream_memory_free(msk->first) ? msk->first : NULL;
> +	}
> +
> 	/* re-use last subflow, if the burst allow that */
> 	if (msk->last_snd && msk->snd_burst > 0 &&
> 	    sk_stream_memory_free(msk->last_snd) &&
> @@ -2210,12 +2219,17 @@ static void mptcp_timeout_timer(struct timer_list *t)
>  *
>  * A backup subflow is returned only if that is the only kind available.
>  */
> -struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk)
> +static struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk)
> {
> 	struct sock *backup = NULL, *pick = NULL;
> 	struct mptcp_subflow_context *subflow;
> 	int min_stale_count = INT_MAX;
>
> +	sock_owned_by_me((const struct sock *)msk);
> +
> +	if (__mptcp_check_fallback(msk))
> +		return NULL;
> +
> 	mptcp_for_each_subflow(msk, subflow) {
> 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 8f48f881adf8..270c187c9001 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -641,10 +641,6 @@ int mptcp_init_sched(struct mptcp_sock *msk,
> void mptcp_release_sched(struct mptcp_sock *msk);
> void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
> 				 bool scheduled);
> -struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk);
> -struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk);
> -int mptcp_sched_get_send(struct mptcp_sock *msk);
> -int mptcp_sched_get_retrans(struct mptcp_sock *msk);
>
> static inline bool __tcp_can_send(const struct sock *ssk)
> {
> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index 9b128714055a..d295b92a5789 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -93,85 +93,3 @@ void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
> {
> 	WRITE_ONCE(subflow->scheduled, scheduled);
> }
> -
> -static int mptcp_sched_data_init(struct mptcp_sock *msk, bool reinject,
> -				 struct mptcp_sched_data *data)
> -{
> -	struct mptcp_subflow_context *subflow;
> -	int i = 0;
> -
> -	data->reinject = reinject;
> -
> -	mptcp_for_each_subflow(msk, subflow) {
> -		if (i == MPTCP_SUBFLOWS_MAX) {
> -			pr_warn_once("too many subflows");
> -			break;
> -		}
> -		mptcp_subflow_set_scheduled(subflow, false);
> -		data->contexts[i++] = subflow;
> -	}
> -
> -	for (; i < MPTCP_SUBFLOWS_MAX; i++)
> -		data->contexts[i] = NULL;
> -
> -	msk->snd_burst = 0;
> -
> -	return 0;
> -}
> -
> -int mptcp_sched_get_send(struct mptcp_sock *msk)
> -{
> -	struct mptcp_sched_data data;
> -	struct sock *ssk = NULL;
> -
> -	sock_owned_by_me((const struct sock *)msk);
> -
> -	/* the following check is moved out of mptcp_subflow_get_send */
> -	if (__mptcp_check_fallback(msk)) {
> -		if (msk->first &&
> -		    __tcp_can_send(msk->first) &&
> -		    sk_stream_memory_free(msk->first)) {
> -			mptcp_subflow_set_scheduled(mptcp_subflow_ctx(msk->first), true);
> -			return 0;
> -		}
> -		return -EINVAL;
> -	}
> -
> -	if (!msk->sched) {
> -		ssk = mptcp_subflow_get_send(msk);
> -		if (!ssk)
> -			return -EINVAL;
> -		mptcp_subflow_set_scheduled(mptcp_subflow_ctx(ssk), true);
> -		return 0;
> -	}
> -
> -	mptcp_sched_data_init(msk, false, &data);
> -	msk->sched->get_subflow(msk, &data);
> -
> -	return 0;
> -}
> -
> -int mptcp_sched_get_retrans(struct mptcp_sock *msk)
> -{
> -	struct mptcp_sched_data data;
> -	struct sock *ssk = NULL;
> -
> -	sock_owned_by_me((const struct sock *)msk);
> -
> -	/* the following check is moved out of mptcp_subflow_get_retrans */
> -	if (__mptcp_check_fallback(msk))
> -		return -EINVAL;
> -
> -	if (!msk->sched) {
> -		ssk = mptcp_subflow_get_retrans(msk);
> -		if (!ssk)
> -			return -EINVAL;
> -		mptcp_subflow_set_scheduled(mptcp_subflow_ctx(ssk), true);
> -		return 0;
> -	}
> -
> -	mptcp_sched_data_init(msk, true, &data);
> -	msk->sched->get_subflow(msk, &data);
> -
> -	return 0;
> -}
> -- 
> 2.35.3
>
>

--
Mat Martineau
Intel