| Message ID | 20240124125813.6611-1-chrubis@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lib: tst_fd: Avoid tst_brk(TCONF, ...) on older distros | expand |
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));
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
Hi! Pushed, thanks.
> 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));
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.
> Hi! > Pushed, thanks. Report too late, fix send. Kind regards, Petr
> 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.
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...
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...
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.
> 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
Hi Cyril, Martin, FYI merged tst_res(TCONF) fix (fixed on 2 places). Kind regards, Petr
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));
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(-)