* [viro-vfs:work.fdtable 13/13] kernel/fork.c:3242 unshare_fd() warn: passing a valid pointer to 'PTR_ERR'
@ 2024-08-13 8:00 Dan Carpenter
2024-08-13 18:16 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2024-08-13 8:00 UTC (permalink / raw)
To: oe-kbuild, Al Viro; +Cc: lkp, oe-kbuild-all, linux-fsdevel
tree: https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.fdtable
head: 3f4b0acefd818ec43b68455994ac2bd5166c06ae
commit: 3f4b0acefd818ec43b68455994ac2bd5166c06ae [13/13] dup_fd(): change calling conventions
config: x86_64-randconfig-161-20240813 (https://download.01.org/0day-ci/archive/20240813/202408130945.I8wIAYBm-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202408130945.I8wIAYBm-lkp@intel.com/
smatch warnings:
kernel/fork.c:3242 unshare_fd() warn: passing a valid pointer to 'PTR_ERR'
vim +/PTR_ERR +3242 kernel/fork.c
60997c3d45d9a6 Christian Brauner 2020-06-03 3232 int unshare_fd(unsigned long unshare_flags, unsigned int max_fds,
60997c3d45d9a6 Christian Brauner 2020-06-03 3233 struct files_struct **new_fdp)
cf2e340f4249b7 JANAK DESAI 2006-02-07 3234 {
cf2e340f4249b7 JANAK DESAI 2006-02-07 3235 struct files_struct *fd = current->files;
cf2e340f4249b7 JANAK DESAI 2006-02-07 3236
cf2e340f4249b7 JANAK DESAI 2006-02-07 3237 if ((unshare_flags & CLONE_FILES) &&
a016f3389c0660 JANAK DESAI 2006-02-07 3238 (fd && atomic_read(&fd->count) > 1)) {
3f4b0acefd818e Al Viro 2024-08-06 3239 *new_fdp = dup_fd(fd, max_fds);
3f4b0acefd818e Al Viro 2024-08-06 3240 if (IS_ERR(*new_fdp)) {
3f4b0acefd818e Al Viro 2024-08-06 3241 *new_fdp = NULL;
3f4b0acefd818e Al Viro 2024-08-06 @3242 return PTR_ERR(new_fdp);
^^^^^^^^^^^^^^^^
err = PTR_ERR(*new_fdp);
*new_fdp = NULL;
return err;
3f4b0acefd818e Al Viro 2024-08-06 3243 }
a016f3389c0660 JANAK DESAI 2006-02-07 3244 }
cf2e340f4249b7 JANAK DESAI 2006-02-07 3245
cf2e340f4249b7 JANAK DESAI 2006-02-07 3246 return 0;
cf2e340f4249b7 JANAK DESAI 2006-02-07 3247 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [viro-vfs:work.fdtable 13/13] kernel/fork.c:3242 unshare_fd() warn: passing a valid pointer to 'PTR_ERR'
2024-08-13 8:00 [viro-vfs:work.fdtable 13/13] kernel/fork.c:3242 unshare_fd() warn: passing a valid pointer to 'PTR_ERR' Dan Carpenter
@ 2024-08-13 18:16 ` Al Viro
2024-08-14 1:03 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2024-08-13 18:16 UTC (permalink / raw)
To: Dan Carpenter; +Cc: oe-kbuild, lkp, oe-kbuild-all, linux-fsdevel
On Tue, Aug 13, 2024 at 11:00:04AM +0300, Dan Carpenter wrote:
> 3f4b0acefd818e Al Viro 2024-08-06 3240 if (IS_ERR(*new_fdp)) {
> 3f4b0acefd818e Al Viro 2024-08-06 3241 *new_fdp = NULL;
> 3f4b0acefd818e Al Viro 2024-08-06 @3242 return PTR_ERR(new_fdp);
> ^^^^^^^^^^^^^^^^
> err = PTR_ERR(*new_fdp);
> *new_fdp = NULL;
> return err;
Argh... Obvious braino, but what it shows is that failures of that
thing are not covered by anything in e.g. LTP. Or in-kernel
self-tests, for that matter...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [viro-vfs:work.fdtable 13/13] kernel/fork.c:3242 unshare_fd() warn: passing a valid pointer to 'PTR_ERR'
2024-08-13 18:16 ` Al Viro
@ 2024-08-14 1:03 ` Al Viro
2024-08-21 6:38 ` Shuah Khan
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2024-08-14 1:03 UTC (permalink / raw)
To: Dan Carpenter; +Cc: oe-kbuild, lkp, oe-kbuild-all, linux-fsdevel, Shuah Khan
On Tue, Aug 13, 2024 at 07:16:00PM +0100, Al Viro wrote:
> On Tue, Aug 13, 2024 at 11:00:04AM +0300, Dan Carpenter wrote:
> > 3f4b0acefd818e Al Viro 2024-08-06 3240 if (IS_ERR(*new_fdp)) {
> > 3f4b0acefd818e Al Viro 2024-08-06 3241 *new_fdp = NULL;
> > 3f4b0acefd818e Al Viro 2024-08-06 @3242 return PTR_ERR(new_fdp);
> > ^^^^^^^^^^^^^^^^
> > err = PTR_ERR(*new_fdp);
> > *new_fdp = NULL;
> > return err;
>
> Argh... Obvious braino, but what it shows is that failures of that
> thing are not covered by anything in e.g. LTP. Or in-kernel
> self-tests, for that matter...
FWIW, this does exercise that codepath, but I would really like to
have kselftest folks to comment on the damn thing - I'm pretty sure
that it's _not_ a good style for those.
diff --git a/tools/testing/selftests/core/Makefile b/tools/testing/selftests/core/Makefile
index ce262d097269..8e99f87f5d7c 100644
--- a/tools/testing/selftests/core/Makefile
+++ b/tools/testing/selftests/core/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
CFLAGS += -g $(KHDR_INCLUDES)
-TEST_GEN_PROGS := close_range_test
+TEST_GEN_PROGS := close_range_test unshare_test
include ../lib.mk
diff --git a/tools/testing/selftests/core/unshare_test.c b/tools/testing/selftests/core/unshare_test.c
new file mode 100644
index 000000000000..7fec9dfb1b0e
--- /dev/null
+++ b/tools/testing/selftests/core/unshare_test.c
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/kernel.h>
+#include <limits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <unistd.h>
+#include <sys/resource.h>
+#include <linux/close_range.h>
+
+#include "../kselftest_harness.h"
+#include "../clone3/clone3_selftests.h"
+
+TEST(unshare_EMFILE)
+{
+ pid_t pid;
+ int status;
+ struct __clone_args args = {
+ .flags = CLONE_FILES,
+ .exit_signal = SIGCHLD,
+ };
+ int fd;
+ ssize_t n, n2;
+ static char buf[512], buf2[512];
+ struct rlimit rlimit;
+ int nr_open;
+
+ fd = open("/proc/sys/fs/nr_open", O_RDWR);
+ ASSERT_GE(fd, 0);
+
+ n = read(fd, buf, sizeof(buf));
+ ASSERT_GT(n, 0);
+ ASSERT_EQ(buf[n - 1], '\n');
+
+ ASSERT_EQ(sscanf(buf, "%d", &nr_open), 1);
+
+ ASSERT_EQ(0, getrlimit(RLIMIT_NOFILE, &rlimit));
+
+ /* bump fs.nr_open */
+ n2 = sprintf(buf2, "%d\n", nr_open + 1024);
+ lseek(fd, 0, SEEK_SET);
+ write(fd, buf2, n2);
+
+ /* bump ulimit -n */
+ rlimit.rlim_cur = nr_open + 1024;
+ rlimit.rlim_max = nr_open + 1024;
+ EXPECT_EQ(0, setrlimit(RLIMIT_NOFILE, &rlimit)) {
+ lseek(fd, 0, SEEK_SET);
+ write(fd, buf, n);
+ exit(EXIT_FAILURE);
+ }
+
+ /* get a descriptor past the old fs.nr_open */
+ EXPECT_GE(dup2(2, nr_open + 64), 0) {
+ lseek(fd, 0, SEEK_SET);
+ write(fd, buf, n);
+ exit(EXIT_FAILURE);
+ }
+
+ /* get descriptor table shared */
+ pid = sys_clone3(&args, sizeof(args));
+ EXPECT_GE(pid, 0) {
+ lseek(fd, 0, SEEK_SET);
+ write(fd, buf, n);
+ exit(EXIT_FAILURE);
+ }
+
+ if (pid == 0) {
+ int err;
+
+ /* restore fs.nr_open */
+ lseek(fd, 0, SEEK_SET);
+ write(fd, buf, n);
+ /* ... and now unshare(CLONE_FILES) must fail with EMFILE */
+ err = unshare(CLONE_FILES);
+ EXPECT_EQ(err, -1)
+ exit(EXIT_FAILURE);
+ EXPECT_EQ(errno, EMFILE)
+ exit(EXIT_FAILURE);
+ exit(EXIT_SUCCESS);
+ }
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+}
+
+TEST_HARNESS_MAIN
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [viro-vfs:work.fdtable 13/13] kernel/fork.c:3242 unshare_fd() warn: passing a valid pointer to 'PTR_ERR'
2024-08-14 1:03 ` Al Viro
@ 2024-08-21 6:38 ` Shuah Khan
2024-08-22 0:15 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Shuah Khan @ 2024-08-21 6:38 UTC (permalink / raw)
To: Al Viro, Dan Carpenter
Cc: oe-kbuild, lkp, oe-kbuild-all, linux-fsdevel, Shuah Khan,
Shuah Khan
On 8/13/24 19:03, Al Viro wrote:
> On Tue, Aug 13, 2024 at 07:16:00PM +0100, Al Viro wrote:
>> On Tue, Aug 13, 2024 at 11:00:04AM +0300, Dan Carpenter wrote:
>>> 3f4b0acefd818e Al Viro 2024-08-06 3240 if (IS_ERR(*new_fdp)) {
>>> 3f4b0acefd818e Al Viro 2024-08-06 3241 *new_fdp = NULL;
>>> 3f4b0acefd818e Al Viro 2024-08-06 @3242 return PTR_ERR(new_fdp);
>>> ^^^^^^^^^^^^^^^^
>>> err = PTR_ERR(*new_fdp);
>>> *new_fdp = NULL;
>>> return err;
>>
>> Argh... Obvious braino, but what it shows is that failures of that
>> thing are not covered by anything in e.g. LTP. Or in-kernel
>> self-tests, for that matter...
>
> FWIW, this does exercise that codepath, but I would really like to
> have kselftest folks to comment on the damn thing - I'm pretty sure
> that it's _not_ a good style for those.
Looks good to me. Would you be able to send a patch for this new test?
>
> diff --git a/tools/testing/selftests/core/Makefile b/tools/testing/selftests/core/Makefile
> index ce262d097269..8e99f87f5d7c 100644
> --- a/tools/testing/selftests/core/Makefile
> +++ b/tools/testing/selftests/core/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> CFLAGS += -g $(KHDR_INCLUDES)
>
> -TEST_GEN_PROGS := close_range_test
> +TEST_GEN_PROGS := close_range_test unshare_test
>
> include ../lib.mk
>
> diff --git a/tools/testing/selftests/core/unshare_test.c b/tools/testing/selftests/core/unshare_test.c
> new file mode 100644
> index 000000000000..7fec9dfb1b0e
> --- /dev/null
> +++ b/tools/testing/selftests/core/unshare_test.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/kernel.h>
> +#include <limits.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <syscall.h>
> +#include <unistd.h>
> +#include <sys/resource.h>
> +#include <linux/close_range.h>
> +
> +#include "../kselftest_harness.h"
> +#include "../clone3/clone3_selftests.h"
> +
> +TEST(unshare_EMFILE)
> +{
> + pid_t pid;
> + int status;
> + struct __clone_args args = {
> + .flags = CLONE_FILES,
> + .exit_signal = SIGCHLD,
> + };
> + int fd;
> + ssize_t n, n2;
> + static char buf[512], buf2[512];
> + struct rlimit rlimit;
> + int nr_open;
> +
> + fd = open("/proc/sys/fs/nr_open", O_RDWR);
> + ASSERT_GE(fd, 0);
> +
> + n = read(fd, buf, sizeof(buf));
> + ASSERT_GT(n, 0);
> + ASSERT_EQ(buf[n - 1], '\n');
> +
> + ASSERT_EQ(sscanf(buf, "%d", &nr_open), 1);
> +
> + ASSERT_EQ(0, getrlimit(RLIMIT_NOFILE, &rlimit));
> +
> + /* bump fs.nr_open */
> + n2 = sprintf(buf2, "%d\n", nr_open + 1024);
> + lseek(fd, 0, SEEK_SET);
> + write(fd, buf2, n2);
> +
> + /* bump ulimit -n */
> + rlimit.rlim_cur = nr_open + 1024;
> + rlimit.rlim_max = nr_open + 1024;
> + EXPECT_EQ(0, setrlimit(RLIMIT_NOFILE, &rlimit)) {
> + lseek(fd, 0, SEEK_SET);
> + write(fd, buf, n);
> + exit(EXIT_FAILURE);
> + }
> +
> + /* get a descriptor past the old fs.nr_open */
> + EXPECT_GE(dup2(2, nr_open + 64), 0) {
> + lseek(fd, 0, SEEK_SET);
> + write(fd, buf, n);
> + exit(EXIT_FAILURE);
> + }
> +
> + /* get descriptor table shared */
> + pid = sys_clone3(&args, sizeof(args));
> + EXPECT_GE(pid, 0) {
> + lseek(fd, 0, SEEK_SET);
> + write(fd, buf, n);
> + exit(EXIT_FAILURE);
> + }
> +
> + if (pid == 0) {
> + int err;
> +
> + /* restore fs.nr_open */
> + lseek(fd, 0, SEEK_SET);
> + write(fd, buf, n);
> + /* ... and now unshare(CLONE_FILES) must fail with EMFILE */
> + err = unshare(CLONE_FILES);
> + EXPECT_EQ(err, -1)
> + exit(EXIT_FAILURE);
> + EXPECT_EQ(errno, EMFILE)
> + exit(EXIT_FAILURE);
> + exit(EXIT_SUCCESS);
> + }
> +
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +}
> +
> +TEST_HARNESS_MAIN
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [viro-vfs:work.fdtable 13/13] kernel/fork.c:3242 unshare_fd() warn: passing a valid pointer to 'PTR_ERR'
2024-08-21 6:38 ` Shuah Khan
@ 2024-08-22 0:15 ` Al Viro
2024-08-22 4:12 ` Shuah Khan
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2024-08-22 0:15 UTC (permalink / raw)
To: Shuah Khan
Cc: Dan Carpenter, oe-kbuild, lkp, oe-kbuild-all, linux-fsdevel,
Shuah Khan
On Wed, Aug 21, 2024 at 12:38:48AM -0600, Shuah Khan wrote:
> On 8/13/24 19:03, Al Viro wrote:
> > On Tue, Aug 13, 2024 at 07:16:00PM +0100, Al Viro wrote:
> > > On Tue, Aug 13, 2024 at 11:00:04AM +0300, Dan Carpenter wrote:
> > > > 3f4b0acefd818e Al Viro 2024-08-06 3240 if (IS_ERR(*new_fdp)) {
> > > > 3f4b0acefd818e Al Viro 2024-08-06 3241 *new_fdp = NULL;
> > > > 3f4b0acefd818e Al Viro 2024-08-06 @3242 return PTR_ERR(new_fdp);
> > > > ^^^^^^^^^^^^^^^^
> > > > err = PTR_ERR(*new_fdp);
> > > > *new_fdp = NULL;
> > > > return err;
> > >
> > > Argh... Obvious braino, but what it shows is that failures of that
> > > thing are not covered by anything in e.g. LTP. Or in-kernel
> > > self-tests, for that matter...
> >
> > FWIW, this does exercise that codepath, but I would really like to
> > have kselftest folks to comment on the damn thing - I'm pretty sure
> > that it's _not_ a good style for those.
>
> Looks good to me. Would you be able to send a patch for this new test?
Umm... Send as in "mail somewhere specific", or as "push into vfs.git",
or...?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [viro-vfs:work.fdtable 13/13] kernel/fork.c:3242 unshare_fd() warn: passing a valid pointer to 'PTR_ERR'
2024-08-22 0:15 ` Al Viro
@ 2024-08-22 4:12 ` Shuah Khan
0 siblings, 0 replies; 6+ messages in thread
From: Shuah Khan @ 2024-08-22 4:12 UTC (permalink / raw)
To: Al Viro
Cc: Dan Carpenter, oe-kbuild, lkp, oe-kbuild-all, linux-fsdevel,
Shuah Khan, Shuah Khan
On 8/21/24 18:15, Al Viro wrote:
> On Wed, Aug 21, 2024 at 12:38:48AM -0600, Shuah Khan wrote:
>> On 8/13/24 19:03, Al Viro wrote:
>>> On Tue, Aug 13, 2024 at 07:16:00PM +0100, Al Viro wrote:
>>>> On Tue, Aug 13, 2024 at 11:00:04AM +0300, Dan Carpenter wrote:
>>>>> 3f4b0acefd818e Al Viro 2024-08-06 3240 if (IS_ERR(*new_fdp)) {
>>>>> 3f4b0acefd818e Al Viro 2024-08-06 3241 *new_fdp = NULL;
>>>>> 3f4b0acefd818e Al Viro 2024-08-06 @3242 return PTR_ERR(new_fdp);
>>>>> ^^^^^^^^^^^^^^^^
>>>>> err = PTR_ERR(*new_fdp);
>>>>> *new_fdp = NULL;
>>>>> return err;
>>>>
>>>> Argh... Obvious braino, but what it shows is that failures of that
>>>> thing are not covered by anything in e.g. LTP. Or in-kernel
>>>> self-tests, for that matter...
>>>
>>> FWIW, this does exercise that codepath, but I would really like to
>>> have kselftest folks to comment on the damn thing - I'm pretty sure
>>> that it's _not_ a good style for those.
>>
>> Looks good to me. Would you be able to send a patch for this new test?
>
> Umm... Send as in "mail somewhere specific", or as "push into vfs.git",
> or...?
If you can send the patch to kselftest mailing test, I can take this through
my tree.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-08-22 4:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-13 8:00 [viro-vfs:work.fdtable 13/13] kernel/fork.c:3242 unshare_fd() warn: passing a valid pointer to 'PTR_ERR' Dan Carpenter
2024-08-13 18:16 ` Al Viro
2024-08-14 1:03 ` Al Viro
2024-08-21 6:38 ` Shuah Khan
2024-08-22 0:15 ` Al Viro
2024-08-22 4:12 ` Shuah Khan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).