Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <157b8db9-82f7-85e7-3bbd-7ef3a1797892@suse.de>
Date:   Sat, 25 Mar 2023 18:16:04 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH resent] bcache: Fix exception handling in mca_alloc()
To:     Markus Elfring <Markus.Elfring@web.de>
Cc:     cocci@inria.fr, kernel-janitors@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Kent Overstreet <kent.overstreet@gmail.com>,
        linux-bcache@vger.kernel.org
References: <f9303bdc-b1a7-be5e-56c6-dfa8232b8b55@web.de>
 <e33f264a-7ee9-4ebc-d58e-bbb7fd567198@web.de>
 <d0381c8e-7302-b0ed-cf69-cbc8c3618106@web.de>
Content-Language: en-US
From:   Coly Li <colyli@suse.de>
In-Reply-To: <d0381c8e-7302-b0ed-cf69-cbc8c3618106@web.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@users.sourceforge.net>
> ---
>   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