[PATCH mptcp-next] tcp: Add mss zero checks to avoid divide errors

[PATCH mptcp-next] tcp: Add mss zero checks to avoid divide errors

[Geliang Tang]

If mss_now is set to zero when invoking tcp_push(), this divide error will occur:

[  120.369955] divide error: 0000 [#1] PREEMPT SMP NOPTI
[  120.369969] CPU: 4 PID: 15485 Comm: test_progs Tainted: G           OE      6.0.0-rc3-mptcp+ #251
[  120.369976] Hardware name: LENOVO 20UASA0901/20UASA0901, BIOS N2WET27W (1.17 ) 03/29/2021
[  120.369980] RIP: 0010:tcp_tso_segs+0x68/0x90
[  120.369990] Code: 0f b6 8e c9 03 00 00 d3 e8 89 c1 8b 83 f4 01 00 00 83 f9 1f 77 09 89 c6 d3 ee 89 f1 48 01 ca 48 39 d0 89 ee 48 0f 47 c2 31 d2 <48> f7 f6 0f b7 93 06 02 00 00 5b 5d 39 c7 0f 47 c7 39 d0 0f 47 c2
[  120.369998] RSP: 0018:ffffab29d1547c60 EFLAGS: 00010246
[  120.370004] RAX: 000000000000febf RBX: ffff9b4a82f83480 RCX: 000000000000febf
[  120.370009] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000002
[  120.370013] RBP: 0000000000000000 R08: 0000000000000a20 R09: 0000000000000000
[  120.370017] R10: 0000000000000000 R11: ffffab29d1547ae0 R12: ffff9b4b02980000
[  120.370022] R13: 0000000000000000 R14: 0000000000000000 R15: ffff9b4a867ec100
[  120.370027] FS:  00007f73957ff700(0000) GS:ffff9b4ded700000(0000) knlGS:0000000000000000
[  120.370032] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  120.370037] CR2: 00007f73957ffda0 CR3: 0000000104102005 CR4: 00000000003706e0
[  120.370042] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  120.370046] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  120.370050] Call Trace:
[  120.370056]  <TASK>
[  120.370062]  tcp_write_xmit+0x80/0x1270
[  120.370069]  __tcp_push_pending_frames+0x32/0xf0
[  120.370076]  __mptcp_push_pending+0x401/0x430
[  120.370083]  mptcp_sendmsg+0x41d/0x4c0
[  120.370089]  sock_sendmsg+0x58/0x70
[  120.370094]  __sys_sendto+0x11e/0x150
[  120.370100]  ? mptcp_setsockopt+0x2a8/0x1170
[  120.370109]  ? aa_sk_perm+0x3e/0x1f0
[  120.370116]  ? fpregs_assert_state_consistent+0x22/0x50
[  120.370129]  __x64_sys_sendto+0x24/0x30
[  120.370133]  do_syscall_64+0x37/0x90
[  120.370138]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
[  120.370145] RIP: 0033:0x7f7395c15b66
[  120.370150] Code: d5 53 49 89 f4 89 fb 48 83 ec 10 e8 c4 f7 ff ff 45 31 c9 89 c5 45 31 c0 45 89 f2 4c 89 ea 4c 89 e6 89 df b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 36 89 ef 48 89 44 24 08 e8 f6 f7 ff ff 48 8b
[  120.370157] RSP: 002b:00007f73957fe860 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
[  120.370163] RAX: ffffffffffffffda RBX: 000000000000000b RCX: 00007f7395c15b66
[  120.370167] RDX: 00000000000005dc RSI: 00007f73957fe8c0 RDI: 000000000000000b
[  120.370172] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[  120.370176] R10: 0000000000000000 R11: 0000000000000246 R12: 00007f73957fe8c0
[  120.370180] R13: 00000000000005dc R14: 0000000000000000 R15: 0000000000000000
[  120.370187]  </TASK>
[  120.370190] Modules linked in: bpf_testmod(OE) uhid btusb btrtl btbcm btintel bluetooth nf_tables ecdh_generic ecc iptable_nat bpfilter i2c_designware_platform i2c_designware_core iwlmvm x86_pkg_temp_thermal iwlwifi intel_lpss_pci thinkpad_acpi intel_lpss mfd_core ledtrig_audio battery platform_profile intel_pmc_core intel_hid hid_multitouch hid_generic usbhid nvme nvme_core i2c_hid_acpi i2c_hid pinctrl_cannonlake sg dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua efivarfs
[  120.370238] ---[ end trace 0000000000000000 ]---
[  120.378634] RIP: 0010:tcp_tso_segs+0x68/0x90
[  120.378644] Code: 0f b6 8e c9 03 00 00 d3 e8 89 c1 8b 83 f4 01 00 00 83 f9 1f 77 09 89 c6 d3 ee 89 f1 48 01 ca 48 39 d0 89 ee 48 0f 47 c2 31 d2 <48> f7 f6 0f b7 93 06 02 00 00 5b 5d 39 c7 0f 47 c7 39 d0 0f 47 c2
[  120.378649] RSP: 0018:ffffab29d1547c60 EFLAGS: 00010246
[  120.378653] RAX: 000000000000febf RBX: ffff9b4a82f83480 RCX: 000000000000febf
[  120.378656] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000002
[  120.378658] RBP: 0000000000000000 R08: 0000000000000a20 R09: 0000000000000000
[  120.378661] R10: 0000000000000000 R11: ffffab29d1547ae0 R12: ffff9b4b02980000
[  120.378664] R13: 0000000000000000 R14: 0000000000000000 R15: ffff9b4a867ec100
[  120.378666] FS:  00007f73957ff700(0000) GS:ffff9b4ded700000(0000) knlGS:0000000000000000
[  120.378670] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  120.378672] CR2: 00007f73957ffda0 CR3: 0000000104102005 CR4: 00000000003706e0
[  120.378675] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  120.378677] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

