[PATCH] btrfs: fix compilation error on sparc/parisc

[PATCH] btrfs: fix compilation error on sparc/parisc

[Sweet Tea Dorminy]

Commit 1ec49744ba83 ("btrfs: turn on -Wmaybe-uninitialized") exposed
that on sparc and parisc, gcc is unaware that fscrypt_setup_filename()
only returns negative error values or 0. This ultimately results in a
maybe-uninitialized warning in btrfs_lookup_dentry().

Change to only return negative error values or 0 from
fscrypt_setup_filename() at the relevant callsite, and assert that no
positive error codes are returned (which would have wider implications
involving other users).

Reported-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/all/481b19b5-83a0-4793-b4fd-194ad7b978c3@roeck-us.net/
Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/btrfs/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 45102785c723..50178609f241 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5409,8 +5409,13 @@ static int btrfs_inode_by_name(struct btrfs_inode *dir, struct dentry *dentry,
 		return -ENOMEM;
 
 	ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 1, &fname);
-	if (ret)
+	if (ret < 0)
 		goto out;
+	/*
+	 * fscrypt_setup_filename() should never return a positive value, but
+	 * gcc on sparc/parisc thinks it can, so assert that doesn't happen.
+	 */
+	ASSERT(ret == 0);
 
 	/* This needs to handle no-key deletions later on */
 
-- 
2.39.2

Re: [PATCH] btrfs: fix compilation error on sparc/parisc

[Guenter Roeck]

On 3/8/23 07:58, Sweet Tea Dorminy wrote:
> Commit 1ec49744ba83 ("btrfs: turn on -Wmaybe-uninitialized") exposed
> that on sparc and parisc, gcc is unaware that fscrypt_setup_filename()
> only returns negative error values or 0. This ultimately results in a
> maybe-uninitialized warning in btrfs_lookup_dentry().
> 
> Change to only return negative error values or 0 from
> fscrypt_setup_filename() at the relevant callsite, and assert that no
> positive error codes are returned (which would have wider implications
> involving other users).
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/all/481b19b5-83a0-4793-b4fd-194ad7b978c3@roeck-us.net/
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>   fs/btrfs/inode.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 45102785c723..50178609f241 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5409,8 +5409,13 @@ static int btrfs_inode_by_name(struct btrfs_inode *dir, struct dentry *dentry,
>   		return -ENOMEM;
>   
>   	ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 1, &fname);
> -	if (ret)
> +	if (ret < 0)
>   		goto out;
> +	/*
> +	 * fscrypt_setup_filename() should never return a positive value, but
> +	 * gcc on sparc/parisc thinks it can, so assert that doesn't happen.
> +	 */

Here is some food for thought:

1) Make sure that "location" is never initialized in btrfs_inode_by_name(),
    and that the functions returns a value >= 0.

2) compile. gcc never reports an error or warning (tested with 10.3, 11.3,
    and 12.2).

On x86, gcc doesn't report a problem even if I replace btrfs_inode_by_name()
with a simple "return 0;".

I looks like there is something else going on which prevents gcc on
architectures other than parisc and sparc from reporting uninitialized
stack variables (or at least uninitialized structure elements),
and I am not entirely sure if this should be blamed on the architecture.

More specifically and importantly, it may not be a good idea to expect
gcc to report potentially uninitialized variables on architectures
other than parisc and sparc.

Guenter

> +	ASSERT(ret == 0);
>   
>   	/* This needs to handle no-key deletions later on */
>   

Re: [PATCH] btrfs: fix compilation error on sparc/parisc

[David Sterba]

On Wed, Mar 08, 2023 at 10:58:36AM -0500, Sweet Tea Dorminy wrote:
> Commit 1ec49744ba83 ("btrfs: turn on -Wmaybe-uninitialized") exposed
> that on sparc and parisc, gcc is unaware that fscrypt_setup_filename()
> only returns negative error values or 0. This ultimately results in a
> maybe-uninitialized warning in btrfs_lookup_dentry().
> 
> Change to only return negative error values or 0 from
> fscrypt_setup_filename() at the relevant callsite, and assert that no
> positive error codes are returned (which would have wider implications
> involving other users).
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/all/481b19b5-83a0-4793-b4fd-194ad7b978c3@roeck-us.net/
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Added to misc-next, thanks.