Re: [PATCH net v3 2/2] skmsg: bugfix for sk_msg sge iteration

Hi John,

On Mon, 2024-07-01 at 18:29 +0800, Geliang Tang wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Hello,

On Mon, 2024-07-01 at 17:00 +0800, D. Wythe wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

On 6/28/24 1:47 PM, Geliang Tang wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

From: Geliang Tang
<tanggeliang@kylinos.cn

quoted

Every time run this BPF
selftests
(./test_sockmap) on a
Loongarch
platform,

This issue seems related to "txmsg_cork".

Every subtest of test_sockmap with a txmsg_cork value greater than 1
will trigger this issue. For example, this test_txmsg_cork_hangs test:

static void test_txmsg_cork_hangs(int cgrp, struct sockmap_options
*opt)
{
        txmsg_pass = 1; 
        txmsg_redir = 0; 
        txmsg_cork = 4097;
        txmsg_apply = 4097;
        test_send_large(opt, cgrp);

        txmsg_pass = 0; 
        txmsg_redir = 1; 
        txmsg_apply = 0; 
        txmsg_cork = 4097;
        test_send_large(opt, cgrp);

        txmsg_pass = 0; 
        txmsg_redir = 1; 
        txmsg_apply = 4097;
        txmsg_cork = 4097;
        test_send_large(opt, cgrp);
}

These tests will break the sk_msg sge iteration in
sk_msg_memcopy_from_iter().

I added the following test code:

'''

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index bbf40b999713..e1fd40aa8586 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c

@@ -374,6 +374,7 @@ int sk_msg_memcopy_from_iter(struct sock *sk,

struct iov_iter *from,
 	void *to;
 
 	do {
+		pr_info("%s i=%d end=%d\n", __func__, i, msg->sg.end);
 		sge = sk_msg_elem(msg, i);
 		/* This is possible if a trim operation shrunk the
buffer */
 		if (msg->sg.copybreak >= sge->length) {
--
'''

And got this logs when running "test hanging corks" test:

'''
# 1/ 1  sockmap::txmsg test hanging corks:OK
# 2/ 1 sockhash::txmsg test hanging corks:OK
# 3/ 1 sockhash:ktls:txmsg test hanging corks:OK

[   55.751687] sk_msg_memcopy_from_iter i=0 end=8
[   55.751712] sk_msg_memcopy_from_iter i=1 end=8
[   55.751726] sk_msg_memcopy_from_iter i=2 end=8
[   55.751769] sk_msg_memcopy_from_iter i=3 end=8
[   55.751778] sk_msg_memcopy_from_iter i=4 end=8
[   55.751787] sk_msg_memcopy_from_iter i=5 end=8
[   55.751796] sk_msg_memcopy_from_iter i=6 end=8
[   55.751805] sk_msg_memcopy_from_iter i=7 end=8
[   55.752979] sk_msg_memcopy_from_iter i=8 end=16
[   55.752988] sk_msg_memcopy_from_iter i=9 end=16
[   55.752995] sk_msg_memcopy_from_iter i=10 end=16
[   55.753002] sk_msg_memcopy_from_iter i=11 end=16
[   55.753008] sk_msg_memcopy_from_iter i=12 end=16
[   55.753015] sk_msg_memcopy_from_iter i=13 end=16
[   55.753022] sk_msg_memcopy_from_iter i=14 end=16
[   55.753028] sk_msg_memcopy_from_iter i=15 end=16
[   56.087047] sk_msg_memcopy_from_iter i=0 end=1
[   56.087075] sk_msg_memcopy_from_iter i=1 end=1
[   56.087081] sk_msg_memcopy_from_iter i=3 end=1
[   56.087086] sk_msg_memcopy_from_iter i=5 end=1
[   56.087091] sk_msg_memcopy_from_iter i=7 end=1
[   56.087095] sk_msg_memcopy_from_iter i=9 end=1
[   56.087100] sk_msg_memcopy_from_iter i=11 end=1
[   56.087105] sk_msg_memcopy_from_iter i=13 end=1
[   56.087110] sk_msg_memcopy_from_iter i=15 end=1
[   56.087115] sk_msg_memcopy_from_iter i=17 end=1
'''

When "i" is greater than "end", the sge we get is empty, sge->length is
0. If we access this sge, this issue will occur. Kernel panics on some
machines.

To fix this, we can't "break" the loop like I did in this patch itself.
It will break test_sockmap with the following error logs:

# 1/ 1  sockmap::txmsg test hanging corks:OK
sendpage loop error: No space left on device
msg_loop_tx: iov_count 1024 iov_buf 256 cnt 2 err -1
tx thread exited with err 1.
# 2/ 1 sockhash::txmsg test hanging corks:FAIL
# 3/ 1 sockhash:ktls:txmsg test hanging corks:OK
Pass: 2 Fail: 1

We must "continue" the loop to get the next valid sge.

A better fix is here:

