| Message ID | 20230131002532.459456-1-edliaw@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | close_range: check for kernel support below 5.9 | expand |
Hi Edward, good catch :). Reviewed-by: Petr Vorel <pvorel@suse.cz> ... > +++ b/include/lapi/close_range.h > @@ -25,4 +25,17 @@ static inline int close_range(unsigned int fd, unsigned int max_fd, > return tst_syscall(__NR_close_range, fd, max_fd, flags); > } > # endif > + > +static inline void close_range_supported_by_kernel(void) > +{ > + long ret; > + > + if ((tst_kvercmp(5, 9, 0)) < 0) { > + /* Check if the syscall is backported on an older kernel */ BTW what particular use case this fixed? Is it backported to some android kernel? Or to some enterprise distro? Because I don't think kernel stable trees accept new functionality, just fixes. Kind regards, Petr
Hi Petr, > > +++ b/include/lapi/close_range.h > > @@ -25,4 +25,17 @@ static inline int close_range(unsigned int fd, unsigned int max_fd, > > return tst_syscall(__NR_close_range, fd, max_fd, flags); > > } > > # endif > > + > > +static inline void close_range_supported_by_kernel(void) > > +{ > > + long ret; > > + > > + if ((tst_kvercmp(5, 9, 0)) < 0) { > > + /* Check if the syscall is backported on an older kernel */ > BTW what particular use case this fixed? Is it backported to some android > kernel? Or to some enterprise distro? Because I don't think kernel stable trees > accept new functionality, just fixes. Oops, should I use .min_kver instead? It isn't backported on Android; I just wasn't sure what the right approach was. Thanks, Edward
Hi Edward, > Hi Petr, > > > +++ b/include/lapi/close_range.h > > > @@ -25,4 +25,17 @@ static inline int close_range(unsigned int fd, unsigned int max_fd, > > > return tst_syscall(__NR_close_range, fd, max_fd, flags); > > > } > > > # endif > > > + > > > +static inline void close_range_supported_by_kernel(void) > > > +{ > > > + long ret; > > > + > > > + if ((tst_kvercmp(5, 9, 0)) < 0) { > > > + /* Check if the syscall is backported on an older kernel */ > > BTW what particular use case this fixed? Is it backported to some android > > kernel? Or to some enterprise distro? Because I don't think kernel stable trees > > accept new functionality, just fixes. > Oops, should I use .min_kver instead? It isn't backported on Android; > I just wasn't sure what the right approach was. Actually, looking into SLES kernel sources, we backported close_range() to SLES 15-SP3 (bsc#1179090), which was 5.3.18 based. Thus you actually did good work :). @Li, @Jan out of curiosity, was this backported to RHEL kernel as well? It'd be useful to note that (one day we will be able to remove it once kernels which backported are EOL). Kind regards, Petr > Thanks, > Edward
On Thu, Feb 2, 2023 at 8:50 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Edward, > > > Hi Petr, > > > > > +++ b/include/lapi/close_range.h > > > > @@ -25,4 +25,17 @@ static inline int close_range(unsigned int fd, unsigned int max_fd, > > > > return tst_syscall(__NR_close_range, fd, max_fd, flags); > > > > } > > > > # endif > > > > + > > > > +static inline void close_range_supported_by_kernel(void) > > > > +{ > > > > + long ret; > > > > + > > > > + if ((tst_kvercmp(5, 9, 0)) < 0) { > > > > + /* Check if the syscall is backported on an older kernel */ > > > BTW what particular use case this fixed? Is it backported to some android > > > kernel? Or to some enterprise distro? Because I don't think kernel stable trees > > > accept new functionality, just fixes. > > > Oops, should I use .min_kver instead? It isn't backported on Android; > > I just wasn't sure what the right approach was. > > Actually, looking into SLES kernel sources, we backported close_range() to SLES > 15-SP3 (bsc#1179090), which was 5.3.18 based. Thus you actually did good work :). > > @Li, @Jan out of curiosity, was this backported to RHEL kernel as well? > It'd be useful to note that (one day we will be able to remove it once kernels > which backported are EOL). Yes, it's present in 8.4 since kernel-4.18.0-290.el8. > > Kind regards, > Petr > > > Thanks, > > Edward >
> On Thu, Feb 2, 2023 at 8:50 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Edward, > > > Hi Petr, > > > > > +++ b/include/lapi/close_range.h > > > > > @@ -25,4 +25,17 @@ static inline int close_range(unsigned int fd, unsigned int max_fd, > > > > > return tst_syscall(__NR_close_range, fd, max_fd, flags); > > > > > } > > > > > # endif > > > > > + > > > > > +static inline void close_range_supported_by_kernel(void) > > > > > +{ > > > > > + long ret; > > > > > + > > > > > + if ((tst_kvercmp(5, 9, 0)) < 0) { > > > > > + /* Check if the syscall is backported on an older kernel */ > > > > BTW what particular use case this fixed? Is it backported to some android > > > > kernel? Or to some enterprise distro? Because I don't think kernel stable trees > > > > accept new functionality, just fixes. > > > Oops, should I use .min_kver instead? It isn't backported on Android; > > > I just wasn't sure what the right approach was. > > Actually, looking into SLES kernel sources, we backported close_range() to SLES > > 15-SP3 (bsc#1179090), which was 5.3.18 based. Thus you actually did good work :). > > @Li, @Jan out of curiosity, was this backported to RHEL kernel as well? > > It'd be useful to note that (one day we will be able to remove it once kernels > > which backported are EOL). > Yes, it's present in 8.4 since kernel-4.18.0-290.el8. Jan, thanks a lot! @Edward we have Hackweek at SUSE, I'll merge this on Monday, noting the reason why to keep this instead simple .min_kver. Kind regards, Petr > > Kind regards, > > Petr > > > Thanks, > > > Edward
Oh, lucky coincidence I guess. Thanks, Petr! On Thu, Feb 2, 2023 at 12:20 AM Petr Vorel <pvorel@suse.cz> wrote: > > > On Thu, Feb 2, 2023 at 8:50 AM Petr Vorel <pvorel@suse.cz> wrote: > > > > Hi Edward, > > > > > Hi Petr, > > > > > > > +++ b/include/lapi/close_range.h > > > > > > @@ -25,4 +25,17 @@ static inline int close_range(unsigned int fd, unsigned int max_fd, > > > > > > return tst_syscall(__NR_close_range, fd, max_fd, flags); > > > > > > } > > > > > > # endif > > > > > > + > > > > > > +static inline void close_range_supported_by_kernel(void) > > > > > > +{ > > > > > > + long ret; > > > > > > + > > > > > > + if ((tst_kvercmp(5, 9, 0)) < 0) { > > > > > > + /* Check if the syscall is backported on an older kernel */ > > > > > BTW what particular use case this fixed? Is it backported to some android > > > > > kernel? Or to some enterprise distro? Because I don't think kernel stable trees > > > > > accept new functionality, just fixes. > > > > > Oops, should I use .min_kver instead? It isn't backported on Android; > > > > I just wasn't sure what the right approach was. > > > > Actually, looking into SLES kernel sources, we backported close_range() to SLES > > > 15-SP3 (bsc#1179090), which was 5.3.18 based. Thus you actually did good work :). > > > > @Li, @Jan out of curiosity, was this backported to RHEL kernel as well? > > > It'd be useful to note that (one day we will be able to remove it once kernels > > > which backported are EOL). > > > Yes, it's present in 8.4 since kernel-4.18.0-290.el8. > > Jan, thanks a lot! > > @Edward we have Hackweek at SUSE, I'll merge this on Monday, > noting the reason why to keep this instead simple .min_kver. > > Kind regards, > Petr > > > > Kind regards, > > > Petr > > > > > Thanks, > > > > Edward > >
Hi Edward, Petr, Sorry for the late append reply. On Tue, Jan 31, 2023 at 8:25 AM Edward Liaw via ltp <ltp@lists.linux.it> wrote: > Check for close_range syscall support in the kernel before running > close_range01 and close_range02 tests. > > Signed-off-by: Edward Liaw <edliaw@google.com> > --- > include/lapi/close_range.h | 13 +++++++++++++ > .../kernel/syscalls/close_range/close_range01.c | 2 ++ > .../kernel/syscalls/close_range/close_range02.c | 1 + > 3 files changed, 16 insertions(+) > > diff --git a/include/lapi/close_range.h b/include/lapi/close_range.h > index 19db52d3d..0e464bb30 100644 > --- a/include/lapi/close_range.h > +++ b/include/lapi/close_range.h > @@ -25,4 +25,17 @@ static inline int close_range(unsigned int fd, unsigned > int max_fd, > return tst_syscall(__NR_close_range, fd, max_fd, flags); > } > # endif > + > +static inline void close_range_supported_by_kernel(void) > +{ > + long ret; > + > + if ((tst_kvercmp(5, 9, 0)) < 0) { > Using tst_kvercmp together with ENOSYS check is a repetitive practice, I think verifying the returned value and errno is quite enough to determine an un-support syscall. And there is possible a lite kernel version newer than 5.9 removes this syscall support. Then this test will fail again because this tst_kvercmp skips the ENOSYS check. Otherwise: Reviewed-by: Li Wang <liwang@redhat.com> > + /* Check if the syscall is backported on an older kernel */ > + ret = syscall(__NR_close_range, 1, 0, 0); > + if (ret == -1 && errno == ENOSYS) > + tst_brk(TCONF, "Test not supported on kernel > version < v5.9"); > + } > +} > + > #endif /* LAPI_CLOSE_RANGE_H__ */ > diff --git a/testcases/kernel/syscalls/close_range/close_range01.c > b/testcases/kernel/syscalls/close_range/close_range01.c > index 30bb600b6..072bbab66 100644 > --- a/testcases/kernel/syscalls/close_range/close_range01.c > +++ b/testcases/kernel/syscalls/close_range/close_range01.c > @@ -53,6 +53,8 @@ static inline void do_close_range(unsigned int fd, > unsigned int max_fd, > > static void setup(void) > { > + close_range_supported_by_kernel(); > + > struct rlimit nfd; > > SAFE_GETRLIMIT(RLIMIT_NOFILE, &nfd); > diff --git a/testcases/kernel/syscalls/close_range/close_range02.c > b/testcases/kernel/syscalls/close_range/close_range02.c > index aec899261..2aa6d2c9f 100644 > --- a/testcases/kernel/syscalls/close_range/close_range02.c > +++ b/testcases/kernel/syscalls/close_range/close_range02.c > @@ -111,4 +111,5 @@ static struct tst_test test = { > .tcnt = 6, > .forks_child = 1, > .test = run, > + .setup = close_range_supported_by_kernel, > }; > -- > 2.39.1.456.gfc5497dd1b-goog > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > >
Hi all, Edward, thanks for your work, merged. Kind regards, Petr
Hi, hm, I was too fast to merge it. Looking at older result, It looks like this (merged as 7b5ee03899) was not needed, because tst_syscall() properly detects missing support: close_range01.c:134: TINFO: Plain close range ../../../../include/lapi/close_range.h:25: TCONF: syscall(436) __NR_close_range not supported on your arch @Edward: Before we revert it, did you encounter some problem that it's really needed? If yes, please share details. Kind regards, Petr
Hey Petr, We turned on the HAVE_CLOSE_RANGE flag to test the bionic-defined close_range on Android, but doing so bypasses tst_syscall in include/lapi/close_range.h. We don't currently have a way to configure ltp differently across each kernel version, so I wanted to use this check as a fallback to gate earlier kernels. Thanks, Edward On Fri, Feb 3, 2023 at 2:25 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi, > > hm, I was too fast to merge it. Looking at older result, > It looks like this (merged as 7b5ee03899) was not needed, > because tst_syscall() properly detects missing support: > > close_range01.c:134: TINFO: Plain close range > ../../../../include/lapi/close_range.h:25: TCONF: syscall(436) __NR_close_range not supported on your arch > > @Edward: Before we revert it, did you encounter some problem that it's really > needed? If yes, please share details. > > Kind regards, > Petr
Hi Edward, > Hey Petr, > We turned on the HAVE_CLOSE_RANGE flag to test the bionic-defined > close_range on Android, but doing so bypasses tst_syscall in > include/lapi/close_range.h. We don't currently have a way to > configure ltp differently across each kernel version, so I wanted to > use this check as a fallback to gate earlier kernels. Thanks for info. OK, at least it's needed :). If these fixes would be needed for more syscalls, we might want to check if there is a way to enhance tst_syscall() (likely there is no way). Also (for next time) it's always safe to be more verbose for the reason in the commit message to prevent cleanup of workarounds too early (before EOL of that particular kernel). Kind regards, Petr > Thanks, > Edward > On Fri, Feb 3, 2023 at 2:25 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi, > > hm, I was too fast to merge it. Looking at older result, > > It looks like this (merged as 7b5ee03899) was not needed, > > because tst_syscall() properly detects missing support: > > close_range01.c:134: TINFO: Plain close range > > ../../../../include/lapi/close_range.h:25: TCONF: syscall(436) __NR_close_range not supported on your arch > > @Edward: Before we revert it, did you encounter some problem that it's really > > needed? If yes, please share details. > > Kind regards, > > Petr
Hi Petr, Thanks, I will do that in the future. This is the only time I've encountered this so far but will look into doing that if there is another. Thanks, Edward
diff --git a/include/lapi/close_range.h b/include/lapi/close_range.h index 19db52d3d..0e464bb30 100644 --- a/include/lapi/close_range.h +++ b/include/lapi/close_range.h @@ -25,4 +25,17 @@ static inline int close_range(unsigned int fd, unsigned int max_fd, return tst_syscall(__NR_close_range, fd, max_fd, flags); } # endif + +static inline void close_range_supported_by_kernel(void) +{ + long ret; + + if ((tst_kvercmp(5, 9, 0)) < 0) { + /* Check if the syscall is backported on an older kernel */ + ret = syscall(__NR_close_range, 1, 0, 0); + if (ret == -1 && errno == ENOSYS) + tst_brk(TCONF, "Test not supported on kernel version < v5.9"); + } +} + #endif /* LAPI_CLOSE_RANGE_H__ */ diff --git a/testcases/kernel/syscalls/close_range/close_range01.c b/testcases/kernel/syscalls/close_range/close_range01.c index 30bb600b6..072bbab66 100644 --- a/testcases/kernel/syscalls/close_range/close_range01.c +++ b/testcases/kernel/syscalls/close_range/close_range01.c @@ -53,6 +53,8 @@ static inline void do_close_range(unsigned int fd, unsigned int max_fd, static void setup(void) { + close_range_supported_by_kernel(); + struct rlimit nfd; SAFE_GETRLIMIT(RLIMIT_NOFILE, &nfd); diff --git a/testcases/kernel/syscalls/close_range/close_range02.c b/testcases/kernel/syscalls/close_range/close_range02.c index aec899261..2aa6d2c9f 100644 --- a/testcases/kernel/syscalls/close_range/close_range02.c +++ b/testcases/kernel/syscalls/close_range/close_range02.c @@ -111,4 +111,5 @@ static struct tst_test test = { .tcnt = 6, .forks_child = 1, .test = run, + .setup = close_range_supported_by_kernel, };
Check for close_range syscall support in the kernel before running close_range01 and close_range02 tests. Signed-off-by: Edward Liaw <edliaw@google.com> --- include/lapi/close_range.h | 13 +++++++++++++ .../kernel/syscalls/close_range/close_range01.c | 2 ++ .../kernel/syscalls/close_range/close_range02.c | 1 + 3 files changed, 16 insertions(+)