Re: [ide] ec7d9c9ce8: WARNING:at_fs/proc/generic.c:#remove_proc_entry

Re: [ide] ec7d9c9ce8: WARNING:at_fs/proc/generic.c:#remove_proc_entry

[Linus Torvalds]

On Thu, Dec 20, 2018 at 1:19 AM kernel test robot <lkp@intel.com> wrote:
>
> FYI, we noticed the following commit (built with gcc-7):
>
> commit: ec7d9c9ce897174243af4fcd201dbfc34df0f3a3 ("ide: replace ->proc_fops with ->proc_show")
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master

Funky.  How did the kernel test robot suddenly figure out an 8-month
old problem?

> [   44.180514] WARNING: CPU: 1 PID: 165 at fs/proc/generic.c:662 remove_proc_entry+0xb9/0x155

This is a warning for somebody doing "remove_proc_entry() on a name
that doesn't actually exist in that /proc directory.

In this case, it does seem to be due to the named commit adding a

+               remove_proc_entry("settings", drive->proc);

to ide_proc_unregister_device(), and looking at the patch I get the
feeling that it's due to a typo: the code *creates* the file called
"setting", but removes the file "settings". Note the missing "s" at
creation time.

And yes, the name of the /proc file _should_be "settings", judging by
the rest of the patch.

So it does seem to be a real bug. Nobody noticed until now? Why did
the test robot suddenly react to it?

              Linus

Re: [ide] ec7d9c9ce8: WARNING:at_fs/proc/generic.c:#remove_proc_entry

[Jens Axboe]

On 12/20/18 9:05 AM, Linus Torvalds wrote:
> On Thu, Dec 20, 2018 at 1:19 AM kernel test robot <lkp@intel.com> wrote:
>>
>> FYI, we noticed the following commit (built with gcc-7):
>>
>> commit: ec7d9c9ce897174243af4fcd201dbfc34df0f3a3 ("ide: replace ->proc_fops with ->proc_show")
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
> 
> Funky.  How did the kernel test robot suddenly figure out an 8-month
> old problem?
> 
>> [   44.180514] WARNING: CPU: 1 PID: 165 at fs/proc/generic.c:662 remove_proc_entry+0xb9/0x155
> 
> This is a warning for somebody doing "remove_proc_entry() on a name
> that doesn't actually exist in that /proc directory.
> 
> In this case, it does seem to be due to the named commit adding a
> 
> +               remove_proc_entry("settings", drive->proc);
> 
> to ide_proc_unregister_device(), and looking at the patch I get the
> feeling that it's due to a typo: the code *creates* the file called
> "setting", but removes the file "settings". Note the missing "s" at
> creation time.
> 
> And yes, the name of the /proc file _should_be "settings", judging by
> the rest of the patch.
> 
> So it does seem to be a real bug. Nobody noticed until now? Why did
> the test robot suddenly react to it?

Maybe the the tiny subset of IDE users don't actually have the proc
stuff enabled? A few months ago I did plenty of IDE testing with the
MQ conversion, but I never saw anything like this. I'm guessing that
I, too, did not have IDE_PROC_FS enabled.

Christoph, do you want to post the one-liner fix for this one?

-- 
Jens Axboe

Re: [ide] ec7d9c9ce8: WARNING:at_fs/proc/generic.c:#remove_proc_entry

[Christoph Hellwig]

On Thu, Dec 20, 2018 at 09:14:50AM -0700, Jens Axboe wrote:
> Maybe the the tiny subset of IDE users don't actually have the proc
> stuff enabled? A few months ago I did plenty of IDE testing with the
> MQ conversion, but I never saw anything like this. I'm guessing that
> I, too, did not have IDE_PROC_FS enabled.

Or the tiny subset of ide users basically doesn't exist and the
few platforms that use ide are basically bitrotting?

> Christoph, do you want to post the one-liner fix for this one?

See below:

--
>From c3550c617ffc48079fe6364a4fa3c7f75a09028b Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 20 Dec 2018 17:16:53 +0100
Subject: ide: fix a typo in the settings proc file name

Fixes: ec7d9c9ce8 ("ide: replace ->proc_fops with ->proc_show")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ide/ide-proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ide/ide-proc.c b/drivers/ide/ide-proc.c
index 45c997430332..0e51803de0e7 100644
--- a/drivers/ide/ide-proc.c
+++ b/drivers/ide/ide-proc.c
@@ -544,7 +544,7 @@ void ide_proc_port_register_devices(ide_hwif_t *hwif)
 		drive->proc = proc_mkdir(drive->name, parent);
 		if (drive->proc) {
 			ide_add_proc_entries(drive->proc, generic_drive_entries, drive);
-			proc_create_data("setting", S_IFREG|S_IRUSR|S_IWUSR,
+			proc_create_data("settings", S_IFREG|S_IRUSR|S_IWUSR,
 					drive->proc, &ide_settings_proc_fops,
 					drive);
 		}
-- 
2.19.2

Re: [LKP] [ide] ec7d9c9ce8: WARNING:at_fs/proc/generic.c:#remove_proc_entry

[Philip Li]

On Thu, Dec 20, 2018 at 08:05:28AM -0800, Linus Torvalds wrote:
> On Thu, Dec 20, 2018 at 1:19 AM kernel test robot <lkp@intel.com> wrote:
> >
> > FYI, we noticed the following commit (built with gcc-7):
> >
> > commit: ec7d9c9ce897174243af4fcd201dbfc34df0f3a3 ("ide: replace ->proc_fops with ->proc_show")
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
> 
> Funky.  How did the kernel test robot suddenly figure out an 8-month
> old problem?
Hi Linus, sorry for this late report. I can't figure out exact reason but some possible clues.

The issue is captured by rcutoture which doesn't work well before. And only from late october,
we have solved a few issues of the execution including the rootfs (yocto) it is using. And this issue
is against an randconfig, i'm not sure whether the issue depends on a certain kconfig thus only
be triggered or successfully bisected this time.

> 
> > [   44.180514] WARNING: CPU: 1 PID: 165 at fs/proc/generic.c:662 remove_proc_entry+0xb9/0x155
> 
> This is a warning for somebody doing "remove_proc_entry() on a name
> that doesn't actually exist in that /proc directory.
> 
> In this case, it does seem to be due to the named commit adding a
> 
> +               remove_proc_entry("settings", drive->proc);
> 
> to ide_proc_unregister_device(), and looking at the patch I get the
> feeling that it's due to a typo: the code *creates* the file called
> "setting", but removes the file "settings". Note the missing "s" at
> creation time.
> 
> And yes, the name of the /proc file _should_be "settings", judging by
> the rest of the patch.
> 
> So it does seem to be a real bug. Nobody noticed until now? Why did
> the test robot suddenly react to it?
> 
>               Linus
> _______________________________________________
> LKP mailing list
> LKP@lists.01.org
> https://lists.01.org/mailman/listinfo/lkp