All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATHC bpf v5 1/3] skmsg: lose offset info in sk_psock_skb_ingress
@ 2021-10-12  6:57 Liu Jian
  2021-10-12  6:57 ` [PATHC bpf v5 2/3] selftests, bpf: Fix test_txmsg_ingress_parser error Liu Jian
  2021-10-12  6:57 ` [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with strparser Liu Jian
  0 siblings, 2 replies; 8+ messages in thread
From: Liu Jian @ 2021-10-12  6:57 UTC (permalink / raw)
  To: john.fastabend, daniel, jakub, lmb, davem, kuba, ast, andrii,
	kafai, songliubraving, yhs, kpsingh, netdev, bpf, xiyou.wangcong
  Cc: liujian56

If sockmap enable strparser, there are lose offset info in
sk_psock_skb_ingress. If the length determined by parse_msg function
is not skb->len, the skb will be converted to sk_msg multiple times,
and userspace app will get the data multiple times.

Fix this by get the offset and length from strp_msg.
And as Cong suggestion, add one bit in skb->_sk_redir to distinguish
enable or disable strparser.

Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
Signed-off-by: Liu Jian <liujian56@huawei.com>
Reviewed-by: Cong Wang <cong.wang@bytedance.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
---
v1->v2: fix build error when disable CONFIG_BPF_STREAM_PARSER
v2->v3: Add one bit in skb->_sk_redir to distinguish enable or disable strparser
v3->v4: Remove "#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)" code;
	and let "stm" have a more precise scope.
v4->v5: Add fix tag.
 include/linux/skmsg.h | 18 ++++++++++++++++--
 net/core/skmsg.c      | 40 ++++++++++++++++++++++++++++++----------
 2 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 14ab0c0bc924..94e2a1f6e58d 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -508,8 +508,22 @@ static inline bool sk_psock_strp_enabled(struct sk_psock *psock)
 
 #if IS_ENABLED(CONFIG_NET_SOCK_MSG)
 
-/* We only have one bit so far. */
-#define BPF_F_PTR_MASK ~(BPF_F_INGRESS)
+#define BPF_F_STRPARSER	(1UL << 1)
+
+/* We only have two bits so far. */
+#define BPF_F_PTR_MASK ~(BPF_F_INGRESS | BPF_F_STRPARSER)
+
+static inline bool skb_bpf_strparser(const struct sk_buff *skb)
+{
+	unsigned long sk_redir = skb->_sk_redir;
+
+	return sk_redir & BPF_F_STRPARSER;
+}
+
+static inline void skb_bpf_set_strparser(struct sk_buff *skb)
+{
+	skb->_sk_redir |= BPF_F_STRPARSER;
+}
 
 static inline bool skb_bpf_ingress(const struct sk_buff *skb)
 {
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 2d6249b28928..e85b7f8491b9 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -494,6 +494,7 @@ static struct sk_msg *sk_psock_create_ingress_msg(struct sock *sk,
 }
 
 static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
+					u32 off, u32 len,
 					struct sk_psock *psock,
 					struct sock *sk,
 					struct sk_msg *msg)
@@ -507,11 +508,11 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
 	 */
 	if (skb_linearize(skb))
 		return -EAGAIN;
-	num_sge = skb_to_sgvec(skb, msg->sg.data, 0, skb->len);
+	num_sge = skb_to_sgvec(skb, msg->sg.data, off, len);
 	if (unlikely(num_sge < 0))
 		return num_sge;
 
-	copied = skb->len;
+	copied = len;
 	msg->sg.start = 0;
 	msg->sg.size = copied;
 	msg->sg.end = num_sge;
@@ -522,9 +523,11 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
 	return copied;
 }
 
-static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb);
+static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,
+				     u32 off, u32 len);
 
-static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb)
+static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
+				u32 off, u32 len)
 {
 	struct sock *sk = psock->sk;
 	struct sk_msg *msg;
@@ -535,7 +538,7 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb)
 	 * correctly.
 	 */
 	if (unlikely(skb->sk == sk))