This patch adds necessary zero checks for mss_now in
tcp_set_skb_tso_segs() and tcp_tso_autosize() to avoid this divide
error.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/ipv4/tcp_output.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 290019de766d..74db29eb6ba6 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1441,7 +1441,7 @@ static void tcp_queue_skb(struct sock *sk, struct sk_buff *skb)
 /* Initialize TSO segments for a packet. */
 static void tcp_set_skb_tso_segs(struct sk_buff *skb, unsigned int mss_now)
 {
-	if (skb->len <= mss_now) {
+	if (!mss_now || skb->len <= mss_now) {
 		/* Avoid the costly divide in the normal
 		 * non-TSO case.
 		 */
@@ -1971,6 +1971,9 @@ static u32 tcp_tso_autosize(const struct sock *sk, unsigned int mss_now,
 	unsigned long bytes;
 	u32 r;
 
+	if (!mss_now)
+		return min_tso_segs;
+
 	bytes = sk->sk_pacing_rate >> READ_ONCE(sk->sk_pacing_shift);
 
 	r = tcp_min_rtt(tcp_sk(sk)) >> READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_tso_rtt_log);
-- 
2.35.3

Re: tcp: Add mss zero checks to avoid divide errors: Tests Results

[MPTCP CI]

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): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/5840132505862144
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5840132505862144/summary/summary.txt

- KVM Validation: debug:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5277182552440832
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5277182552440832/summary/summary.txt

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


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)

Re: [PATCH mptcp-next] tcp: Add mss zero checks to avoid divide errors

[Paolo Abeni]

On Fri, 2022-09-09 at 20:19 +0800, Geliang Tang wrote:
> If mss_now is set to zero when invoking tcp_push(), this divide error will occur:

We should not call at all tcp_push() in such situation. How did you
reproduce the issue?

Adding more check in TCP for the above situation is very likely not the
correct thing to do. Instead we should prevent MPTCP from reaching for
tcp_push() in the critical situation.

Thanks!

Paolo

Re: [PATCH mptcp-next] tcp: Add mss zero checks to avoid divide errors

[Geliang Tang]

Hi Paolo,

Sorry for the late reply.

Paolo Abeni <pabeni@redhat.com> 于2022年9月9日周五 22:44写道：
>
> On Fri, 2022-09-09 at 20:19 +0800, Geliang Tang wrote:
> > If mss_now is set to zero when invoking tcp_push(), this divide error will occur:
>
> We should not call at all tcp_push() in such situation. How did you
> reproduce the issue?

This does not happen on the export branch, but only when I add the BPF
redundant scheduler.

>
> Adding more check in TCP for the above situation is very likely not the
> correct thing to do. Instead we should prevent MPTCP from reaching for
> tcp_push() in the critical situation.

I agree. Let's just drop this patch. I'll add the new one to avoid
calling tcp_push() in such situation into the " BPF redundant scheduler"
series If we really need it.

Thanks!
-Geliang

>
> Thanks!
>
> Paolo
>
>