From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
To: Krystian Kaniewski <krystianmkaniewski@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
syzbot+b0aebd03565f5774f7f8@syzkaller.appspotmail.com
Subject: Re: [PATCH] fat: validate dotdot buffers in VFAT and MSDOS rename and rollback
Date: Wed, 30 Sep 2026 17:02:30 +0900 [thread overview]
Message-ID: <87ld8j4161.fsf@mail.parknet.co.jp> (raw)
In-Reply-To: <20260929090029.742579-1-krystianmkaniewski@gmail.com>
Krystian Kaniewski <krystianmkaniewski@gmail.com> writes:
> During cross-directory rename operations with synchronous directory updates
> enabled in VFAT and MSDOS, updating the ".." directory entry writes the
> buffer via sync_dirty_buffer(). If this write fails due to an I/O error,
> the block layer clears the BH_Uptodate flag. When rename enters its error
> rollback path, it attempts to update the ".." directory entry again with
> the same buffer head, which calls mmb_mark_buffer_dirty() and triggers a
> "!buffer_uptodate(bh)" warning in mark_buffer_dirty().
>
> Fix this by introducing fat_update_dotdot_de() and
> fat_sync_update_dotdot_de() in fs/fat/dir.c, used by both VFAT and MSDOS
> cross-directory rename and rollback paths. The helpers lock the buffer head
> and check buffer_uptodate() before modifying the entry. If the buffer is
> not uptodate, unlock it and return -EIO, preventing mmb_mark_buffer_dirty()
> from being called on a non-uptodate buffer. fat_sync_update_dotdot_de()
> preserves the unconditional buffer sync in the MSDOS rename rollback path.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Assisted-by: Gemini:gemini-3.8-flash syzbot
> Reported-by: syzbot+b0aebd03565f5774f7f8@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=b0aebd03565f5774f7f8
> Link: https://syzkaller.appspot.com/ai_job?id=f915c371-eb83-48b9-af21-45cf1fd21ba0
> Signed-off-by: Krystian Kaniewski <krystianmkaniewski@gmail.com>
>
> ---
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 35bdb6294..cee06e635 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -941,6 +941,40 @@ int fat_get_dotdot_entry(struct inode *dir, struct buffer_head **bh,
> }
> EXPORT_SYMBOL_GPL(fat_get_dotdot_entry);
>
> +static int __fat_update_dotdot_de(struct inode *dir, struct inode *inode,
> + struct buffer_head *dotdot_bh,
> + struct msdos_dir_entry *dotdot_de,
> + bool force_sync)
> +{
> + lock_buffer(dotdot_bh);
This looks like unnecessarily wait the completion of buffer I/O, isn't
it? I guess, it is ok to give up to revert if surely I/O error, because
the reverted buffer will be the I/O error again.
Thanks.
> + if (!buffer_uptodate(dotdot_bh)) {
> + unlock_buffer(dotdot_bh);
> + return -EIO;
> + }
> + fat_set_start(dotdot_de, MSDOS_I(dir)->i_logstart);
> + mmb_mark_buffer_dirty(dotdot_bh, &MSDOS_I(inode)->i_metadata_bhs);
> + unlock_buffer(dotdot_bh);
> + if (force_sync || IS_DIRSYNC(dir))
> + return sync_dirty_buffer(dotdot_bh);
> + return 0;
> +}
> +
> +int fat_update_dotdot_de(struct inode *dir, struct inode *inode,
> + struct buffer_head *dotdot_bh,
> + struct msdos_dir_entry *dotdot_de)
> +{
> + return __fat_update_dotdot_de(dir, inode, dotdot_bh, dotdot_de, false);
> +}
> +EXPORT_SYMBOL_GPL(fat_update_dotdot_de);
> +
> +int fat_sync_update_dotdot_de(struct inode *dir, struct inode *inode,
> + struct buffer_head *dotdot_bh,
> + struct msdos_dir_entry *dotdot_de)
> +{
> + return __fat_update_dotdot_de(dir, inode, dotdot_bh, dotdot_de, true);
> +}
> +EXPORT_SYMBOL_GPL(fat_sync_update_dotdot_de);
> +
> /* See if directory is empty */
> int fat_dir_empty(struct inode *dir)
> {
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 61338413d..d51d3c11e 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -339,6 +339,12 @@ extern int fat_scan_logstart(struct inode *dir, int i_logstart,
> struct fat_slot_info *sinfo);
> extern int fat_get_dotdot_entry(struct inode *dir, struct buffer_head **bh,
> struct msdos_dir_entry **de);
> +extern int fat_update_dotdot_de(struct inode *dir, struct inode *inode,
> + struct buffer_head *dotdot_bh,
> + struct msdos_dir_entry *dotdot_de);
> +extern int fat_sync_update_dotdot_de(struct inode *dir, struct inode *inode,
> + struct buffer_head *dotdot_bh,
> + struct msdos_dir_entry *dotdot_de);
> extern int fat_alloc_new_dir(struct inode *dir, struct timespec64 *ts);
> extern int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
> struct fat_slot_info *sinfo);
> diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
> index d46d1a385..31faaeae8 100644
> --- a/fs/fat/namei_msdos.c
> +++ b/fs/fat/namei_msdos.c
> @@ -527,14 +527,10 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
> }
>
> if (update_dotdot) {
> - fat_set_start(dotdot_de, MSDOS_I(new_dir)->i_logstart);
> - mmb_mark_buffer_dirty(dotdot_bh,
> - &MSDOS_I(old_inode)->i_metadata_bhs);
> - if (IS_DIRSYNC(new_dir)) {
> - err = sync_dirty_buffer(dotdot_bh);
> - if (err)
> - goto error_dotdot;
> - }
> + err = fat_update_dotdot_de(new_dir, old_inode, dotdot_bh,
> + dotdot_de);
> + if (err)
> + goto error_dotdot;
> drop_nlink(old_dir);
> if (!new_inode)
> inc_nlink(new_dir);
> @@ -565,12 +561,9 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
> /* data cluster is shared, serious corruption */
> corrupt = 1;
>
> - if (update_dotdot) {
> - fat_set_start(dotdot_de, MSDOS_I(old_dir)->i_logstart);
> - mmb_mark_buffer_dirty(dotdot_bh,
> - &MSDOS_I(old_inode)->i_metadata_bhs);
> - corrupt |= sync_dirty_buffer(dotdot_bh);
> - }
> + if (update_dotdot)
> + corrupt |= fat_sync_update_dotdot_de(old_dir, old_inode,
> + dotdot_bh, dotdot_de);
> error_inode:
> fat_detach(old_inode);
> fat_attach(old_inode, old_sinfo.i_pos);
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index da3e89c0b..56da78455 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -909,16 +909,6 @@ static int vfat_sync_ipos(struct inode *dir, struct inode *inode)
> return 0;
> }
>
> -static int vfat_update_dotdot_de(struct inode *dir, struct inode *inode,
> - struct buffer_head *dotdot_bh,
> - struct msdos_dir_entry *dotdot_de)
> -{
> - fat_set_start(dotdot_de, MSDOS_I(dir)->i_logstart);
> - mmb_mark_buffer_dirty(dotdot_bh, &MSDOS_I(inode)->i_metadata_bhs);
> - if (IS_DIRSYNC(dir))
> - return sync_dirty_buffer(dotdot_bh);
> - return 0;
> -}
>
> static void vfat_update_dir_metadata(struct inode *dir, struct timespec64 *ts)
> {
> @@ -981,8 +971,8 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
> goto error_inode;
>
> if (dotdot_de) {
> - err = vfat_update_dotdot_de(new_dir, old_inode, dotdot_bh,
> - dotdot_de);
> + err = fat_update_dotdot_de(new_dir, old_inode, dotdot_bh,
> + dotdot_de);
> if (err)
> goto error_dotdot;
> drop_nlink(old_dir);
> @@ -1014,8 +1004,8 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
> corrupt = 1;
>
> if (dotdot_de) {
> - corrupt |= vfat_update_dotdot_de(old_dir, old_inode, dotdot_bh,
> - dotdot_de);
> + corrupt |= fat_update_dotdot_de(old_dir, old_inode, dotdot_bh,
> + dotdot_de);
> }
> error_inode:
> fat_detach(old_inode);
> @@ -1103,14 +1093,14 @@ static int vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry
>
> /* update ".." directory entry info */
> if (old_dotdot_de) {
> - err = vfat_update_dotdot_de(new_dir, old_inode, old_dotdot_bh,
> - old_dotdot_de);
> + err = fat_update_dotdot_de(new_dir, old_inode, old_dotdot_bh,
> + old_dotdot_de);
> if (err)
> goto error_old_dotdot;
> }
> if (new_dotdot_de) {
> - err = vfat_update_dotdot_de(old_dir, new_inode, new_dotdot_bh,
> - new_dotdot_de);
> + err = fat_update_dotdot_de(old_dir, new_inode, new_dotdot_bh,
> + new_dotdot_de);
> if (err)
> goto error_new_dotdot;
> }
> @@ -1137,14 +1127,14 @@ static int vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry
>
> error_new_dotdot:
> if (new_dotdot_de) {
> - corrupt |= vfat_update_dotdot_de(new_dir, new_inode,
> - new_dotdot_bh, new_dotdot_de);
> + corrupt |= fat_update_dotdot_de(new_dir, new_inode,
> + new_dotdot_bh, new_dotdot_de);
> }
>
> error_old_dotdot:
> if (old_dotdot_de) {
> - corrupt |= vfat_update_dotdot_de(old_dir, old_inode,
> - old_dotdot_bh, old_dotdot_de);
> + corrupt |= fat_update_dotdot_de(old_dir, old_inode,
> + old_dotdot_bh, old_dotdot_de);
> }
>
> error_exchange:
>
> base-commit: 93f51579e7df248780214094418f205253383cc5
>
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
next prev parent reply other threads:[~2026-09-30 8:12 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-14 16:38 [syzbot] [exfat?] WARNING in vfat_update_dotdot_de syzbot
2026-09-29 9:00 ` [PATCH] fat: validate dotdot buffers in VFAT and MSDOS rename and rollback Krystian Kaniewski
2026-09-30 8:02 ` OGAWA Hirofumi [this message]
2026-09-30 12:17 ` Krystian Kaniewski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ld8j4161.fsf@mail.parknet.co.jp \
--to=hirofumi@mail.parknet.co.jp \
--cc=krystianmkaniewski@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=syzbot+b0aebd03565f5774f7f8@syzkaller.appspotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®