[LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Cyril Hrubis]

All the lapi/ functions does call tst_syscall() that does
tst_brk(TCONF, ...) on ENOSYS which exits the testrun prematurely on older
distributions.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
Reported-by: Martin Doucha <mdoucha@suse.cz>
---
 lib/tst_fd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/tst_fd.c b/lib/tst_fd.c
index ea84e1c85..ab7de81aa 100644
--- a/lib/tst_fd.c
+++ b/lib/tst_fd.c
@@ -139,7 +139,7 @@ static void open_timerfd(struct tst_fd *fd)
 
 static void open_pidfd(struct tst_fd *fd)
 {
-	fd->fd = pidfd_open(getpid(), 0);
+	fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
 	if (fd->fd < 0)
 		tst_brk(TBROK | TERRNO, "pidfd_open()");
 }
@@ -194,7 +194,7 @@ static void open_io_uring(struct tst_fd *fd)
 {
 	struct io_uring_params uring_params = {};
 
-	fd->fd = io_uring_setup(1, &uring_params);
+	fd->fd = syscall(__NR_io_uring_setup, 1, &uring_params);
 	if (fd->fd < 0) {
 		tst_res(TCONF | TERRNO,
 			"Skipping %s", tst_fd_desc(fd));
@@ -210,7 +210,7 @@ static void open_bpf_map(struct tst_fd *fd)
 		.max_entries = 1,
 	};
 
-	fd->fd = bpf(BPF_MAP_CREATE, &array_attr, sizeof(array_attr));
+	fd->fd = syscall(__NR_bpf, BPF_MAP_CREATE, &array_attr, sizeof(array_attr));
 	if (fd->fd < 0) {
 		tst_res(TCONF | TERRNO,
 			"Skipping %s", tst_fd_desc(fd));
@@ -219,7 +219,7 @@ static void open_bpf_map(struct tst_fd *fd)
 
 static void open_fsopen(struct tst_fd *fd)
 {
-	fd->fd = fsopen("ext2", 0);
+	fd->fd = syscall(__NR_fsopen, "ext2", 0);
 	if (fd->fd < 0) {
 		tst_res(TCONF | TERRNO,
 			"Skipping %s", tst_fd_desc(fd));
@@ -228,7 +228,7 @@ static void open_fsopen(struct tst_fd *fd)
 
 static void open_fspick(struct tst_fd *fd)
 {
-	fd->fd = fspick(AT_FDCWD, "/", 0);
+	fd->fd = syscall(__NR_fspick, AT_FDCWD, "/", 0);
 	if (fd->fd < 0) {
 		tst_res(TCONF | TERRNO,
 			"Skipping %s", tst_fd_desc(fd));
@@ -237,7 +237,7 @@ static void open_fspick(struct tst_fd *fd)
 
 static void open_open_tree(struct tst_fd *fd)
 {
-	fd->fd = open_tree(AT_FDCWD, "/", 0);
+	fd->fd = syscall(__NR_open_tree, AT_FDCWD, "/", 0);
 	if (fd->fd < 0) {
 		tst_res(TCONF | TERRNO,
 			"Skipping %s", tst_fd_desc(fd));
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Martin Doucha]

Hi,
Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 24. 01. 24 13:58, Cyril Hrubis wrote:
> All the lapi/ functions does call tst_syscall() that does
> tst_brk(TCONF, ...) on ENOSYS which exits the testrun prematurely on older
> distributions.
> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> Reported-by: Martin Doucha <mdoucha@suse.cz>
> ---
>   lib/tst_fd.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/tst_fd.c b/lib/tst_fd.c
> index ea84e1c85..ab7de81aa 100644
> --- a/lib/tst_fd.c
> +++ b/lib/tst_fd.c
> @@ -139,7 +139,7 @@ static void open_timerfd(struct tst_fd *fd)
>   
>   static void open_pidfd(struct tst_fd *fd)
>   {
> -	fd->fd = pidfd_open(getpid(), 0);
> +	fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
>   	if (fd->fd < 0)
>   		tst_brk(TBROK | TERRNO, "pidfd_open()");
>   }
> @@ -194,7 +194,7 @@ static void open_io_uring(struct tst_fd *fd)
>   {
>   	struct io_uring_params uring_params = {};
>   
> -	fd->fd = io_uring_setup(1, &uring_params);
> +	fd->fd = syscall(__NR_io_uring_setup, 1, &uring_params);
>   	if (fd->fd < 0) {
>   		tst_res(TCONF | TERRNO,
>   			"Skipping %s", tst_fd_desc(fd));
> @@ -210,7 +210,7 @@ static void open_bpf_map(struct tst_fd *fd)
>   		.max_entries = 1,
>   	};
>   
> -	fd->fd = bpf(BPF_MAP_CREATE, &array_attr, sizeof(array_attr));
> +	fd->fd = syscall(__NR_bpf, BPF_MAP_CREATE, &array_attr, sizeof(array_attr));
>   	if (fd->fd < 0) {
>   		tst_res(TCONF | TERRNO,
>   			"Skipping %s", tst_fd_desc(fd));
> @@ -219,7 +219,7 @@ static void open_bpf_map(struct tst_fd *fd)
>   
>   static void open_fsopen(struct tst_fd *fd)
>   {
> -	fd->fd = fsopen("ext2", 0);
> +	fd->fd = syscall(__NR_fsopen, "ext2", 0);
>   	if (fd->fd < 0) {
>   		tst_res(TCONF | TERRNO,
>   			"Skipping %s", tst_fd_desc(fd));
> @@ -228,7 +228,7 @@ static void open_fsopen(struct tst_fd *fd)
>   
>   static void open_fspick(struct tst_fd *fd)
>   {
> -	fd->fd = fspick(AT_FDCWD, "/", 0);
> +	fd->fd = syscall(__NR_fspick, AT_FDCWD, "/", 0);
>   	if (fd->fd < 0) {
>   		tst_res(TCONF | TERRNO,
>   			"Skipping %s", tst_fd_desc(fd));
> @@ -237,7 +237,7 @@ static void open_fspick(struct tst_fd *fd)
>   
>   static void open_open_tree(struct tst_fd *fd)
>   {
> -	fd->fd = open_tree(AT_FDCWD, "/", 0);
> +	fd->fd = syscall(__NR_open_tree, AT_FDCWD, "/", 0);
>   	if (fd->fd < 0) {
>   		tst_res(TCONF | TERRNO,
>   			"Skipping %s", tst_fd_desc(fd));

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Petr Vorel]

Hi Cyril,

> All the lapi/ functions does call tst_syscall() that does
> tst_brk(TCONF, ...) on ENOSYS which exits the testrun prematurely on older
> distributions.

> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> Reported-by: Martin Doucha <mdoucha@suse.cz>

Good catch, thanks!

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Petr Vorel]

> All the lapi/ functions does call tst_syscall() that does
> tst_brk(TCONF, ...) on ENOSYS which exits the testrun prematurely on older
> distributions.

> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> Reported-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  lib/tst_fd.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

> diff --git a/lib/tst_fd.c b/lib/tst_fd.c
> index ea84e1c85..ab7de81aa 100644
> --- a/lib/tst_fd.c
> +++ b/lib/tst_fd.c
> @@ -139,7 +139,7 @@ static void open_timerfd(struct tst_fd *fd)

>  static void open_pidfd(struct tst_fd *fd)
>  {
> -	fd->fd = pidfd_open(getpid(), 0);
> +	fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
Actually at least here tst_syscall() needs to be called or it fails on older
distros due missing ENOSYS check in raw syscall():

tst_fd.c:144: TBROK: pidfd_open(): ENOSYS (38)

Other syscall() are probably skipped due tst_brk(), but I would replace them
anyway.

Also, I wonder if we can avoid tst_brk() because it quits the test.

Kind regards,
Petr

>  	if (fd->fd < 0)
>  		tst_brk(TBROK | TERRNO, "pidfd_open()");
>  }
> @@ -194,7 +194,7 @@ static void open_io_uring(struct tst_fd *fd)
>  {
>  	struct io_uring_params uring_params = {};

> -	fd->fd = io_uring_setup(1, &uring_params);
> +	fd->fd = syscall(__NR_io_uring_setup, 1, &uring_params);
>  	if (fd->fd < 0) {
>  		tst_res(TCONF | TERRNO,
>  			"Skipping %s", tst_fd_desc(fd));
> @@ -210,7 +210,7 @@ static void open_bpf_map(struct tst_fd *fd)
>  		.max_entries = 1,
>  	};

> -	fd->fd = bpf(BPF_MAP_CREATE, &array_attr, sizeof(array_attr));
> +	fd->fd = syscall(__NR_bpf, BPF_MAP_CREATE, &array_attr, sizeof(array_attr));
>  	if (fd->fd < 0) {
>  		tst_res(TCONF | TERRNO,
>  			"Skipping %s", tst_fd_desc(fd));
> @@ -219,7 +219,7 @@ static void open_bpf_map(struct tst_fd *fd)

>  static void open_fsopen(struct tst_fd *fd)
>  {
> -	fd->fd = fsopen("ext2", 0);
> +	fd->fd = syscall(__NR_fsopen, "ext2", 0);
>  	if (fd->fd < 0) {
>  		tst_res(TCONF | TERRNO,
>  			"Skipping %s", tst_fd_desc(fd));
> @@ -228,7 +228,7 @@ static void open_fsopen(struct tst_fd *fd)

>  static void open_fspick(struct tst_fd *fd)
>  {
> -	fd->fd = fspick(AT_FDCWD, "/", 0);
> +	fd->fd = syscall(__NR_fspick, AT_FDCWD, "/", 0);
>  	if (fd->fd < 0) {
>  		tst_res(TCONF | TERRNO,
>  			"Skipping %s", tst_fd_desc(fd));
> @@ -237,7 +237,7 @@ static void open_fspick(struct tst_fd *fd)

>  static void open_open_tree(struct tst_fd *fd)
>  {
> -	fd->fd = open_tree(AT_FDCWD, "/", 0);
> +	fd->fd = syscall(__NR_open_tree, AT_FDCWD, "/", 0);
>  	if (fd->fd < 0) {
>  		tst_res(TCONF | TERRNO,
>  			"Skipping %s", tst_fd_desc(fd));

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Cyril Hrubis]

Hi!
> Actually at least here tst_syscall() needs to be called or it fails on older
> distros due missing ENOSYS check in raw syscall():

Ah, no we have to handle the ENOSYS ourselves as we do in the other
cases. Sorry for not realizing that.

We likely need just:

diff --git a/lib/tst_fd.c b/lib/tst_fd.c
index ab7de81aa..8b26ff8e5 100644
--- a/lib/tst_fd.c
+++ b/lib/tst_fd.c
@@ -141,7 +141,7 @@ static void open_pidfd(struct tst_fd *fd)
 {
        fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
        if (fd->fd < 0)
-               tst_brk(TBROK | TERRNO, "pidfd_open()");
+               tst_res(TCONF | TERRNO, "pidfd_open()");
 }


The tst_sycall() can't be called there at all _because_ it calls
tst_brk() itself.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Petr Vorel]

> Hi!
> Pushed, thanks.

Report too late, fix send.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Petr Vorel]

> Hi!
> > Actually at least here tst_syscall() needs to be called or it fails on older
> > distros due missing ENOSYS check in raw syscall():

> Ah, no we have to handle the ENOSYS ourselves as we do in the other
> cases. Sorry for not realizing that.

> We likely need just:

Yes, I also realized that when I double check your commit message.

Kind regards,
Petr

> diff --git a/lib/tst_fd.c b/lib/tst_fd.c
> index ab7de81aa..8b26ff8e5 100644
> --- a/lib/tst_fd.c
> +++ b/lib/tst_fd.c
> @@ -141,7 +141,7 @@ static void open_pidfd(struct tst_fd *fd)
>  {
>         fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
>         if (fd->fd < 0)
> -               tst_brk(TBROK | TERRNO, "pidfd_open()");
> +               tst_res(TCONF | TERRNO, "pidfd_open()");
>  }


> The tst_sycall() can't be called there at all _because_ it calls
> tst_brk() itself.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Martin Doucha]

On 24. 01. 24 15:21, Cyril Hrubis wrote:
> Hi!
>> Actually at least here tst_syscall() needs to be called or it fails on older
>> distros due missing ENOSYS check in raw syscall():
> 
> Ah, no we have to handle the ENOSYS ourselves as we do in the other
> cases. Sorry for not realizing that.
> 
> We likely need just:
> 
> diff --git a/lib/tst_fd.c b/lib/tst_fd.c
> index ab7de81aa..8b26ff8e5 100644
> --- a/lib/tst_fd.c
> +++ b/lib/tst_fd.c
> @@ -141,7 +141,7 @@ static void open_pidfd(struct tst_fd *fd)
>   {
>          fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
>          if (fd->fd < 0)
> -               tst_brk(TBROK | TERRNO, "pidfd_open()");
> +               tst_res(TCONF | TERRNO, "pidfd_open()");
>   }

Yes, this will work. I missed the TBROK because I didn't look too 
closely at the unchanged lines in the patch...

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Petr Vorel]

Hi Martin, Cyril,

> On 24. 01. 24 15:21, Cyril Hrubis wrote:
> > Hi!
> > > Actually at least here tst_syscall() needs to be called or it fails on older
> > > distros due missing ENOSYS check in raw syscall():

> > Ah, no we have to handle the ENOSYS ourselves as we do in the other
> > cases. Sorry for not realizing that.

> > We likely need just:

> > diff --git a/lib/tst_fd.c b/lib/tst_fd.c
> > index ab7de81aa..8b26ff8e5 100644
> > --- a/lib/tst_fd.c
> > +++ b/lib/tst_fd.c
> > @@ -141,7 +141,7 @@ static void open_pidfd(struct tst_fd *fd)
> >   {
> >          fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
> >          if (fd->fd < 0)
> > -               tst_brk(TBROK | TERRNO, "pidfd_open()");
> > +               tst_res(TCONF | TERRNO, "pidfd_open()");
> >   }

Actually, the solution I posted [1] works on both old (affected) kernel and new
one:

-	fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
+	fd->fd = tst_syscall(__NR_pidfd_open, getpid(), 0);
 	if (fd->fd < 0)
 		tst_brk(TBROK | TERRNO, "pidfd_open()");

I guess we should merge your solution (otherwise we would need to change other
cases to be consistent), but I'm a bit confused. Is it the reason why we use
syscall() + tst_res(TCONF) instead of tst_syscall() + tst_brk(TBROK) that for
some cases it's expected to fail, thus TBROK is not accepted? Again, I was blind
when doing review.

If yes, please just go ahead and merge it.

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20240124142108.303782-1-pvorel@suse.cz/

> Yes, this will work. I missed the TBROK because I didn't look too closely at
> the unchanged lines in the patch...

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Cyril Hrubis]

Hi!
> Actually, the solution I posted [1] works on both old (affected) kernel and new
> one:
> 
> -	fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
> +	fd->fd = tst_syscall(__NR_pidfd_open, getpid(), 0);
>  	if (fd->fd < 0)
>  		tst_brk(TBROK | TERRNO, "pidfd_open()");

This cannot work on kenrel that does not implement pidfd_open. That's
what the code in tst_syscall() does:

#define tst_syscall(NR, ...) ({ \
        intptr_t tst_ret; \
        if (NR == __LTP__NR_INVALID_SYSCALL) { \
                errno = ENOSYS; \
                tst_ret = -1; \
        } else { \
                tst_ret = syscall(NR, ##__VA_ARGS__); \
        } \
        if (tst_ret == -1 && errno == ENOSYS) { \
                TST_SYSCALL_BRK__(NR, #NR); \
        } \
        tst_ret; \
})

This means that either if the syscall number is undefined or if the
actuall syscall fails with ENOSYS we call tst_brk(TCONF, ...) or
tst_brkm(TCONF, ...) on old API.

> I guess we should merge your solution (otherwise we would need to change other
> cases to be consistent), but I'm a bit confused. Is it the reason why we use
> syscall() + tst_res(TCONF) instead of tst_syscall() + tst_brk(TBROK) that for
> some cases it's expected to fail, thus TBROK is not accepted? Again, I was blind
> when doing review.

The problem is that if kernel does not implement a particular syscall
the tst_syscall() calls tst_brk() which ends the tst_fd iteration in the
middle. The tst_fd iterator just loop over different types of file
descriptors, if you call tst_brk() anywhere the test ends before we
managed to finish the loop. We do not want that to happen because of
either syscall not implemented in older kernels or syscalls disabled by
CONFIG options.

That's why we have to call syscall() and do tst_res(TCONF, ...) when the
syscall had failed. The tst_fd_next() will just continue with next fd
type if we failed to produce a valid fd.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Petr Vorel]

> Hi!
> > Actually, the solution I posted [1] works on both old (affected) kernel and new
> > one:

> > -	fd->fd = syscall(__NR_pidfd_open, getpid(), 0);
> > +	fd->fd = tst_syscall(__NR_pidfd_open, getpid(), 0);
> >  	if (fd->fd < 0)
> >  		tst_brk(TBROK | TERRNO, "pidfd_open()");

> This cannot work on kenrel that does not implement pidfd_open. That's
> what the code in tst_syscall() does:

> #define tst_syscall(NR, ...) ({ \
>         intptr_t tst_ret; \
>         if (NR == __LTP__NR_INVALID_SYSCALL) { \
>                 errno = ENOSYS; \
>                 tst_ret = -1; \
>         } else { \
>                 tst_ret = syscall(NR, ##__VA_ARGS__); \
>         } \
>         if (tst_ret == -1 && errno == ENOSYS) { \
>                 TST_SYSCALL_BRK__(NR, #NR); \
>         } \
>         tst_ret; \
> })

> This means that either if the syscall number is undefined or if the
> actuall syscall fails with ENOSYS we call tst_brk(TCONF, ...) or
> tst_brkm(TCONF, ...) on old API.

> > I guess we should merge your solution (otherwise we would need to change other
> > cases to be consistent), but I'm a bit confused. Is it the reason why we use
> > syscall() + tst_res(TCONF) instead of tst_syscall() + tst_brk(TBROK) that for
> > some cases it's expected to fail, thus TBROK is not accepted? Again, I was blind
> > when doing review.

> The problem is that if kernel does not implement a particular syscall
> the tst_syscall() calls tst_brk() which ends the tst_fd iteration in the
> middle. The tst_fd iterator just loop over different types of file
> descriptors, if you call tst_brk() anywhere the test ends before we
> managed to finish the loop. We do not want that to happen because of
> either syscall not implemented in older kernels or syscalls disabled by
> CONFIG options.

> That's why we have to call syscall() and do tst_res(TCONF, ...) when the
> syscall had failed. The tst_fd_next() will just continue with next fd
> type if we failed to produce a valid fd.

Understand, you want to keep tst_res(), that makes sense.
And it does not make sense whether the failure is due ENOSYS or due other
failure (most likely due expected error).

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros

[Petr Vorel]

Hi Cyril, Martin,

FYI merged tst_res(TCONF) fix (fixed on 2 places).

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp