Re: [PATCH] fat: preserve reserved lcase bits across rename

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

Cihan Karadag <cihan.cihan@gmail.com> writes:

> The lcase field of struct msdos_dir_entry
> (include/uapi/linux/msdos_fs.h) has only two bits the kernel
> interprets, CASE_LOWER_BASE and CASE_LOWER_EXT; the rest are reserved
> and other implementations may use them to carry their own metadata
> (e.g. the Windows EFS "encrypted file" bit pattern).
>
> vfat_rename() currently loses those reserved bits in both of its
> paths: when the destination name doesn't already exist, a brand new
> directory entry is built from scratch with a freshly computed lcase
> byte, zeroing anything outside the two known bits; when the rename
> overwrites an existing destination, the destination's own (unrelated)
> reserved bits are left in place instead of the source file's.
>
> Add vfat_overwrite_lcase_reserved_bits() and call it once new_i_pos is
> established inside vfat_rename(), overwriting the surviving entry's
> reserved lcase bits with the original file's, so the metadata is
> carried over across the rename instead of being lost.
>
> Tested by booting the patched kernel under QEMU and replaying the bug
> report's own before/after FAT32 images: the renamed entry's lcase byte
> now matches the source's reserved bits instead of being zeroed.

A bit strange design. It is mixing different lifetime, one is for dirent
and one is for inode.

Well, so, windows normal FAT really reserve those bits too over rename?
And implementation side, vfat_overwrite_lcase_reserved_bits() doesn't
race with fat_write_inode()?

If it has to reserve the inode lifetime bits, I feel it should write via
fat_write_inode().

Thanks.

> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=215093
> Signed-off-by: Cihan Karadag <cihan.cihan@gmail.com>
> ---
>  fs/fat/namei_vfat.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index e909447873e36..d4677c1eaabc3 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -931,6 +931,38 @@ static void vfat_update_dir_metadata(struct inode *dir, struct timespec64 *ts)
>  		mark_inode_dirty(dir);
>  }
>  
> +/*
> + * Overwrite the reserved lcase bits (those not used by the
> + * kernel) of the entry at i_pos with those from the source
> + * lcase byte.
> + */
> +static int vfat_overwrite_lcase_reserved_bits(struct inode *dir, loff_t i_pos,
> +					      unsigned char src_lcase)
> +{
> +	/* lcase bits the kernel actually assigns meaning to */
> +	const unsigned char lcase_unreserved_bits = CASE_LOWER_BASE | CASE_LOWER_EXT;
> +	struct super_block *sb = dir->i_sb;
> +	struct buffer_head *bh;
> +	struct msdos_dir_entry *de;
> +	sector_t blocknr;
> +	int offset, err = 0;
> +
> +	fat_get_blknr_offset(MSDOS_SB(sb), i_pos, &blocknr, &offset);
> +	bh = sb_bread(sb, blocknr);
> +	if (!bh)
> +		return -EIO;
> +
> +	de = &((struct msdos_dir_entry *)bh->b_data)[offset];
> +	de->lcase = (de->lcase & lcase_unreserved_bits) |
> +		    (src_lcase & ~lcase_unreserved_bits);
> +
> +	mmb_mark_buffer_dirty(bh, &MSDOS_I(dir)->i_metadata_bhs);
> +	if (IS_DIRSYNC(dir))
> +		err = sync_dirty_buffer(bh);
> +	brelse(bh);
> +	return err;
> +}
> +
>  static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
>  		       struct inode *new_dir, struct dentry *new_dentry)
>  {
> @@ -974,6 +1006,17 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
>  			goto out;
>  		new_i_pos = sinfo.i_pos;
>  	}
> +
> +	/*
> +	 * The kernel only interprets some of the lcase bits; make
> +	 * sure the rest carry over from old_dentry's lcase instead
> +	 * of being silently clobbered.
> +	 */
> +	err = vfat_overwrite_lcase_reserved_bits(new_dir, new_i_pos,
> +						 old_sinfo.de->lcase);
> +	if (err)
> +		goto error_inode;
> +
>  	inode_inc_iversion(new_dir);
>  
>  	fat_detach(old_inode);

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