Received-SPF: pass (google.com: domain of linux-kernel+bounces-154003-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 22 Apr 2024 13:42:24 -0700
From: Nathan Chancellor <nathan@kernel.org>
To: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc: ntfs3@lists.linux.dev, LKML <linux-kernel@vger.kernel.org>,
	Linux-fsdevel <linux-fsdevel@vger.kernel.org>, llvm@lists.linux.dev
Subject: Re: [PATCH 10/11] fs/ntfs3: Remove cached label from sbi
Message-ID: <20240422204224.GA770800@dev-arch.thelio-3990X>
References: <6c99c1bd-448d-4301-8404-50df34e8df8e@paragon-software.com>
 <890cc224-fdb8-4c5e-a22e-b96dc86e6908@paragon-software.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <890cc224-fdb8-4c5e-a22e-b96dc86e6908@paragon-software.com>

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