(please be noted we reported "[ocfs2] 46e33fd45a: sysctl_table_check_failed"
on https://lists.01.org/hyperkitty/list/[email protected]/thread/KQ2F6TPJWMDVEXJM4WTUC4DU3EH3YJVT/
when this commit on:
commit: 46e33fd45a52bf03769906e64d8a8a1ab317777d ("ocfs2: simplify subdirectory
registration with register_sysctl()")
https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git 20211118-sysctl-cleanups-set-04
we resend this report as a reminder the issue still exists on maineline)
Greeting,
FYI, we noticed the following commit (built with clang-14):
commit: c42ff46f97c1c25577a84fbfb111710d25a129e0 ("ocfs2: simplify subdirectory registration with register_sysctl()")
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu Icelake-Server -smp 4 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+---------------------------+------------+------------+
| | e99f5e7479 | c42ff46f97 |
+---------------------------+------------+------------+
| sysctl_table_check_failed | 0 | 10 |
+---------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 187.975448][ T1] ksmbd: The ksmbd server is experimental, use at your own risk.
[ 187.977238][ T1] QNX4 filesystem 0.2.3 registered.
[ 187.979872][ T1] orangefs_debugfs_init: called with debug mask: :none: :0:
[ 187.981449][ T1] orangefs_init: module version upstream loaded
[ 187.983277][ T1] JFS: nTxBlock = 8192, nTxLock = 65536
[ 188.000617][ T1] sysctl table check failed: fs/ocfs2/nm Not a file
[ 188.001528][ T1] sysctl table check failed: fs/ocfs2/nm No proc_handler
[ 188.002483][ T1] sysctl table check failed: fs/ocfs2/nm bogus .mode 0555
[ 188.003437][ T1] CPU: 0 PID: 1 Comm: swapper Not tainted 5.16.0-11534-gc42ff46f97c1 #1 843f34bc11d03a69a557abb9c2a31a40f9711bc8
[ 188.005020][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 188.006289][ T1] Call Trace:
[ 188.006805][ T1] <TASK>
[ 188.007273][ T1] dump_stack_lvl (??:?)
[ 188.007965][ T1] dump_stack (??:?)
[ 188.008592][ T1] __register_sysctl_table (??:?)
[ 188.009381][ T1] ? ocfs2_stack_glue_init (stackglue.c:?)
To reproduce:
# build kernel
cd linux
cp config-5.16.0-11534-gc42ff46f97c1 .config
make HOSTCC=clang-14 CC=clang-14 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=clang-14 CC=clang-14 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Thanks,
Oliver Sang
On Fri, Jan 28, 2022 at 8:53 AM kernel test robot <[email protected]> wrote:
>
> commit: 46e33fd45a52bf03769906e64d8a8a1ab317777d ("ocfs2: simplify subdirectory
> registration with register_sysctl()")
Well, it's apparently commit c42ff46f97c1 ("ocfs2: simplify
subdirectory registration with register_sysctl()") in mainline now.
What worries me a bit is that the commit was auto-generated, and when
reading the commit message it reads as if it wasn't supposed to cause
any semantic changes at all.
Is the cause of this that 'nm' is supposed to be a directory, and
register_sysctl() doesn't handle directories?
I don't know this code at all, should it have been simplified even
further with something (TOTALLY UNTESTED) like the attached?
Linus
On Fri 28-01-22 10:00:29, Linus Torvalds wrote:
> On Fri, Jan 28, 2022 at 8:53 AM kernel test robot <[email protected]> wrote:
> >
> > commit: 46e33fd45a52bf03769906e64d8a8a1ab317777d ("ocfs2: simplify subdirectory
> > registration with register_sysctl()")
>
> Well, it's apparently commit c42ff46f97c1 ("ocfs2: simplify
> subdirectory registration with register_sysctl()") in mainline now.
>
> What worries me a bit is that the commit was auto-generated, and when
> reading the commit message it reads as if it wasn't supposed to cause
> any semantic changes at all.
>
> Is the cause of this that 'nm' is supposed to be a directory, and
> register_sysctl() doesn't handle directories?
>
> I don't know this code at all, should it have been simplified even
> further with something (TOTALLY UNTESTED) like the attached?
Yep, I've tested the patch and it fixes the failure for me. Feel free to
add:
Tested-by: Jan Kara <[email protected]>
Also the change makes sense to me as far as I'm reading register_sysctl()
so you can also add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
> index 731558a6f27d..dd77b7aaabf5 100644
> --- a/fs/ocfs2/stackglue.c
> +++ b/fs/ocfs2/stackglue.c
> @@ -661,17 +661,6 @@ static struct ctl_table ocfs2_nm_table[] = {
> { }
> };
>
> -static struct ctl_table ocfs2_mod_table[] = {
> - {
> - .procname = "nm",
> - .data = NULL,
> - .maxlen = 0,
> - .mode = 0555,
> - .child = ocfs2_nm_table
> - },
> - { }
> -};
> -
> static struct ctl_table_header *ocfs2_table_header;
>
> /*
> @@ -682,7 +671,7 @@ static int __init ocfs2_stack_glue_init(void)
> {
> strcpy(cluster_stack_name, OCFS2_STACK_PLUGIN_O2CB);
>
> - ocfs2_table_header = register_sysctl("fs/ocfs2", ocfs2_mod_table);
> + ocfs2_table_header = register_sysctl("fs/ocfs2/nm", ocfs2_nm_table);
> if (!ocfs2_table_header) {
> printk(KERN_ERR
> "ocfs2 stack glue: unable to register sysctl\n");
--
Jan Kara <[email protected]>
SUSE Labs, CR
Jan Kara <[email protected]> writes:
> On Fri 28-01-22 10:00:29, Linus Torvalds wrote:
>> On Fri, Jan 28, 2022 at 8:53 AM kernel test robot <[email protected]> wrote:
>> >
>> > commit: 46e33fd45a52bf03769906e64d8a8a1ab317777d ("ocfs2: simplify subdirectory
>> > registration with register_sysctl()")
>>
>> Well, it's apparently commit c42ff46f97c1 ("ocfs2: simplify
>> subdirectory registration with register_sysctl()") in mainline now.
>>
>> What worries me a bit is that the commit was auto-generated, and when
>> reading the commit message it reads as if it wasn't supposed to cause
>> any semantic changes at all.
>>
>> Is the cause of this that 'nm' is supposed to be a directory, and
>> register_sysctl() doesn't handle directories?
>>
>> I don't know this code at all, should it have been simplified even
>> further with something (TOTALLY UNTESTED) like the attached?
>
> Yep, I've tested the patch and it fixes the failure for me. Feel free to
> add:
>
> Tested-by: Jan Kara <[email protected]>
>
> Also the change makes sense to me as far as I'm reading register_sysctl()
> so you can also add:
>
> Reviewed-by: Jan Kara <[email protected]>
Yes. There is a register_sysctl_paths that can be used if you want/need
the embedded directories. That probably would have been a better
choice for an automated conversion.
But since this there is only the single file in a single directory
register_sysctl() with the full path is perfectly fine in this case.
Reviewed-by: "Eric W. Biederman" <[email protected]>
>> diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
>> index 731558a6f27d..dd77b7aaabf5 100644
>> --- a/fs/ocfs2/stackglue.c
>> +++ b/fs/ocfs2/stackglue.c
>> @@ -661,17 +661,6 @@ static struct ctl_table ocfs2_nm_table[] = {
>> { }
>> };
>>
>> -static struct ctl_table ocfs2_mod_table[] = {
>> - {
>> - .procname = "nm",
>> - .data = NULL,
>> - .maxlen = 0,
>> - .mode = 0555,
>> - .child = ocfs2_nm_table
>> - },
>> - { }
>> -};
>> -
>> static struct ctl_table_header *ocfs2_table_header;
>>
>> /*
>> @@ -682,7 +671,7 @@ static int __init ocfs2_stack_glue_init(void)
>> {
>> strcpy(cluster_stack_name, OCFS2_STACK_PLUGIN_O2CB);
>>
>> - ocfs2_table_header = register_sysctl("fs/ocfs2", ocfs2_mod_table);
>> + ocfs2_table_header = register_sysctl("fs/ocfs2/nm", ocfs2_nm_table);
>> if (!ocfs2_table_header) {
>> printk(KERN_ERR
>> "ocfs2 stack glue: unable to register sysctl\n");
On Fri, Jan 28, 2022 at 6:49 PM Eric W. Biederman <[email protected]> wrote:
>
> Yes. There is a register_sysctl_paths that can be used if you want/need
> the embedded directories. That probably would have been a better
> choice for an automated conversion.
>
> But since this there is only the single file in a single directory
> register_sysctl() with the full path is perfectly fine in this case.
>
> Reviewed-by: "Eric W. Biederman" <[email protected]>
I was doing filesystem pull requests anyway, and as a result (I tend
to group things by topic if there's multiple things pending) I already
committed that patch of mine based on Jan's reviewed/tested-by, so
this reviewed-by ended up not in the tree.
But maybe somebody should check the other automated conversions for
the same issue? Hint hint.
Also, I'm somewhat unhappy about the fact that apparently the kernel
test robot already found this issue back in November of last year, yet
it made it to mainline several months later without being fixed.
It's hard to tell from this link:
https://lists.01.org/hyperkitty/list/[email protected]/thread/KQ2F6TPJWMDVEXJM4WTUC4DU3EH3YJVT/
but it does look like that original report only made it to that lkp
list and not the actual people listed on the commit itself? That would
explain why the report was overlooked.
Kind of sad.
Linus
On Fri, Jan 28, 2022 at 07:16:21PM +0200, Linus Torvalds wrote:
> On Fri, Jan 28, 2022 at 6:49 PM Eric W. Biederman <[email protected]> wrote:
> >
> > Yes. There is a register_sysctl_paths that can be used if you want/need
> > the embedded directories. That probably would have been a better
> > choice for an automated conversion.
> >
> > But since this there is only the single file in a single directory
> > register_sysctl() with the full path is perfectly fine in this case.
> >
> > Reviewed-by: "Eric W. Biederman" <[email protected]>
>
> I was doing filesystem pull requests anyway, and as a result (I tend
> to group things by topic if there's multiple things pending) I already
> committed that patch of mine based on Jan's reviewed/tested-by, so
> this reviewed-by ended up not in the tree.
>
> But maybe somebody should check the other automated conversions for
> the same issue? Hint hint.
I just double checked and indeed, the issue was that ocfs had a path
underneath it, so indeed register_sysctl_paths() would have been better.
The other drivers which were converted do not have paths underneath so
they are safe.
> Also, I'm somewhat unhappy about the fact that apparently the kernel
> test robot already found this issue back in November of last year, yet
> it made it to mainline several months later without being fixed.
>
> It's hard to tell from this link:
>
> https://lists.01.org/hyperkitty/list/[email protected]/thread/KQ2F6TPJWMDVEXJM4WTUC4DU3EH3YJVT/
>
> but it does look like that original report only made it to that lkp
> list and not the actual people listed on the commit itself? That would
> explain why the report was overlooked.
Odd, no I got Cc'd on the email as I get 0day testing on all my branches
prior to pushing patches out. For some reason this failure fell through
the cracks. Sorry about that!
Luis