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@xxxxxxxxxxxxxxxxxxxx>
<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
