Re: [LTP] [PATCH] lib: tst_fd: Add kernel version check to memfd_secret

[Petr Vorel <pvorel@suse.cz>]

> Hi,

> Thanks for your review.

> > -----Original Message-----
> > From: Cyril Hrubis <chrubis@suse.cz>
> > Sent: Tuesday, June 18, 2024 7:16 PM
> > To: iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ＯＳＴ)
> > <nobuhiro1.iwamatsu@toshiba.co.jp>
> > Cc: ltp@lists.linux.it
> > Subject: Re: [PATCH] lib: tst_fd: Add kernel version check to memfd_secret

> > Hi!
> > > memfd_secret is a syscall added since 5.14. On earlier kernels, tests
> > > such as accept03, readahead01 and splice07 that use memfd_secret fail.
> > > This adds a kernel version check to the tst_fd library when running
> > > tests using memfd_secret.

> > > Test log on linux-5.10.162/arm32 with version 20240524:
> > > ```
> > > $ ./testcases/kernel/syscalls/accept/accept03
> > > tst_test.c:1733: TINFO: LTP version: 20240524
> > > tst_test.c:1617: TINFO: Timeout per run is 0h 00m 30s
> > > accept03.c:58: TPASS: accept() on file : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on O_PATH file : EBADF (9)
> > > accept03.c:58: TPASS: accept() on directory : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on /dev/zero : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on /proc/self/maps : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on pipe read end : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on pipe write end : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on epoll : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on eventfd : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on signalfd : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on timerfd : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on pidfd : ENOTSOCK (88)
> > > tst_fd.c:151: TCONF: Skipping fanotify: ENOSYS (38)
> > > accept03.c:58: TPASS: accept() on inotify : ENOTSOCK (88)
> > > tst_fd.c:170: TCONF: Skipping userfaultfd: ENOSYS (38)
> > > accept03.c:58: TPASS: accept() on perf event : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on io uring : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on bpf map : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on fsopen : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on fspick : ENOTSOCK (88)
> > > accept03.c:58: TPASS: accept() on open_tree : EBADF (9)
> > > accept03.c:58: TPASS: accept() on memfd : ENOTSOCK (88)
> > > tst_test.c:1677: TBROK: Test killed by SIGILL!

> > This looks like a bug either in kernel or libc.

> This is caused by __NR_memfd_secure being defined as -1 (0xffffffff)and "Illegal instruction" 
> occurs when syscall() is executed. And this problem does not occur on x86_64. 
> I cannot decide if this is a bug or not. I can't decide if this is a bug or not, because this behavior has
> existed for a long time.

Interesting. But it'd be good to discuss it, right? In case there is something
to improve. Cc linux-arm-kernel ML.

> > > Summary:
> > > passed   20
> > > failed   0
> > > broken   1
> > > skipped  2
> > > warnings 0
> > > ```

> > > Closed: #1145
> > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > > ---
> > >  lib/tst_fd.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)

> > > diff --git a/lib/tst_fd.c b/lib/tst_fd.c index 6538a098c..53f583fa0
> > > 100644
> > > --- a/lib/tst_fd.c
> > > +++ b/lib/tst_fd.c
> > > @@ -255,8 +255,16 @@ static void open_memfd(struct tst_fd *fd)

> > >  static void open_memfd_secret(struct tst_fd *fd)  {
> > > +	if ((tst_kvercmp(5, 14, 0)) < 0) {
> > > +		tst_res(TINFO, "accept() on %s: Linux kernel version is before
> > than v5.14", tst_fd_desc(fd));


> > > +		errno = ENOSYS;
> > > +		goto skip;
> > > +	}
> > > +
> > >  	fd->fd = syscall(__NR_memfd_secret, 0);
> > > +
> > >  	if (fd->fd < 0) {
> > > +skip:
> > >  		tst_res(TCONF | TERRNO,
> > >  			"Skipping %s", tst_fd_desc(fd));
> > >  	}

> > And this looks like you are working around the bug.

> Your point is correct...
> I would suggest using tst_syscall() to check for syscall undefined instead

Well, I guess we don't want to use tst_syscall() otherwise it would call
tst_brk(). I proposed similar patch some time ago [1], I suppose you told me
privately exactly this.

[1] https://patchwork.ozlabs.org/project/ltp/patch/20240124142108.303782-1-pvorel@suse.cz/

> of this modification. How about this modification? 

> ```
> --- a/lib/tst_fd.c
> +++ b/lib/tst_fd.c
> @@ -255,7 +255,8 @@ static void open_memfd(struct tst_fd *fd)

>  static void open_memfd_secret(struct tst_fd *fd)
>  {
> -       fd->fd = syscall(__NR_memfd_secret, 0);
> +       fd->fd = tst_syscall(__NR_memfd_secret, 0);
>         if (fd->fd < 0) {
>                 tst_res(TCONF | TERRNO,
>                         "Skipping %s", tst_fd_desc(fd));


Therefore how about this?

	if ((tst_kvercmp(5, 14, 0)) < 0) {
		tst_res(TCONF, "accept() on %s: skipping due old kernel", tst_fd_desc(fd));
		return;
	}

Kind regards,
Petr

> Best regards,
>   Nobuhiro

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