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
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
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
Vegard Nossum <[email protected]> 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 <[email protected]>
[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 <[email protected]>
---
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,
_
On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
> Vegard Nossum <[email protected]> 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
Vegard Nossum <[email protected]> writes:
> On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
>> Vegard Nossum <[email protected]> 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 <[email protected]>
Vegard Nossum <[email protected]> writes:
> On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
>> Vegard Nossum <[email protected]> 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 <[email protected]>
[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 <[email protected]>
Tested-by: Vegard Nossum <[email protected]>
Signed-off-by: OGAWA Hirofumi <[email protected]>
---
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,
_
On 11/26/2015 09:30 AM, OGAWA Hirofumi wrote:
> Vegard Nossum <[email protected]> writes:
>> On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
>>> Vegard Nossum <[email protected]> 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
On Sun, 13 Dec 2015 23:19:31 +0100 Vegard Nossum <[email protected]> wrote:
> On 11/26/2015 09:30 AM, OGAWA Hirofumi wrote:
> > Vegard Nossum <[email protected]> writes:
> >> On 11/25/2015 10:54 PM, OGAWA Hirofumi wrote:
> >>> Vegard Nossum <[email protected]> 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?