WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[Vegard Nossum]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

Hi,

With the attached vfat disk image (fuzzed), I get the following WARNING:

WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()
CPU: 0 PID: 913 Comm: a.out Not tainted 4.2.5+ #39
Stack:
  e0931b50 60075412 e13981e8 00000009
  00000000 605cc684 e0931b60 605cf637
  e0931bc0 60040f6d e0931ba0 6011e12b
Call Trace:
  [<60029f3b>] show_stack+0xdb/0x1a0
  [<605cf637>] dump_stack+0x2a/0x2c
  [<60040f6d>] warn_slowpath_common+0x9d/0xf0
  [<6004114c>] warn_slowpath_null+0x1c/0x20
  [<6011e12b>] drop_nlink+0x4b/0x50
  [<601e9e2f>] vfat_rename+0x56f/0x800
  [<60113dd2>] vfs_rename+0x9a2/0x9d0
  [<60114439>] SyS_renameat2+0x639/0x690
  [<601144d0>] SyS_rename+0x20/0x30
  [<6002c5ce>] handle_syscall+0x6e/0xa0
  [<6003a911>] userspace+0x4f1/0x5e0

To trigger it, you have to do a rename("/mnt/a/b/1", "/mnt/1"), where
/mnt is your mountpoint.

Also happens on 3.13.0 Ubuntu trusty kernel.


Vegard

[-- Attachment #2: vfat2.img.bz2 --]
[-- Type: application/x-bzip, Size: 615 bytes --]

Re: WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[Richard Weinberger]

Am 23.11.2015 um 08:55 schrieb Vegard Nossum:
> Hi,
> 
> With the attached vfat disk image (fuzzed), I get the following WARNING:
> 
> WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()
> CPU: 0 PID: 913 Comm: a.out Not tainted 4.2.5+ #39
> Stack:
>  e0931b50 60075412 e13981e8 00000009
>  00000000 605cc684 e0931b60 605cf637
>  e0931bc0 60040f6d e0931ba0 6011e12b
> Call Trace:
>  [<60029f3b>] show_stack+0xdb/0x1a0
>  [<605cf637>] dump_stack+0x2a/0x2c
>  [<60040f6d>] warn_slowpath_common+0x9d/0xf0
>  [<6004114c>] warn_slowpath_null+0x1c/0x20
>  [<6011e12b>] drop_nlink+0x4b/0x50
>  [<601e9e2f>] vfat_rename+0x56f/0x800
>  [<60113dd2>] vfs_rename+0x9a2/0x9d0
>  [<60114439>] SyS_renameat2+0x639/0x690
>  [<601144d0>] SyS_rename+0x20/0x30
>  [<6002c5ce>] handle_syscall+0x6e/0xa0
>  [<6003a911>] userspace+0x4f1/0x5e0
> 
> To trigger it, you have to do a rename("/mnt/a/b/1", "/mnt/1"), where
> /mnt is your mountpoint.

Not here. All I get is:
FAT-fs (ubdb): Corrupted directory (i_pos 244)

Did you something before the rename()?

Thanks,
//richard

Re: WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[Vegard Nossum]

On 11/23/2015 11:21 PM, Richard Weinberger wrote:
> Am 23.11.2015 um 08:55 schrieb Vegard Nossum:
>> With the attached vfat disk image (fuzzed), I get the following WARNING:
>>
>> WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()
[...]
>>
>> To trigger it, you have to do a rename("/mnt/a/b/1", "/mnt/1"), where
>> /mnt is your mountpoint.
>
> Not here. All I get is:
> FAT-fs (ubdb): Corrupted directory (i_pos 244)
>
> Did you something before the rename()?

No, nothing before the rename.

Did you use mv to generate the rename()? Then you may have to do 'mv
/mnt/a/b/1 /mnt/', otherwise it ends up doing rename("/mnt/a/b/1",
"/mnt/1/1") which only shows the message you saw. Let me know if this helps.

Thanks for having a look,


Vegard

Re: WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[OGAWA Hirofumi]

Vegard Nossum <vegard.nossum@oracle.com> writes:

> On 11/23/2015 11:21 PM, Richard Weinberger wrote:
>> Am 23.11.2015 um 08:55 schrieb Vegard Nossum:
>>> With the attached vfat disk image (fuzzed), I get the following WARNING:
>>>
>>> WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()
> [...]
>>>
>>> To trigger it, you have to do a rename("/mnt/a/b/1", "/mnt/1"), where
>>> /mnt is your mountpoint.
>>
>> Not here. All I get is:
>> FAT-fs (ubdb): Corrupted directory (i_pos 244)
>>
>> Did you something before the rename()?
>
> No, nothing before the rename.
>
> Did you use mv to generate the rename()? Then you may have to do 'mv
> /mnt/a/b/1 /mnt/', otherwise it ends up doing rename("/mnt/a/b/1",
> "/mnt/1/1") which only shows the message you saw. Let me know if this helps.
>
> Thanks for having a look,

Can you try this one?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


[PATCH] fat: Add simple validation for directory inode


This detects simple corruption cases of directory, and try to avoid
further damage to user data.

And performance impact of this validation should be very low, or not
measurable.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/inode.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff -puN fs/fat/inode.c~fat-validate-dir fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-validate-dir	2015-11-26 06:31:39.666959958 +0900
+++ linux-hirofumi/fs/fat/inode.c	2015-11-26 06:31:39.670959945 +0900
@@ -449,6 +449,24 @@ static int fat_calc_dir_size(struct inod
 	return 0;
 }
 
+static int fat_validate_dir(struct inode *dir)
+{
+	struct super_block *sb = dir->i_sb;
+
+	if (dir->i_nlink < 2) {
+		/* Directory should have "."/".." entries at least. */
+		fat_fs_error(sb, "corrupted directory (invalid entries)");
+		return -EIO;
+	}
+	if (MSDOS_I(dir)->i_start == 0 ||
+	    MSDOS_I(dir)->i_start == MSDOS_SB(sb)->root_cluster) {
+		/* Directory should point valid cluster. */
+		fat_fs_error(sb, "corrupted directory (invalid i_start)");
+		return -EIO;
+	}
+	return 0;
+}
+
 /* doesn't deal with root inode */
 int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
 {
@@ -475,6 +493,10 @@ int fat_fill_inode(struct inode *inode,
 		MSDOS_I(inode)->mmu_private = inode->i_size;
 
 		set_nlink(inode, fat_subdirs(inode));
+
+		error = fat_validate_dir(inode);
+		if (error < 0)
+			return error;
 	} else { /* not a directory */
 		inode->i_generation |= 1;
 		inode->i_mode = fat_make_mode(sbi, de->attr,
_

Re: WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[Vegard Nossum]

On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
> Vegard Nossum <vegard.nossum@oracle.com> writes:
>
>> On 11/23/2015 11:21 PM, Richard Weinberger wrote:
>>> Am 23.11.2015 um 08:55 schrieb Vegard Nossum:
>>>> With the attached vfat disk image (fuzzed), I get the following WARNING:
>>>>
>>>> WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[...]

>
> Can you try this one?
>

That seems to fix the problem here, thanks!

The last potential issue I'm seeing (completely unrelated to your patch) 
is this:

[  340.610000] VFS: Lookup of '1' in vfat loop0 would have caused loop
[  354.360000] d_splice_alias: 1104 callbacks suppressed
[  354.360000] VFS: Lookup of '1' in vfat loop0 would have caused loop

Is that worth investigating closer?

Thanks,


Vegard

Re: WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[OGAWA Hirofumi]

Vegard Nossum <vegard.nossum@oracle.com> writes:

> On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
>> Vegard Nossum <vegard.nossum@oracle.com> writes:
>>
>>> On 11/23/2015 11:21 PM, Richard Weinberger wrote:
>>>> Am 23.11.2015 um 08:55 schrieb Vegard Nossum:
>>>>> With the attached vfat disk image (fuzzed), I get the following WARNING:
>>>>>
>>>>> WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()
>
> [...]
>
>>
>> Can you try this one?
>>
>
> That seems to fix the problem here, thanks!
>
> The last potential issue I'm seeing (completely unrelated to your patch) 
> is this:
>
> [  340.610000] VFS: Lookup of '1' in vfat loop0 would have caused loop
> [  354.360000] d_splice_alias: 1104 callbacks suppressed
> [  354.360000] VFS: Lookup of '1' in vfat loop0 would have caused loop
>
> Is that worth investigating closer?

It looks like corruption detected with ratelimited printk (at vfs
level. dir is hardlink of ancestor). IOW, it looks like intended
behavior.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[OGAWA Hirofumi]

Vegard Nossum <vegard.nossum@oracle.com> writes:

> On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
>> Vegard Nossum <vegard.nossum@oracle.com> writes:
>>
>>> On 11/23/2015 11:21 PM, Richard Weinberger wrote:
>>>> Am 23.11.2015 um 08:55 schrieb Vegard Nossum:
>>>>> With the attached vfat disk image (fuzzed), I get the following WARNING:
>>>>>
>>>>> WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()
>
> [...]
>
>>
>> Can you try this one?
>>
>
> That seems to fix the problem here, thanks!

Andrew, please queue this up for next chance.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


[PATCH] fat: Add simple validation for directory inode


This detects simple corruption cases of directory, and try to avoid
further damage to user data.

And performance impact of this validation should be very low, or not
measurable.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Tested-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/inode.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff -puN fs/fat/inode.c~fat-validate-dir fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-validate-dir	2015-11-26 06:31:39.666959958 +0900
+++ linux-hirofumi/fs/fat/inode.c	2015-11-26 06:31:39.670959945 +0900
@@ -449,6 +449,24 @@ static int fat_calc_dir_size(struct inod
 	return 0;
 }
 
+static int fat_validate_dir(struct inode *dir)
+{
+	struct super_block *sb = dir->i_sb;
+
+	if (dir->i_nlink < 2) {
+		/* Directory should have "."/".." entries at least. */
+		fat_fs_error(sb, "corrupted directory (invalid entries)");
+		return -EIO;
+	}
+	if (MSDOS_I(dir)->i_start == 0 ||
+	    MSDOS_I(dir)->i_start == MSDOS_SB(sb)->root_cluster) {
+		/* Directory should point valid cluster. */
+		fat_fs_error(sb, "corrupted directory (invalid i_start)");
+		return -EIO;
+	}
+	return 0;
+}
+
 /* doesn't deal with root inode */
 int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
 {
@@ -475,6 +493,10 @@ int fat_fill_inode(struct inode *inode,
 		MSDOS_I(inode)->mmu_private = inode->i_size;
 
 		set_nlink(inode, fat_subdirs(inode));
+
+		error = fat_validate_dir(inode);
+		if (error < 0)
+			return error;
 	} else { /* not a directory */
 		inode->i_generation |= 1;
 		inode->i_mode = fat_make_mode(sbi, de->attr,
_

Re: WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[Vegard Nossum]

On 11/26/2015 09:30 AM, OGAWA Hirofumi wrote:
> Vegard Nossum <vegard.nossum@oracle.com> writes:
>> On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
>>> Vegard Nossum <vegard.nossum@oracle.com> writes:
>>>> On 11/23/2015 11:21 PM, Richard Weinberger wrote:
>>>>> Am 23.11.2015 um 08:55 schrieb Vegard Nossum:
>>>>>> With the attached vfat disk image (fuzzed), I get the
>>>>>> following WARNING:
>>>>>>
>>>>>> WARNING: CPU: 0 PID: 913 at fs/inode.c:275
>>>>>> drop_nlink+0x4b/0x50()
>> [...]
>>> Can you try this one?
>> That seems to fix the problem here, thanks!
> Andrew, please queue this up for next chance.

Sorry to bug you, I didn't see this merged yet and just making sure this
doesn't slip through the cracks. Or should I expect it only for the next
merge window?


Vegard

Re: WARNING: CPU: 0 PID: 913 at fs/inode.c:275 drop_nlink+0x4b/0x50()

[Andrew Morton]

On Sun, 13 Dec 2015 23:19:31 +0100 Vegard Nossum <vegard.nossum@oracle.com> wrote:

> On 11/26/2015 09:30 AM, OGAWA Hirofumi wrote:
> > Vegard Nossum <vegard.nossum@oracle.com> writes:
> >> On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
> >>> Vegard Nossum <vegard.nossum@oracle.com> writes:
> >>>> On 11/23/2015 11:21 PM, Richard Weinberger wrote:
> >>>>> Am 23.11.2015 um 08:55 schrieb Vegard Nossum:
> >>>>>> With the attached vfat disk image (fuzzed), I get the
> >>>>>> following WARNING:
> >>>>>>
> >>>>>> WARNING: CPU: 0 PID: 913 at fs/inode.c:275
> >>>>>> drop_nlink+0x4b/0x50()
> >> [...]
> >>> Can you try this one?
> >> That seems to fix the problem here, thanks!
> > Andrew, please queue this up for next chance.
> 
> Sorry to bug you, I didn't see this merged yet and just making sure this
> doesn't slip through the cracks. Or should I expect it only for the next
> merge window?

I merged this into -mm on Nov 11.  I'd assumed it was 4.5 material.  Was
that wrong?