linux-kernel - Re: [PATCH linux-next] fs: btrfs: fix returnvar.cocci warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <900f28f9-6efe-48a7-246f-797a9aa48c07@gmx.com>
Date:   Fri, 20 Aug 2021 11:16:24 +0800
From:   Qu Wenruo <quwenruo.btrfs@....com>
To:     jing yangyang <cgel.zte@...il.com>, Chris Mason <clm@...com>
Cc:     Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        jing yangyang <jing.yangyang@....com.cn>,
        Zeal Robot <zealci@....com.cn>
Subject: Re: [PATCH linux-next] fs: btrfs: fix returnvar.cocci warnings



On 2021/8/20 上午10:32, jing yangyang wrote:
> Remove unneeded variables when "0" can be returned.
>
> Generated by: scripts/coccinelle/misc/returnvar.cocci
>
> Reported-by: Zeal Robot <zealci@....com.cn>
> Signed-off-by: jing yangyang <jing.yangyang@....com.cn>
> ---
>   fs/btrfs/extent_map.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
> index 4a8e02f..58860d7 100644
> --- a/fs/btrfs/extent_map.c
> +++ b/fs/btrfs/extent_map.c
> @@ -296,7 +296,6 @@ static void try_merge_map(struct extent_map_tree *tree, struct extent_map *em)
>   int unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len,
>   		       u64 gen)
>   {
> -	int ret = 0;
>   	struct extent_map *em;
>   	bool prealloc = false;
>

Please just check the lines below:

	em = lookup_extent_mapping(tree, start, len);

	WARN_ON(!em || em->start != start);

	if (!em)
		goto out;

This looks more like a missing error handling.

Thus the proper way to fix it is not just simply remove the "int ret =
0;" line (which compiler is more than able to optimize it out), but
properly add the error handling, and modify the only caller to catch
such error properly.

Some diff like the below would be more meaningful:

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 4a8e02f7b6c7..9182d747a50e 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -303,10 +303,11 @@ int unpin_extent_cache(struct extent_map_tree
*tree, u64 start, u64 len,
  	write_lock(&tree->lock);
  	em = lookup_extent_mapping(tree, start, len);

-	WARN_ON(!em || em->start != start);
-
-	if (!em)
+	if (!em || em->start != start) {
+		WARN(1, KERN_WARNING "unexpected extent mapping\n");
+		ret = -EUCLEAN;
  		goto out;
+	}

  	em->generation = gen;
  	clear_bit(EXTENT_FLAG_PINNED, &em->flags);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2aa9646bce56..313b0a314c0b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2989,6 +2989,7 @@ static int btrfs_finish_ordered_io(struct
btrfs_ordered_extent *ordered_extent)
  	u64 start, end;
  	int compress_type = 0;
  	int ret = 0;
+	int ret2;
  	u64 logical_len = ordered_extent->num_bytes;
  	bool freespace_inode;
  	bool truncated = false;
@@ -3076,8 +3077,11 @@ static int btrfs_finish_ordered_io(struct
btrfs_ordered_extent *ordered_extent)
  						ordered_extent->disk_num_bytes);
  		}
  	}
-	unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset,
+	ret2 = unpin_extent_cache(&inode->extent_tree,
ordered_extent->file_offset,
  			   ordered_extent->num_bytes, trans->transid);
+	if (ret2 < 0 && !ret)
+		ret = ret2;
+
  	if (ret < 0) {
  		btrfs_abort_transaction(trans, ret);
  		goto out;


Thanks,
Qu
> @@ -328,7 +327,7 @@ int unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len,
>   	free_extent_map(em);
>   out:
>   	write_unlock(&tree->lock);
> -	return ret;
> +	return 0;
>
>   }
>
>