-		return sk_psock_skb_ingress_self(psock, skb);
+		return sk_psock_skb_ingress_self(psock, skb, off, len);
 	msg = sk_psock_create_ingress_msg(sk, skb);
 	if (!msg)
 		return -EAGAIN;
@@ -547,7 +550,7 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb)
 	 * into user buffers.
 	 */
 	skb_set_owner_r(skb, sk);
-	err = sk_psock_skb_ingress_enqueue(skb, psock, sk, msg);
+	err = sk_psock_skb_ingress_enqueue(skb, off, len, psock, sk, msg);
 	if (err < 0)
 		kfree(msg);
 	return err;
@@ -557,7 +560,8 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb)
  * skb. In this case we do not need to check memory limits or skb_set_owner_r
  * because the skb is already accounted for here.
  */
-static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb)
+static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,
+				     u32 off, u32 len)
 {
 	struct sk_msg *msg = kzalloc(sizeof(*msg), __GFP_NOWARN | GFP_ATOMIC);
 	struct sock *sk = psock->sk;
@@ -567,7 +571,7 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
 		return -EAGAIN;
 	sk_msg_init(msg);
 	skb_set_owner_r(skb, sk);
-	err = sk_psock_skb_ingress_enqueue(skb, psock, sk, msg);
+	err = sk_psock_skb_ingress_enqueue(skb, off, len, psock, sk, msg);
 	if (err < 0)
 		kfree(msg);
 	return err;
@@ -581,7 +585,7 @@ static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
 			return -EAGAIN;
 		return skb_send_sock(psock->sk, skb, off, len);
 	}