'''

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index bbf40b999713..bbaf909d0f9c 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c

@@ -376,13 +376,8 @@ int sk_msg_memcopy_from_iter(struct sock *sk,

struct iov_iter *from,
 	do {
 		sge = sk_msg_elem(msg, i);
 		/* This is possible if a trim operation shrunk the
buffer */
-		if (msg->sg.copybreak >= sge->length) {
-			msg->sg.copybreak = 0;
-			sk_msg_iter_var_next(i);
-			if (i == msg->sg.end)
-				break;
-			sge = sk_msg_elem(msg, i);
-		}
+		if (msg->sg.copybreak >= sge->length)
+			goto next;
 
 		buf_size = sge->length - msg->sg.copybreak;
 		copy = (buf_size > bytes) ? bytes : buf_size;

@@ -399,6 +394,7 @@ int sk_msg_memcopy_from_iter(struct sock *sk,

struct iov_iter *from,
 		bytes -= copy;
 		if (!bytes)
 			break;
+next:
 		msg->sg.copybreak = 0;
 		sk_msg_iter_var_next(i);
 	} while (i != msg->sg.end);
-- 
'''

WDYT? I want to hear your opinion.

Thanks,
-Geliang

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

a Kernel panic occurs:

'''
  Oops[#1]:
  CPU: 20 PID: 23245
Comm: test_sockmap
Tainted: G     OE
6.10.0-
rc2+ #32
  Hardware name:
LOONGSON
Dabieshan/Loongson-
TC542F0, BIOS
Loongson-UDK2018
  ... ...
     ra:
90000000043a315c
tcp_bpf_sendmsg+0x23c/0
x420
    ERA:
900000000426cd1c
sk_msg_memcopy_from_ite
r+0xbc/0x220
   CRMD: 000000b0 (PLV0
-IE -DA +PG DACF=CC
DACM=CC -WE)
   PRMD: 0000000c
(PPLV0 +PIE +PWE)
   EUEN: 00000007 (+FPE
+SXE +ASXE -BTE)
   ECFG: 00071c1d
(LIE=0,2-4,10-12 VS=7)
  ESTAT: 00010000 [PIL]
(IS= ECode=1
EsubCode=0)
   BADV:
0000000000000040
   PRID: 0014c011
(Loongson-64bit,
Loongson-3C5000)
  Modules linked in:
tls xt_CHECKSUM
xt_MASQUERADE
xt_conntrack
ipt_REJECT
  Process test_sockmap
(pid: 23245,
threadinfo=00000000aeb6
8043,
task=...)
  Stack : ... ...
          ...
  Call Trace:
  [<900000000426cd1c>]
sk_msg_memcopy_from_ite
r+0xbc/0x220
  [<90000000043a315c>]
tcp_bpf_sendmsg+0x23c/0
x420
  [<90000000041cafc8>]
__sock_sendmsg+0x68/0xe
0
  [<90000000041cc4bc>]
____sys_sendmsg+0x2bc/0
x360
  [<90000000041cea18>]
___sys_sendmsg+0xb8/0x1
20
  [<90000000041cf1f8>]
__sys_sendmsg+0x98/0x10
0
  [<90000000045b76ec>]
do_syscall+0x8c/0xc0
  [<90000000030e1da4>]
handle_syscall+0xc4/0x1
60

  Code: ...

  ---[ end trace
0000000000000000 ]---
'''

This crash is because a
NULL pointer is passed
to
page_address()
in
sk_msg_memcopy_from_ite
r(). Due to the
difference in
architecture,
page_address(0) will
not trigger a panic on
the
X86
platform but
will panic
on the Loogarch
platform. So this bug
was
hidden on the x86
platform, but
now it is exposed on
the Loogarch platform.

This bug is a logic
error indeed. In
sk_msg_memcopy_from_ite
r(),
an
invalid
"sge" is always used:

 if (msg->sg.copybreak

quoted

= sge->length) {

 msg->sg.copybreak = 0;
 sk_msg_iter_var_next(i
);
 if (i == msg->sg.end)
 break;
 sge = sk_msg_elem(msg,
i);
 }

If the value of i is 2,
msg->sg.end is also 2
when entering
this
if
block.
sk_msg_iter_var_next()
increases i by 1, and
now i is 3,
which is
no longer
equal to msg->sg.end.
The break will not be
triggered, and
the
next
sge
obtained by
sk_msg_elem(3) will be
an invalid
one.

The correct approach is
to check (i ==
msg->sg.end) first,
and
then
invoke
sk_msg_iter_var_next()
if they are not equal.

Fixes: 604326b41a6f
("bpf, sockmap: convert
to
generic
sk_msg
interface")
Signed-off-by: Geliang
Tang
<
tanggeliang@kylinos.cn>
---
  net/core/skmsg.c | 2
+-
  1 file changed, 1
insertion(+), 1
deletion(-)

diff --git
a/net/core/skmsg.c
b/net/core/skmsg.c
index
44952cdd1425..1906d0d0e
eac 100644

--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c

@@ -378,9 +378,9 @@ int

sk_msg_memcopy_from_ite
r(struct
sock *sk,
struct iov_iter *from,
   /* This is possible
if a trim operation
shrunk the
buffer */
   if (msg-

quoted

sg.copybreak >= sge-
length) {

   msg->sg.copybreak =
0;
-
sk_msg_iter_var_next(i)
;
   if (i == msg-

quoted

sg.end)

   break;
+
sk_msg_iter_var_next(i)
;

Reviewed-by: D. Wythe
[off-list ref]

Thanks for your review.

But this change breaks test_sockmap. Will send a v4 to
fix
this.

Changes Requested.

-Geliang

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

   sge =
sk_msg_elem(msg, i);
   }