Re: [PATCH resent] bcache: Fix exception handling in mca_alloc()

[Coly Li]

On 3/25/23 5:31 PM, Markus Elfring wrote:
> Date: Mon, 20 Mar 2023 13:13:37 +0100
>
> The label “err” was used to jump to another pointer check despite of
> the detail in the implementation of the function “mca_alloc”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed function call “mca_bucket_alloc”.


Hmm, I don't get the exact point from the above long sentence. Could you 
please describe a bit more specific where the problem is at exact line 
number of the code?

> * Thus use a more appropriate label instead.

So far I am not convinced the modified label is more appropriate.

> * Delete a redundant check.

Where is the location of the redundant check?


> This issue was detected by using the Coccinelle software.

Just curious, what is the warning reported by the mentioned software ?


> Fixes: cafe563591446cf80bfbc2fe3bc72a2e36cf1060 ("bcache: A block layer cache")
> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> ---
>   drivers/md/bcache/btree.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 147c493a989a..166afd7ec499 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -921,18 +921,18 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
>   		if (!mca_reap(b, 0, false)) {
>   			mca_data_alloc(b, k, __GFP_NOWARN|GFP_NOIO);
>   			if (!b->keys.set[0].data)
> -				goto err;
> +				goto unlock;
>   			else
>   				goto out;
>   		}
>
>   	b = mca_bucket_alloc(c, k, __GFP_NOWARN|GFP_NOIO);
>   	if (!b)
> -		goto err;
> +		goto unlock;
>
>   	BUG_ON(!down_write_trylock(&b->lock));
>   	if (!b->keys.set->data)
> -		goto err;
> +		goto unlock;
>   out:
>   	BUG_ON(b->io_mutex.count != 1);
>
> @@ -955,9 +955,8 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
>   				    &b->c->expensive_debug_checks);
>
>   	return b;
> -err:
> -	if (b)
> -		rw_unlock(true, b);
> +unlock:
> +	rw_unlock(true, b);

If b is NULL, is it a bit fishing to send the NULL pointer into 
rw_unlock() ?


Thanks in advance for more information.


Coly Li