[PATCH bpf-next] selftests/bpf: Add F

public inbox for linux-kselftest@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH bpf-next] selftests/bpf: Add F_SETFL for fcntl
@ 2024-04-03  8:32 Geliang Tang
  2024-04-03 12:05 ` Jakub Sitnicki
  0 siblings, 1 reply; 6+ messages in thread
From: Geliang Tang @ 2024-04-03  8:32 UTC (permalink / raw)
  To: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, Jiri Olsa, Shuah Khan
  Cc: Geliang Tang, bpf, mptcp, linux-kselftest

From: Geliang Tang <tanggeliang@kylinos.cn>

Incorrect arguments are passed to fcntl() in test_sockmap.c when invoking
it to set file status flags. If O_NONBLOCK is used as 2nd argument and
passed into fcntl, -EINVAL will be returned (See do_fcntl() in fs/fcntl.c).
The correct approach is to use F_SETFL as 2nd argument, and O_NONBLOCK as
3rd one.

Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/test_sockmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 024a0faafb3b..34d6a1e6f664 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -603,7 +603,7 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 		struct timeval timeout;
 		fd_set w;
 
-		fcntl(fd, fd_flags);
+		fcntl(fd, F_SETFL, fd_flags);
 		/* Account for pop bytes noting each iteration of apply will
 		 * call msg_pop_data helper so we need to account for this
 		 * by calculating the number of apply iterations. Note user
-- 
2.40.1


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

* Re: [PATCH bpf-next] selftests/bpf: Add F_SETFL for fcntl
  2024-04-03  8:32 [PATCH bpf-next] selftests/bpf: Add F_SETFL for fcntl Geliang Tang
@ 2024-04-03 12:05 ` Jakub Sitnicki
  2024-04-03 22:29   ` John Fastabend
  0 siblings, 1 reply; 6+ messages in thread
From: Jakub Sitnicki @ 2024-04-03 12:05 UTC (permalink / raw)
  To: Geliang Tang
  Cc: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, Jiri Olsa, Shuah Khan, Geliang Tang, bpf, mptcp,
	linux-kselftest

Hi Geliang,

On Wed, Apr 03, 2024 at 04:32 PM +08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Incorrect arguments are passed to fcntl() in test_sockmap.c when invoking
> it to set file status flags. If O_NONBLOCK is used as 2nd argument and
> passed into fcntl, -EINVAL will be returned (See do_fcntl() in fs/fcntl.c).
> The correct approach is to use F_SETFL as 2nd argument, and O_NONBLOCK as
> 3rd one.
>
> Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index 024a0faafb3b..34d6a1e6f664 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -603,7 +603,7 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
>  		struct timeval timeout;
>  		fd_set w;
>  
> -		fcntl(fd, fd_flags);
> +		fcntl(fd, F_SETFL, fd_flags);
>  		/* Account for pop bytes noting each iteration of apply will
>  		 * call msg_pop_data helper so we need to account for this
>  		 * by calculating the number of apply iterations. Note user

Good catch. But we also need to figure out why some tests failing with
this patch applied and fix them in one go:

# 6/ 7  sockmap::txmsg test skb:FAIL
#21/ 7 sockhash::txmsg test skb:FAIL
#36/ 7 sockhash:ktls:txmsg test skb:FAIL
Pass: 42 Fail: 3

I'm seeing this error message when running `test_sockmap`:

detected skb data error with skb ingress update @iov[0]:0 "00 00 00 00" != "PASS"
data verify msg failed: Unknown error -5
rx thread exited with err 1.

I'd also:
- add an error check for fnctl, so we don't regress,
- get rid of fd_flags, pass O_NONBLOCK flag directly to fnctl.

Thanks,
-jkbs

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

* Re: [PATCH bpf-next] selftests/bpf: Add F_SETFL for fcntl
  2024-04-03 12:05 ` Jakub Sitnicki
@ 2024-04-03 22:29   ` John Fastabend
  2024-04-04  0:27     ` Geliang Tang
  0 siblings, 1 reply; 6+ messages in thread
From: John Fastabend @ 2024-04-03 22:29 UTC (permalink / raw)
  To: Jakub Sitnicki, Geliang Tang
  Cc: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, Jiri Olsa, Shuah Khan, Geliang Tang, bpf, mptcp,
	linux-kselftest

Jakub Sitnicki wrote:
> Hi Geliang,
> 
> On Wed, Apr 03, 2024 at 04:32 PM +08, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> >
> > Incorrect arguments are passed to fcntl() in test_sockmap.c when invoking
> > it to set file status flags. If O_NONBLOCK is used as 2nd argument and
> > passed into fcntl, -EINVAL will be returned (See do_fcntl() in fs/fcntl.c).
> > The correct approach is to use F_SETFL as 2nd argument, and O_NONBLOCK as
> > 3rd one.
> >
> > Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  tools/testing/selftests/bpf/test_sockmap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> > index 024a0faafb3b..34d6a1e6f664 100644
> > --- a/tools/testing/selftests/bpf/test_sockmap.c
> > +++ b/tools/testing/selftests/bpf/test_sockmap.c
> > @@ -603,7 +603,7 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
> >  		struct timeval timeout;
> >  		fd_set w;
> >  
> > -		fcntl(fd, fd_flags);
> > +		fcntl(fd, F_SETFL, fd_flags);
> >  		/* Account for pop bytes noting each iteration of apply will
> >  		 * call msg_pop_data helper so we need to account for this
> >  		 * by calculating the number of apply iterations. Note user
> 
> Good catch. But we also need to figure out why some tests failing with
> this patch applied and fix them in one go:
> 
> # 6/ 7  sockmap::txmsg test skb:FAIL
> #21/ 7 sockhash::txmsg test skb:FAIL
> #36/ 7 sockhash:ktls:txmsg test skb:FAIL
> Pass: 42 Fail: 3
> 
> I'm seeing this error message when running `test_sockmap`:
> 
> detected skb data error with skb ingress update @iov[0]:0 "00 00 00 00" != "PASS"
> data verify msg failed: Unknown error -5
> rx thread exited with err 1.

I have a theory this is a real bug in the SK_SKB_STREAM_PARSER which has an
issue with wakup logic. Maybe we wakeup the poll/select logic before the
data is copied and because the recv() is actually nonblocking now we get
the error.

> 
> I'd also:
> - add an error check for fnctl, so we don't regress,
> - get rid of fd_flags, pass O_NONBLOCK flag directly to fnctl.
> 
> Thanks,
> -jkbs



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

* Re: [PATCH bpf-next] selftests/bpf: Add F_SETFL for fcntl
  2024-04-03 22:29   ` John Fastabend
@ 2024-04-04  0:27     ` Geliang Tang
  2024-04-05  4:47       ` John Fastabend
  0 siblings, 1 reply; 6+ messages in thread
From: Geliang Tang @ 2024-04-04  0:27 UTC (permalink / raw)
  To: Jakub Sitnicki, John Fastabend
  Cc: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, bpf, mptcp, linux-kselftest

Hi Jakub,
 
On Wed, 2024-04-03 at 15:29 -0700, John Fastabend wrote:
> Jakub Sitnicki wrote:
> > Hi Geliang,
> > 
> > On Wed, Apr 03, 2024 at 04:32 PM +08, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > Incorrect arguments are passed to fcntl() in test_sockmap.c when
> > > invoking
> > > it to set file status flags. If O_NONBLOCK is used as 2nd
> > > argument and
> > > passed into fcntl, -EINVAL will be returned (See do_fcntl() in
> > > fs/fcntl.c).
> > > The correct approach is to use F_SETFL as 2nd argument, and
> > > O_NONBLOCK as
> > > 3rd one.
> > > 
> > > Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > ---
> > >  tools/testing/selftests/bpf/test_sockmap.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/testing/selftests/bpf/test_sockmap.c
> > > b/tools/testing/selftests/bpf/test_sockmap.c
> > > index 024a0faafb3b..34d6a1e6f664 100644
> > > --- a/tools/testing/selftests/bpf/test_sockmap.c
> > > +++ b/tools/testing/selftests/bpf/test_sockmap.c
> > > @@ -603,7 +603,7 @@ static int msg_loop(int fd, int iov_count,
> > > int iov_length, int cnt,
> > >  		struct timeval timeout;
> > >  		fd_set w;
> > >  
> > > -		fcntl(fd, fd_flags);
> > > +		fcntl(fd, F_SETFL, fd_flags);
> > >  		/* Account for pop bytes noting each iteration
> > > of apply will
> > >  		 * call msg_pop_data helper so we need to
> > > account for this
> > >  		 * by calculating the number of apply
> > > iterations. Note user
> > 
> > Good catch. But we also need to figure out why some tests failing
> > with
> > this patch applied and fix them in one go:
> > 
> > # 6/ 7  sockmap::txmsg test skb:FAIL
> > #21/ 7 sockhash::txmsg test skb:FAIL
> > #36/ 7 sockhash:ktls:txmsg test skb:FAIL
> > Pass: 42 Fail: 3

Sorry, I didn't notice these fails in my testing before, they do exist.
I'll try to fix them and sent a v2 soon.

Thanks,
-Geliang

> > 
> > I'm seeing this error message when running `test_sockmap`:
> > 
> > detected skb data error with skb ingress update @iov[0]:0 "00 00 00
> > 00" != "PASS"
> > data verify msg failed: Unknown error -5
> > rx thread exited with err 1.
> 
> I have a theory this is a real bug in the SK_SKB_STREAM_PARSER which
> has an
> issue with wakup logic. Maybe we wakeup the poll/select logic before
> the
> data is copied and because the recv() is actually nonblocking now we
> get
> the error.
> 
> > 
> > I'd also:
> > - add an error check for fnctl, so we don't regress,
> > - get rid of fd_flags, pass O_NONBLOCK flag directly to fnctl.
> > 
> > Thanks,
> > -jkbs
> 
> 


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

* Re: [PATCH bpf-next] selftests/bpf: Add F_SETFL for fcntl
  2024-04-04  0:27     ` Geliang Tang
@ 2024-04-05  4:47       ` John Fastabend
  2024-04-07  8:29         ` Geliang Tang
  0 siblings, 1 reply; 6+ messages in thread
From: John Fastabend @ 2024-04-05  4:47 UTC (permalink / raw)
  To: Geliang Tang, Jakub Sitnicki, John Fastabend
  Cc: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, bpf, mptcp, linux-kselftest

Geliang Tang wrote:
> Hi Jakub,
>  
> On Wed, 2024-04-03 at 15:29 -0700, John Fastabend wrote:
> > Jakub Sitnicki wrote:
> > > Hi Geliang,
> > > 
> > > On Wed, Apr 03, 2024 at 04:32 PM +08, Geliang Tang wrote:
> > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > 
> > > > Incorrect arguments are passed to fcntl() in test_sockmap.c when
> > > > invoking
> > > > it to set file status flags. If O_NONBLOCK is used as 2nd
> > > > argument and
> > > > passed into fcntl, -EINVAL will be returned (See do_fcntl() in
> > > > fs/fcntl.c).
> > > > The correct approach is to use F_SETFL as 2nd argument, and
> > > > O_NONBLOCK as
> > > > 3rd one.
> > > > 
> > > > Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> > > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > > ---
> > > >  tools/testing/selftests/bpf/test_sockmap.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/bpf/test_sockmap.c
> > > > b/tools/testing/selftests/bpf/test_sockmap.c
> > > > index 024a0faafb3b..34d6a1e6f664 100644
> > > > --- a/tools/testing/selftests/bpf/test_sockmap.c
> > > > +++ b/tools/testing/selftests/bpf/test_sockmap.c
> > > > @@ -603,7 +603,7 @@ static int msg_loop(int fd, int iov_count,
> > > > int iov_length, int cnt,
> > > >  		struct timeval timeout;
> > > >  		fd_set w;
> > > >  
> > > > -		fcntl(fd, fd_flags);
> > > > +		fcntl(fd, F_SETFL, fd_flags);
> > > >  		/* Account for pop bytes noting each iteration
> > > > of apply will
> > > >  		 * call msg_pop_data helper so we need to
> > > > account for this
> > > >  		 * by calculating the number of apply
> > > > iterations. Note user
> > > 
> > > Good catch. But we also need to figure out why some tests failing
> > > with
> > > this patch applied and fix them in one go:
> > > 
> > > # 6/ 7  sockmap::txmsg test skb:FAIL
> > > #21/ 7 sockhash::txmsg test skb:FAIL
> > > #36/ 7 sockhash:ktls:txmsg test skb:FAIL
> > > Pass: 42 Fail: 3
> 
> Sorry, I didn't notice these fails in my testing before, they do exist.
> I'll try to fix them and sent a v2 soon.

Not 100% sure but a hint. I would look into where the stream parser
is waking up the sock here. I'm not sure there is an easy fix tbh.
I can help look at this soon as well if you don't beat me to a fix.

Thanks for looking into it.

> 
> Thanks,
> -Geliang

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

* Re: [PATCH bpf-next] selftests/bpf: Add F_SETFL for fcntl
  2024-04-05  4:47       ` John Fastabend
@ 2024-04-07  8:29         ` Geliang Tang
  0 siblings, 0 replies; 6+ messages in thread
From: Geliang Tang @ 2024-04-07  8:29 UTC (permalink / raw)
  To: John Fastabend, Jakub Sitnicki
  Cc: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, bpf, mptcp, linux-kselftest

On Thu, 2024-04-04 at 21:47 -0700, John Fastabend wrote:
> Geliang Tang wrote:
> > Hi Jakub,
> >  
> > On Wed, 2024-04-03 at 15:29 -0700, John Fastabend wrote:
> > > Jakub Sitnicki wrote:
> > > > Hi Geliang,
> > > > 
> > > > On Wed, Apr 03, 2024 at 04:32 PM +08, Geliang Tang wrote:
> > > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > > 
> > > > > Incorrect arguments are passed to fcntl() in test_sockmap.c
> > > > > when
> > > > > invoking
> > > > > it to set file status flags. If O_NONBLOCK is used as 2nd
> > > > > argument and
> > > > > passed into fcntl, -EINVAL will be returned (See do_fcntl()
> > > > > in
> > > > > fs/fcntl.c).
> > > > > The correct approach is to use F_SETFL as 2nd argument, and
> > > > > O_NONBLOCK as
> > > > > 3rd one.
> > > > > 
> > > > > Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> > > > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > > > ---
> > > > >  tools/testing/selftests/bpf/test_sockmap.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/bpf/test_sockmap.c
> > > > > b/tools/testing/selftests/bpf/test_sockmap.c
> > > > > index 024a0faafb3b..34d6a1e6f664 100644
> > > > > --- a/tools/testing/selftests/bpf/test_sockmap.c
> > > > > +++ b/tools/testing/selftests/bpf/test_sockmap.c
> > > > > @@ -603,7 +603,7 @@ static int msg_loop(int fd, int
> > > > > iov_count,
> > > > > int iov_length, int cnt,
> > > > >  		struct timeval timeout;
> > > > >  		fd_set w;
> > > > >  
> > > > > -		fcntl(fd, fd_flags);
> > > > > +		fcntl(fd, F_SETFL, fd_flags);
> > > > >  		/* Account for pop bytes noting each
> > > > > iteration
> > > > > of apply will
> > > > >  		 * call msg_pop_data helper so we need to
> > > > > account for this
> > > > >  		 * by calculating the number of apply
> > > > > iterations. Note user
> > > > 
> > > > Good catch. But we also need to figure out why some tests
> > > > failing
> > > > with
> > > > this patch applied and fix them in one go:
> > > > 
> > > > # 6/ 7  sockmap::txmsg test skb:FAIL
> > > > #21/ 7 sockhash::txmsg test skb:FAIL
> > > > #36/ 7 sockhash:ktls:txmsg test skb:FAIL
> > > > Pass: 42 Fail: 3
> > 
> > Sorry, I didn't notice these fails in my testing before, they do
> > exist.
> > I'll try to fix them and sent a v2 soon.
> 
> Not 100% sure but a hint. I would look into where the stream parser
> is waking up the sock here. I'm not sure there is an easy fix tbh.
> I can help look at this soon as well if you don't beat me to a fix.
> 
> Thanks for looking into it.

I just sent a v2, please review it for me.

Thanks,
-Geliang

> 
> > 
> > Thanks,
> > -Geliang


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

end of thread, other threads:[~2024-04-07  8:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-03  8:32 [PATCH bpf-next] selftests/bpf: Add F_SETFL for fcntl Geliang Tang
2024-04-03 12:05 ` Jakub Sitnicki
2024-04-03 22:29   ` John Fastabend
2024-04-04  0:27     ` Geliang Tang
2024-04-05  4:47       ` John Fastabend
2024-04-07  8:29         ` Geliang Tang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox