Re: [PATCH 10/11] fs/ntfs3: Remove cached label from sbi

[Nathan Chancellor <nathan@kernel.org>]

Hi Konstantin,

On Wed, Apr 17, 2024 at 04:09:00PM +0300, Konstantin Komarov wrote:
> Add more checks using $AttrDef.
> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>

<snip>

> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 8beefbca5769..dae961d2d6f8 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -481,11 +481,39 @@ static int ntfs3_volinfo_open(struct inode *inode,
> struct file *file)
>  /* read /proc/fs/ntfs3/<dev>/label */
>  static int ntfs3_label_show(struct seq_file *m, void *o)
>  {
> +    int len;
>      struct super_block *sb = m->private;
>      struct ntfs_sb_info *sbi = sb->s_fs_info;
> +    struct ATTRIB *attr;
> +    u8 *label = kmalloc(PAGE_SIZE, GFP_NOFS);
> +
> +    if (!label)
> +        return -ENOMEM;
> +
> +    attr = ni_find_attr(sbi->volume.ni, NULL, NULL, ATTR_LABEL, NULL, 0,
> +                NULL, NULL);
> +
> +    if (!attr) {
> +        /* It is ok if no ATTR_LABEL */
> +        label[0] = 0;
> +        len = 0;
> +    } else if (!attr_check(attr, sbi, sbi->volume.ni)) {
> +        len = sprintf(label, "%pg: failed to get label", sb->s_bdev);
> +    } else {
> +        len = ntfs_utf16_to_nls(sbi, resident_data(attr),
> +                    le32_to_cpu(attr->res.data_size) >> 1,
> +                    label, PAGE_SIZE);
> +        if (len < 0) {
> +            label[0] = 0;
> +            len = 0;
> +        } else if (len >= PAGE_SIZE) {
> +            len = PAGE_SIZE - 1;
> +        }
> +    }
> 
> -    seq_printf(m, "%s\n", sbi->volume.label);
> +    seq_printf(m, "%.*s\n", len, label);
> 
> +    kfree(label);
>      return 0;
>  }
> 
> @@ -1210,25 +1238,6 @@ static int ntfs_fill_super(struct super_block *sb,
> struct fs_context *fc)
> 
>      ni = ntfs_i(inode);
> 
> -    /* Load and save label (not necessary). */
> -    attr = ni_find_attr(ni, NULL, NULL, ATTR_LABEL, NULL, 0, NULL, NULL);
> -
> -    if (!attr) {
> -        /* It is ok if no ATTR_LABEL */
> -    } else if (!attr->non_res && !is_attr_ext(attr)) {
> -        /* $AttrDef allows labels to be up to 128 symbols. */
> -        err = utf16s_to_utf8s(resident_data(attr),
> -                      le32_to_cpu(attr->res.data_size) >> 1,
> -                      UTF16_LITTLE_ENDIAN, sbi->volume.label,
> -                      sizeof(sbi->volume.label));
> -        if (err < 0)
> -            sbi->volume.label[0] = 0;
> -    } else {
> -        /* Should we break mounting here? */
> -        //err = -EINVAL;
> -        //goto put_inode_out;
> -    }
> -

The attr initialization above causes the use in the ni_find_attr() to be
uninitialized, which results in the following clang warning (or error
when CONFIG_WERROR is enabled):

  fs/ntfs3/super.c:1247:26: error: variable 'attr' is uninitialized when used here [-Werror,-Wuninitialized]
   1247 |         attr = ni_find_attr(ni, attr, NULL, ATTR_VOL_INFO, NULL, 0, NULL, NULL);
        |                                 ^~~~
  fs/ntfs3/super.c:1192:21: note: initialize the variable 'attr' to silence this warning
   1192 |         struct ATTRIB *attr;
        |                            ^
        |                             = NULL
  1 error generated.

Please either fix this quickly (as this isn't the first time an ntfs3
change has broken us for some time in -next for a similar reason [1]) or
remove this series from -next. Given the issues that Johan has brought
up in other comments in this thread, I feel like the latter may be
better, as this series is clearly not ready for Linus (which is one of
-next's general requirements AFAIUI).

>      attr = ni_find_attr(ni, attr, NULL, ATTR_VOL_INFO, NULL, 0, NULL,
> NULL);
>      if (!attr || is_attr_ext(attr) ||
>          !(info = resident_data_ex(attr, SIZEOF_ATTRIBUTE_VOLUME_INFO))) {

[1]: https://github.com/ClangBuiltLinux/linux/issues/1729

Cheers,
Nathan