linux-kernel - Re: [PATCH v2] bcache: Fix exception handling in mca

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Sun, 26 Mar 2023 00:07:14 +0800
From:   Coly Li <colyli@...e.de>
To:     Markus Elfring <Markus.Elfring@....de>
Cc:     kernel-janitors@...r.kernel.org, linux-bcache@...r.kernel.org,
        Kent Overstreet <kent.overstreet@...il.com>, cocci@...ia.fr,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] bcache: Fix exception handling in mca_alloc()



> 2023年3月25日 20:21，Markus Elfring <Markus.Elfring@....de> 写道：
> 
> Date: Sat, 25 Mar 2023 13:08:01 +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”.
> 
> 1. Thus use more appropriate labels instead.

It is not convinced to me that the new added labels are more appropriate. IMHO, the change just makes the code to be more complicated.


> 
> 2. Delete a repeated check (for the variable “b”)
>   which became unnecessary with this refactoring.
> 

To remove one line ‘if’ check, 13 lines are changed. IMHO this is not worthy. Yes the extra ‘if’ check can be avoided, but the code is more simple before adding labels unlock and cannibalize_mca.

The ‘if’ check is in error handling, which is not hot code path. Comparing to avoid an ‘if’ check, I prefer more for more simpler code. I am not supportive to this change.


Thanks.

Coly Li


> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: cafe563591446cf80bfbc2fe3bc72a2e36cf1060 ("bcache: A block layer cache")
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
> V2:
> Use another label.
> 
> drivers/md/bcache/btree.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 147c493a989a..c6a20595302f 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 cannibalize_mca;
> 
> 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,10 +955,9 @@ 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);
> +cannibalize_mca:
> b = mca_cannibalize(c, op, k);
> if (!IS_ERR(b))
> goto out;
> --
> 2.40.0
>