Re: [Kernel Bug] WARNING in vfat_rmdir

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Zhiyu Zhang <zhiyuzhang999@gmail.com> writes:

> Dear Linux kernel developers and maintainers,
>
> I’m sorry — the root cause analysis in my previous report was likely
> incorrect, and the patch I suggested there does not alleviate the
> issue after further testing, which means that the root cause is not on
> the errno passing.
>
> After adding debug prints in fat__get_entry(), I observed frequent
> cases of err=0, phys=0 at pos == i_size, which means the code is
> taking a "normal EOF" path (as decided by fat_bmap()) rather than
> hitting and swallowing a negative errno. As a result,
> fat_get_short_entry() still returns -ENOENT, fat_dir_empty() still
> returns 0, and the code path does not prevent drop_nlink(dir) from
> being executed even when the parent directory's i_nlink is already
> abnormal. In other words, the parent directory's i_nlink appears to be
> wrong/corrupted in the first place. Subsequent vfat_rmdir() calls then
> decrement the already-too-low link count, eventually reaching 0 and
> triggering WARN_ON(inode->i_nlink == 0) in drop_nlink() (and panicking
> if panic_on_warn is enabled).
>
> So please IGNORE my previous patch proposal. A conservative mitigation
> that I tested EFFECTIVE is similar to the UDF fix for corrupted parent
> link count handling (Jan Kara's WARNING in udf_rmdir fix:
> "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c5566903af56dd1abb092f18dcb0c770d6cd8dcb").

It can occur on a corrupted image that didn't correctly initialized as a
directory.

> static int vfat_rmdir(struct inode *dir, struct dentry *dentry)
>         err = fat_remove_entries(dir, &sinfo);  /* and releases bh */
>         if (err)
>                 goto out;
> -       drop_nlink(dir);
> +       if (dir->i_nlink >= 3)
> +               drop_nlink(dir);
> +       else
> +               fat_fs_error_ratelimit(sb, "parent dir link count too
> low (%u)\n",
> +                       dir->i_nlink);

Looks good sanity check. However, I think it doesn't need
_ratelimit. And this should be done in msdos_namei.c too.

And fat_fs_error() should remove last "\n", and please add {} for 2
lines code (it is not necessary as c though, sorry for kind of my
preference), also looks like no "sb" here, need testing before actually
submitting the patch.

	else {
		fat_fs_error(sb, "parent dir link count too low (%u)",
			     dir->i_nlink);
	}

Thanks.

>         clear_nlink(inode);
>         fat_truncate_time(inode, NULL, S_ATIME|S_MTIME);
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>