-	return sk_psock_skb_ingress(psock, skb);
+	return sk_psock_skb_ingress(psock, skb, off, len);
 }
 
 static void sk_psock_skb_state(struct sk_psock *psock,
@@ -624,6 +628,12 @@ static void sk_psock_backlog(struct work_struct *work)
 	while ((skb = skb_dequeue(&psock->ingress_skb))) {
 		len = skb->len;
 		off = 0;
+		if (skb_bpf_strparser(skb)) {
+			struct strp_msg *stm = strp_msg(skb);
+
+			off = stm->offset;
+			len = stm->full_len;
+		}
 start:
 		ingress = skb_bpf_ingress(skb);
 		skb_bpf_redirect_clear(skb);
@@ -930,6 +940,7 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
 {
 	struct sock *sk_other;
 	int err = 0;
+	u32 len, off;
 
 	switch (verdict) {
 	case __SK_PASS:
@@ -949,7 +960,15 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
 		 * retrying later from workqueue.
 		 */
 		if (skb_queue_empty(&psock->ingress_skb)) {
-			err = sk_psock_skb_ingress_self(psock, skb);
+			len = skb->len;
+			off = 0;
+			if (skb_bpf_strparser(skb)) {
+				struct strp_msg *stm = strp_msg(skb);
+
+				off = stm->offset;
+				len = stm->full_len;
+			}
+			err = sk_psock_skb_ingress_self(psock, skb, off, len);
 		}
 		if (err < 0) {
 			spin_lock_bh(&psock->ingress_lock);
@@ -1018,6 +1037,7 @@ static void sk_psock_strp_read(struct strparser *strp, struct sk_buff *skb)
 		ret = sk_psock_map_verd(ret, skb_bpf_redirect_fetch(skb));
 		skb->sk = NULL;
 	}
+	skb_bpf_set_strparser(skb);
 	sk_psock_verdict_apply(psock, skb, ret);
 out:
 	rcu_read_unlock();
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATHC bpf v5 2/3] selftests, bpf: Fix test_txmsg_ingress_parser error
  2021-10-12  6:57 [PATHC bpf v5 1/3] skmsg: lose offset info in sk_psock_skb_ingress Liu Jian
@ 2021-10-12  6:57 ` Liu Jian
  2021-10-22 15:22   ` John Fastabend
  2021-10-12  6:57 ` [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with strparser Liu Jian
  1 sibling, 1 reply; 8+ messages in thread
From: Liu Jian @ 2021-10-12  6:57 UTC (permalink / raw)
  To: john.fastabend, daniel, jakub, lmb, davem, kuba, ast, andrii,
	kafai, songliubraving, yhs, kpsingh, netdev, bpf, xiyou.wangcong
  Cc: liujian56

After "skmsg: lose offset info in sk_psock_skb_ingress", the test case
with ktls failed. This because ktls parser(tls_read_size) return value
is 285 not 256.

the case like this:
	tls_sk1 --> redir_sk --> tls_sk2
tls_sk1 sent out 512 bytes data, after tls related processing redir_sk
recved 570 btyes data, and redirect 512 (skb_use_parser) bytes data to
tls_sk2; but tls_sk2 needs 285 * 2 bytes data, receive timeout occurred.

Signed-off-by: Liu Jian <liujian56@huawei.com>
---
 tools/testing/selftests/bpf/test_sockmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index eefd445b96fc..06924917ad77 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -1680,6 +1680,8 @@ static void test_txmsg_ingress_parser(int cgrp, struct sockmap_options *opt)
 {
 	txmsg_pass = 1;
 	skb_use_parser = 512;
+	if (ktls == 1)
+		skb_use_parser = 570;
 	opt->iov_length = 256;
 	opt->iov_count = 1;
 	opt->rate = 2;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with strparser
  2021-10-12  6:57 [PATHC bpf v5 1/3] skmsg: lose offset info in sk_psock_skb_ingress Liu Jian
  2021-10-12  6:57 ` [PATHC bpf v5 2/3] selftests, bpf: Fix test_txmsg_ingress_parser error Liu Jian
@ 2021-10-12  6:57 ` Liu Jian
  2021-10-22 15:31   ` John Fastabend
  1 sibling, 1 reply; 8+ messages in thread
From: Liu Jian @ 2021-10-12  6:57 UTC (permalink / raw)
  To: john.fastabend, daniel, jakub, lmb, davem, kuba, ast, andrii,
	kafai, songliubraving, yhs, kpsingh, netdev, bpf, xiyou.wangcong
  Cc: liujian56

Add the test to check sockmap with strparser is working well.

Signed-off-by: Liu Jian <liujian56@huawei.com>
---
 tools/testing/selftests/bpf/test_sockmap.c | 33 ++++++++++++++++++++--
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 06924917ad77..1ba7e7346afb 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -139,6 +139,7 @@ struct sockmap_options {
 	bool sendpage;
 	bool data_test;
 	bool drop_expected;
+	bool check_recved_len;
 	int iov_count;
 	int iov_length;
 	int rate;
@@ -556,8 +557,12 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 	int err, i, flags = MSG_NOSIGNAL;
 	bool drop = opt->drop_expected;
 	bool data = opt->data_test;
+	int iov_alloc_length = iov_length;
 
-	err = msg_alloc_iov(&msg, iov_count, iov_length, data, tx);
+	if (!tx && opt->check_recved_len)
+		iov_alloc_length *= 2;
+
+	err = msg_alloc_iov(&msg, iov_count, iov_alloc_length, data, tx);
 	if (err)
 		goto out_errno;
 	if (peek_flag) {
@@ -665,6 +670,13 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 
 			s->bytes_recvd += recv;
 
+			if (opt->check_recved_len && s->bytes_recvd > total_bytes) {
+				errno = EMSGSIZE;
+				fprintf(stderr, "recv failed(), bytes_recvd:%zd, total_bytes:%f\n",
+						s->bytes_recvd, total_bytes);
+				goto out_errno;
+			}
+
 			if (data) {
 				int chunk_sz = opt->sendpage ?
 						iov_length * cnt :
@@ -744,7 +756,8 @@ static int sendmsg_test(struct sockmap_options *opt)
 
 	rxpid = fork();
 	if (rxpid == 0) {
-		iov_buf -= (txmsg_pop - txmsg_start_pop + 1);
+		if (txmsg_pop || txmsg_start_pop)
+			iov_buf -= (txmsg_pop - txmsg_start_pop + 1);
 		if (opt->drop_expected || txmsg_ktls_skb_drop)
 			_exit(0);
 
@@ -1688,6 +1701,19 @@ static void test_txmsg_ingress_parser(int cgrp, struct sockmap_options *opt)
 	test_exec(cgrp, opt);
 }
 
+static void test_txmsg_ingress_parser2(int cgrp, struct sockmap_options *opt)
+{
+	if (ktls == 1)
+		return;
+	skb_use_parser = 10;
+	opt->iov_length = 20;
+	opt->iov_count = 1;
+	opt->rate = 1;
+	opt->check_recved_len = true;
+	test_exec(cgrp, opt);
+	opt->check_recved_len = false;
+}
+
 char *map_names[] = {
 	"sock_map",
 	"sock_map_txmsg",
@@ -1786,7 +1812,8 @@ struct _test test[] = {
 	{"txmsg test pull-data", test_txmsg_pull},
 	{"txmsg test pop-data", test_txmsg_pop},
 	{"txmsg test push/pop data", test_txmsg_push_pop},
-	{"txmsg text ingress parser", test_txmsg_ingress_parser},
+	{"txmsg test ingress parser", test_txmsg_ingress_parser},
+	{"txmsg test ingress parser2", test_txmsg_ingress_parser2},
 };
 
 static int check_whitelist(struct _test *t, struct sockmap_options *opt)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* RE: [PATHC bpf v5 2/3] selftests, bpf: Fix test_txmsg_ingress_parser error
  2021-10-12  6:57 ` [PATHC bpf v5 2/3] selftests, bpf: Fix test_txmsg_ingress_parser error Liu Jian
@ 2021-10-22 15:22   ` John Fastabend
  2021-10-29  1:40     ` Alexei Starovoitov
  0 siblings, 1 reply; 8+ messages in thread
From: John Fastabend @ 2021-10-22 15:22 UTC (permalink / raw)
  To: Liu Jian, john.fastabend, daniel, jakub, lmb, davem, kuba, ast,
	andrii, kafai, songliubraving, yhs, kpsingh, netdev, bpf,
	xiyou.wangcong
  Cc: liujian56

Liu Jian wrote:
> After "skmsg: lose offset info in sk_psock_skb_ingress", the test case
> with ktls failed. This because ktls parser(tls_read_size) return value
> is 285 not 256.
> 
> the case like this:
> 	tls_sk1 --> redir_sk --> tls_sk2
> tls_sk1 sent out 512 bytes data, after tls related processing redir_sk
> recved 570 btyes data, and redirect 512 (skb_use_parser) bytes data to
> tls_sk2; but tls_sk2 needs 285 * 2 bytes data, receive timeout occurred.
> 
> Signed-off-by: Liu Jian <liujian56@huawei.com>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index eefd445b96fc..06924917ad77 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -1680,6 +1680,8 @@ static void test_txmsg_ingress_parser(int cgrp, struct sockmap_options *opt)
>  {
>  	txmsg_pass = 1;
>  	skb_use_parser = 512;
> +	if (ktls == 1)
> +		skb_use_parser = 570;
>  	opt->iov_length = 256;
>  	opt->iov_count = 1;
>  	opt->rate = 2;
> -- 
> 2.17.1
> 

Hi Liu LGTM sorry about the delay there I thought I acked this already, but
guess now.

Acked-by: John Fastabend <john.fastabend@gmail.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with strparser
  2021-10-12  6:57 ` [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with strparser Liu Jian
@ 2021-10-22 15:31   ` John Fastabend
  2021-10-25 10:17     ` liujian (CE)
  2021-10-26  8:20     ` liujian (CE)
  0 siblings, 2 replies; 8+ messages in thread
From: John Fastabend @ 2021-10-22 15:31 UTC (permalink / raw)
  To: Liu Jian, john.fastabend, daniel, jakub, lmb, davem, kuba, ast,
	andrii, kafai, songliubraving, yhs, kpsingh, netdev, bpf,
	xiyou.wangcong
  Cc: liujian56

Liu Jian wrote:
> Add the test to check sockmap with strparser is working well.
> 
> Signed-off-by: Liu Jian <liujian56@huawei.com>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 33 ++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 3 deletions(-)

Hi Liu,

This is a good test, but we should also add one with a parser returning
a value that is not skb->len. This doesn't cover the case fixed in
patch 1/3 correct? For that we would need to modify the BPF prog
itself as well sockmap_parse_prog.c.

For this patch though,

Acked-by: John Fastabend <john.fastabend@gmail.com>

Then one more patch is all we need something to break up the skb from
the parser. We really need the test because its not something we
can easily test otherwise and I don't have any use cases that do this
so wouldn't catch it.

Thanks!
John

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with strparser
  2021-10-22 15:31   ` John Fastabend
@ 2021-10-25 10:17     ` liujian (CE)
  2021-10-26  8:20     ` liujian (CE)
  1 sibling, 0 replies; 8+ messages in thread
From: liujian (CE) @ 2021-10-25 10:17 UTC (permalink / raw)
  To: John Fastabend, daniel, jakub, lmb, davem, kuba, ast, andrii,
	kafai, songliubraving, yhs, kpsingh, netdev, bpf, xiyou.wangcong



> -----Original Message-----
> From: John Fastabend [mailto:john.fastabend@gmail.com]
> Sent: Friday, October 22, 2021 11:31 PM
> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
> daniel@iogearbox.net; jakub@cloudflare.com; lmb@cloudflare.com;
> davem@davemloft.net; kuba@kernel.org; ast@kernel.org;
> andrii@kernel.org; kafai@fb.com; songliubraving@fb.com; yhs@fb.com;
> kpsingh@kernel.org; netdev@vger.kernel.org; bpf@vger.kernel.org;
> xiyou.wangcong@gmail.com
> Cc: liujian (CE) <liujian56@huawei.com>
> Subject: RE: [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with
> strparser
> 
> Liu Jian wrote:
> > Add the test to check sockmap with strparser is working well.
> >
> > Signed-off-by: Liu Jian <liujian56@huawei.com>
> > ---
> >  tools/testing/selftests/bpf/test_sockmap.c | 33
> > ++++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> Hi Liu,
> 
> This is a good test, but we should also add one with a parser returning a value
> that is not skb->len. This doesn't cover the case fixed in patch 1/3 correct?
> For that we would need to modify the BPF prog itself as well
> sockmap_parse_prog.c.
> 
Hi John,
This test patch use tools/testing/selftests/bpf/progs/test_sockmap_kern.c not sockmap_parse_prog.c.

If we set skb_use_parser to nonzero, the bpf parser program will return skb_use_parser not skb->len.
In this test case, I set skb_use_parser to 10, skb->len to 20 (opt->iov_length). 
This can test 1/3 patch, it will check the recved data len is 10 not 20.

The parser prog in tools/testing/selftests/bpf/progs/test_sockmap_kern.h
SEC("sk_skb1")
int bpf_prog1(struct __sk_buff *skb)
{
        int *f, two = 2;

        f = bpf_map_lookup_elem(&sock_skb_opts, &two);
        if (f && *f) {
                return *f;
        }
        return skb->len;
}
> For this patch though,
> 
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> 
> Then one more patch is all we need something to break up the skb from the
> parser. We really need the test because its not something we can easily test
> otherwise and I don't have any use cases that do this so wouldn't catch it.
> 
> Thanks!
> John

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with strparser
  2021-10-22 15:31   ` John Fastabend
  2021-10-25 10:17     ` liujian (CE)
@ 2021-10-26  8:20     ` liujian (CE)
  1 sibling, 0 replies; 8+ messages in thread
From: liujian (CE) @ 2021-10-26  8:20 UTC (permalink / raw)
  To: John Fastabend, daniel, jakub, lmb, davem, kuba, ast, andrii,
	kafai, songliubraving, yhs, kpsingh, netdev, bpf, xiyou.wangcong



> -----Original Message-----
> From: liujian (CE)
> Sent: Monday, October 25, 2021 6:17 PM
> To: 'John Fastabend' <john.fastabend@gmail.com>; daniel@iogearbox.net;
> jakub@cloudflare.com; lmb@cloudflare.com; davem@davemloft.net;
> kuba@kernel.org; ast@kernel.org; andrii@kernel.org; kafai@fb.com;
> songliubraving@fb.com; yhs@fb.com; kpsingh@kernel.org;
> netdev@vger.kernel.org; bpf@vger.kernel.org; xiyou.wangcong@gmail.com
> Subject: RE: [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with
> strparser
> 
> 
> 
> > -----Original Message-----
> > From: John Fastabend [mailto:john.fastabend@gmail.com]
> > Sent: Friday, October 22, 2021 11:31 PM
> > To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
> > daniel@iogearbox.net; jakub@cloudflare.com; lmb@cloudflare.com;
> > davem@davemloft.net; kuba@kernel.org; ast@kernel.org;
> > andrii@kernel.org; kafai@fb.com; songliubraving@fb.com; yhs@fb.com;
> > kpsingh@kernel.org; netdev@vger.kernel.org; bpf@vger.kernel.org;
> > xiyou.wangcong@gmail.com
> > Cc: liujian (CE) <liujian56@huawei.com>
> > Subject: RE: [PATHC bpf v5 3/3] selftests, bpf: Add one test for
> > sockmap with strparser
> >
> > Liu Jian wrote:
> > > Add the test to check sockmap with strparser is working well.
> > >
> > > Signed-off-by: Liu Jian <liujian56@huawei.com>
> > > ---
> > >  tools/testing/selftests/bpf/test_sockmap.c | 33
> > > ++++++++++++++++++++--
> > >  1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > Hi Liu,
> >
> > This is a good test, but we should also add one with a parser
> > returning a value that is not skb->len. This doesn't cover the case fixed in
> patch 1/3 correct?
> > For that we would need to modify the BPF prog itself as well
> > sockmap_parse_prog.c.
> >
> Hi John,
> This test patch use tools/testing/selftests/bpf/progs/test_sockmap_kern.c
> not sockmap_parse_prog.c.
> 
> If we set skb_use_parser to nonzero, the bpf parser program will return
> skb_use_parser not skb->len.
> In this test case, I set skb_use_parser to 10, skb->len to 20 (opt->iov_length).
> This can test 1/3 patch, it will check the recved data len is 10 not 20.
> 
Sorry, it will check the recved data length is 20 not 40.

> The parser prog in tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> SEC("sk_skb1")
> int bpf_prog1(struct __sk_buff *skb)
> {
>         int *f, two = 2;
> 
>         f = bpf_map_lookup_elem(&sock_skb_opts, &two);
>         if (f && *f) {
>                 return *f;
>         }
>         return skb->len;
> }
> > For this patch though,
> >
> > Acked-by: John Fastabend <john.fastabend@gmail.com>
> >
> > Then one more patch is all we need something to break up the skb from
> > the parser. We really need the test because its not something we can
> > easily test otherwise and I don't have any use cases that do this so wouldn't
> catch it.
> >
> > Thanks!
> > John

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATHC bpf v5 2/3] selftests, bpf: Fix test_txmsg_ingress_parser error
  2021-10-22 15:22   ` John Fastabend
@ 2021-10-29  1:40     ` Alexei Starovoitov
  0 siblings, 0 replies; 8+ messages in thread
From: Alexei Starovoitov @ 2021-10-29  1:40 UTC (permalink / raw)
  To: John Fastabend
  Cc: Liu Jian, Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	KP Singh, Network Development, bpf, Cong Wang

On Fri, Oct 22, 2021 at 8:22 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Liu Jian wrote:
> > After "skmsg: lose offset info in sk_psock_skb_ingress", the test case
> > with ktls failed. This because ktls parser(tls_read_size) return value
> > is 285 not 256.
> >
> > the case like this:
> >       tls_sk1 --> redir_sk --> tls_sk2
> > tls_sk1 sent out 512 bytes data, after tls related processing redir_sk
> > recved 570 btyes data, and redirect 512 (skb_use_parser) bytes data to
> > tls_sk2; but tls_sk2 needs 285 * 2 bytes data, receive timeout occurred.
> >
> > Signed-off-by: Liu Jian <liujian56@huawei.com>
> > ---
> >  tools/testing/selftests/bpf/test_sockmap.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> > index eefd445b96fc..06924917ad77 100644
> > --- a/tools/testing/selftests/bpf/test_sockmap.c
> > +++ b/tools/testing/selftests/bpf/test_sockmap.c
> > @@ -1680,6 +1680,8 @@ static void test_txmsg_ingress_parser(int cgrp, struct sockmap_options *opt)
> >  {
> >       txmsg_pass = 1;
> >       skb_use_parser = 512;
> > +     if (ktls == 1)
> > +             skb_use_parser = 570;
> >       opt->iov_length = 256;
> >       opt->iov_count = 1;
> >       opt->rate = 2;
> > --
> > 2.17.1
> >
>
> Hi Liu LGTM sorry about the delay there I thought I acked this already, but
> guess now.
>
> Acked-by: John Fastabend <john.fastabend@gmail.com>

Hmm.
patch 1 is causing a crash.

./test_progs -t sockmap
#124 sockmap_basic:OK
#125 sockmap_ktls:OK
[   15.391661] ==================================================================
[   15.392635] BUG: KASAN: null-ptr-deref in dst_release+0x1d/0x80
[   15.393337] Write of size 4 at addr 0000000000000042 by task test_progs/1358
[   15.394144]
[   15.394326] CPU: 3 PID: 1358 Comm: test_progs Tainted: G
O      5.15.0-rc3-01147-ge4bcff4e3384 #3617
[   15.395415] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
[   15.396653] Call Trace:
[   15.396929]  <IRQ>
[   15.397163]  dump_stack_lvl+0x44/0x57
[   15.397569]  ? dst_release+0x1d/0x80
[   15.397970]  kasan_report.cold.15+0x66/0xdf
[   15.398430]  ? dst_release+0x1d/0x80
[   15.398824]  ? sk_psock_verdict_apply+0x149/0x460
[   15.399341]  kasan_check_range+0x1c1/0x1e0
[   15.399789]  ? sk_psock_verdict_apply+0x149/0x460
[   15.400308]  dst_release+0x1d/0x80
[   15.400679]  skb_release_head_state+0x100/0x170
[   15.401178]  skb_release_all+0xe/0x50
[   15.401580]  kfree_skb+0xa1/0x230
[   15.401957]  sk_psock_verdict_apply+0x149/0x460
[   15.402450]  ? bpf_sk_redirect_map+0x2b/0x1a0
[   15.402974]  sk_psock_strp_read+0x239/0x550
[   15.403452]  __strp_recv+0x4a7/0x1b70
[   15.403917]  tcp_read_sock+0x1d2/0x760

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-10-29  1:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-12  6:57 [PATHC bpf v5 1/3] skmsg: lose offset info in sk_psock_skb_ingress Liu Jian
2021-10-12  6:57 ` [PATHC bpf v5 2/3] selftests, bpf: Fix test_txmsg_ingress_parser error Liu Jian
2021-10-22 15:22   ` John Fastabend
2021-10-29  1:40     ` Alexei Starovoitov
2021-10-12  6:57 ` [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with strparser Liu Jian
2021-10-22 15:31   ` John Fastabend
2021-10-25 10:17     ` liujian (CE)
2021-10-26  8:20     ` liujian (CE)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.