[PATCH] fat: serialize primary and mirror updates

[PATCH] fat: serialize primary and mirror updates

[Matthias Goergens]

__fat_alloc_clusters() drops fat_lock before syncing and mirroring
modified buffer heads. Another allocation can change the primary FAT
while an earlier allocation is copying it to the mirror.

fat_ent_write() also updates and mirrors without fat_lock. Take the lock
in both paths, preserving the existing post-unlock error cleanup.

The test detector reproduced a primary-FAT write during a mirror copy.
Three exact-current trials completed with no FAT conflicts and matching
mirrors.

Link: https://syzkaller.appspot.com/bug?extid=018e6f695ad9bccb182a
---
 fs/fat/fatent.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
index 2d10a815a579..5f2779c7f947 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -419,6 +419,7 @@ int fat_ent_write(struct inode *inode, struct fat_entry *fatent,
 	const struct fatent_operations *ops = MSDOS_SB(sb)->fatent_ops;
 	int err;
 
+	lock_fat(MSDOS_SB(sb));
 	ops->ent_put(fatent, new);
 	if (wait) {
 		err = fat_sync_bhs(fatent->bhs, fatent->nr_bhs);
@@ -429,7 +430,7 @@ int fat_ent_write(struct inode *inode, struct fat_entry *fatent,
 			 * would not have value to write again on I/O
 			 * error.
 			 */
-			return err;
+			goto out_unlock;
 		}
 	}
 
@@ -440,6 +441,8 @@ int fat_ent_write(struct inode *inode, struct fat_entry *fatent,
 		if (wait)
 			fat_sync_bhs(fatent->bhs, fatent->nr_bhs);
 	}
+out_unlock:
+	unlock_fat(MSDOS_SB(sb));
 	return err;
 }
 
@@ -582,7 +585,6 @@ static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluste
 	err = -ENOSPC;
 
 out:
-	unlock_fat(sbi);
 	mark_fsinfo_dirty(sb);
 	fatent_brelse(&fatent);
 	if (!err) {
@@ -591,6 +593,7 @@ static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluste
 		if (!err)
 			err = fat_mirror_bhs(sb, bhs, nr_bhs);
 	}
+	unlock_fat(sbi);
 	for (i = 0; i < nr_bhs; i++)
 		brelse(bhs[i]);
 

base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3
-- 
2.56.0

Re: [PATCH] fat: serialize primary and mirror updates

[OGAWA Hirofumi]

Matthias Goergens <matthias.goergens@gmail.com> writes:

> __fat_alloc_clusters() drops fat_lock before syncing and mirroring
> modified buffer heads. Another allocation can change the primary FAT
> while an earlier allocation is copying it to the mirror.
>
> fat_ent_write() also updates and mirrors without fat_lock. Take the lock
> in both paths, preserving the existing post-unlock error cleanup.
>
> The test detector reproduced a primary-FAT write during a mirror copy.
> Three exact-current trials completed with no FAT conflicts and matching
> mirrors.

What is the real issue?

> Link: https://syzkaller.appspot.com/bug?extid=018e6f695ad9bccb182a
> ---
>  fs/fat/fatent.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
> index 2d10a815a579..5f2779c7f947 100644
> --- a/fs/fat/fatent.c
> +++ b/fs/fat/fatent.c
> @@ -419,6 +419,7 @@ int fat_ent_write(struct inode *inode, struct fat_entry *fatent,
>  	const struct fatent_operations *ops = MSDOS_SB(sb)->fatent_ops;
>  	int err;
>  
> +	lock_fat(MSDOS_SB(sb));
>  	ops->ent_put(fatent, new);
>  	if (wait) {
>  		err = fat_sync_bhs(fatent->bhs, fatent->nr_bhs);
> @@ -429,7 +430,7 @@ int fat_ent_write(struct inode *inode, struct fat_entry *fatent,
>  			 * would not have value to write again on I/O
>  			 * error.
>  			 */
> -			return err;
> +			goto out_unlock;
>  		}
>  	}
>  
> @@ -440,6 +441,8 @@ int fat_ent_write(struct inode *inode, struct fat_entry *fatent,
>  		if (wait)
>  			fat_sync_bhs(fatent->bhs, fatent->nr_bhs);
>  	}
> +out_unlock:
> +	unlock_fat(MSDOS_SB(sb));
>  	return err;
>  }
>  
> @@ -582,7 +585,6 @@ static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluste
>  	err = -ENOSPC;
>  
>  out:
> -	unlock_fat(sbi);
>  	mark_fsinfo_dirty(sb);
>  	fatent_brelse(&fatent);
>  	if (!err) {
> @@ -591,6 +593,7 @@ static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluste
>  		if (!err)
>  			err = fat_mirror_bhs(sb, bhs, nr_bhs);
>  	}
> +	unlock_fat(sbi);
>  	for (i = 0; i < nr_bhs; i++)
>  		brelse(bhs[i]);
>  
>
> base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3

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