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: 6+ 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
2026-10-02 13:43 ` [PATCH v2] fat: skip rename rollback on buffers with failed I/O Krystian Kaniewski
2026-10-11 18:36 ` OGAWA Hirofumi
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.