Re: [PATCH] fat: roll back the new entry when create/mkdir fails to build the inode

[OGAWA Hirofumi]

zhoumin <teczm@xxxxxxxxxxx> writes:

> vfat_create(), vfat_mkdir(), msdos_create() and msdos_mkdir() add the
> on-disk entry before calling fat_build_inode() to set up the VFS
> inode.  If fat_build_inode() fails, the error is returned but the
> entry stays on disk and the file or directory shows up after remount.
>
> Remove the entry in that case.  For mkdir, also free the directory
> cluster and decrement the parent's link count.
>
> Signed-off-by: zhoumin <teczm@xxxxxxxxxxx>
> ---
>  fs/fat/namei_msdos.c | 19 ++++++++++++++-----
>  fs/fat/namei_vfat.c  | 19 ++++++++++++++-----
>  2 files changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
> index d46d1a3851f2..a0d1af5b8a5b 100644
> --- a/fs/fat/namei_msdos.c
> +++ b/fs/fat/namei_msdos.c
> @@ -271,7 +271,7 @@ static int msdos_create(struct mnt_idmap *idmap, struct inode *dir,
>  	struct fat_slot_info sinfo;
>  	struct timespec64 ts;
>  	unsigned char msdos_name[MSDOS_NAME];
> -	int err, is_hid;
> +	int err, err2, is_hid;
>  
>  	mutex_lock(&MSDOS_SB(sb)->s_lock);
>  
> @@ -292,11 +292,15 @@ static int msdos_create(struct mnt_idmap *idmap, struct inode *dir,
>  	if (err)
>  		goto out;
>  	inode = fat_build_inode(sb, sinfo.de, sinfo.i_pos);
> -	brelse(sinfo.bh);
>  	if (IS_ERR(inode)) {
>  		err = PTR_ERR(inode);
> +		err2 = fat_remove_entries(dir, &sinfo);
> +		if (err2)
> +			fat_msg(sb, KERN_WARNING,
> +				"failed to roll back new entry (%d)", err2);

		int err2 = fat_remove_entries(dir, &sinfo);
		if (err2)
			fat_msg(sb, KERN_WARNING,
				"failed to roll back new entry (%d)", err2);
		err = PTR_ERR(inode);

is better? (and other 3 parts are same).

>  		goto out;
>  	}
> +	brelse(sinfo.bh);
>  	fat_truncate_time(inode, &ts, FAT_UPDATE_ATIME | FAT_UPDATE_CMTIME);
>  	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
>  
> @@ -353,7 +357,7 @@ static struct dentry *msdos_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	struct inode *inode;
>  	unsigned char msdos_name[MSDOS_NAME];
>  	struct timespec64 ts;
> -	int err, is_hid, cluster;
> +	int err, err2, is_hid, cluster;
>  
>  	mutex_lock(&MSDOS_SB(sb)->s_lock);
>  
> @@ -381,12 +385,17 @@ static struct dentry *msdos_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	inc_nlink(dir);
>  
>  	inode = fat_build_inode(sb, sinfo.de, sinfo.i_pos);
> -	brelse(sinfo.bh);
>  	if (IS_ERR(inode)) {
>  		err = PTR_ERR(inode);
> -		/* the directory was completed, just return a error */
> +		err2 = fat_remove_entries(dir, &sinfo);
> +		if (err2)
> +			fat_msg(sb, KERN_WARNING,
> +				"failed to roll back new entry (%d)", err2);
> +		drop_nlink(dir);

ditto

> +		fat_free_clusters(dir, cluster);

Maybe "goto out_free" instead of fat_free_cluster()?

>  		goto out;
>  	}
> +	brelse(sinfo.bh);
>  	set_nlink(inode, 2);
>  	fat_truncate_time(inode, &ts, FAT_UPDATE_ATIME | FAT_UPDATE_CMTIME);
>  	/* timestamp is already written, so mark_inode_dirty() is unneeded. */
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index da3e89c0b16a..31d83de5beaa 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -760,7 +760,7 @@ static int vfat_create(struct mnt_idmap *idmap, struct inode *dir,
>  	struct inode *inode;
>  	struct fat_slot_info sinfo;
>  	struct timespec64 ts;
> -	int err;
> +	int err, err2;
>  
>  	mutex_lock(&MSDOS_SB(sb)->s_lock);
>  
> @@ -771,11 +771,15 @@ static int vfat_create(struct mnt_idmap *idmap, struct inode *dir,
>  	inode_inc_iversion(dir);
>  
>  	inode = fat_build_inode(sb, sinfo.de, sinfo.i_pos);
> -	brelse(sinfo.bh);
>  	if (IS_ERR(inode)) {
>  		err = PTR_ERR(inode);
> +		err2 = fat_remove_entries(dir, &sinfo);
> +		if (err2)
> +			fat_msg(sb, KERN_WARNING,
> +				"failed to roll back new entry (%d)", err2);

ditto

>  		goto out;
>  	}
> +	brelse(sinfo.bh);
>  	inode_inc_iversion(inode);
>  
>  	d_instantiate(dentry, inode);
> @@ -853,7 +857,7 @@ static struct dentry *vfat_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	struct inode *inode;
>  	struct fat_slot_info sinfo;
>  	struct timespec64 ts;
> -	int err, cluster;
> +	int err, err2, cluster;
>  
>  	mutex_lock(&MSDOS_SB(sb)->s_lock);
>  
> @@ -870,12 +874,17 @@ static struct dentry *vfat_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	inc_nlink(dir);
>  
>  	inode = fat_build_inode(sb, sinfo.de, sinfo.i_pos);
> -	brelse(sinfo.bh);
>  	if (IS_ERR(inode)) {
>  		err = PTR_ERR(inode);
> -		/* the directory was completed, just return a error */
> +		err2 = fat_remove_entries(dir, &sinfo);
> +		if (err2)
> +			fat_msg(sb, KERN_WARNING,
> +				"failed to roll back new entry (%d)", err2);
> +		drop_nlink(dir);

ditto

> +		fat_free_clusters(dir, cluster);

out_free?

>  		goto out;
>  	}
> +	brelse(sinfo.bh);
>  	inode_inc_iversion(inode);
>  	set_nlink(inode, 2);

Thanks.
-- 
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>