linux-kernel - Re: [PATCH] lightnvm: pblk: adjust the position of the lock

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 27 Nov 2018 12:57:54 +0000
From:   Javier Gonzalez <javier@...xlabs.com>
To:     Hua Su <suhua.tanke@...il.com>
CC:     Matias Bjørling <mb@...htnvm.io>,
        Jens Axboe <axboe@...nel.dk>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] lightnvm: pblk: adjust the position of the lock


> On 27 Nov 2018, at 02.53, Hua Su <suhua.tanke@...il.com> wrote:
> 
> Add lock protection for list operations.
> Signed-off-by: Hua Su <suhua.tanke@...il.com>
> ---
> drivers/lightnvm/pblk-core.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
> index 6944aac43b01..e490df217dac 100644
> --- a/drivers/lightnvm/pblk-core.c
> +++ b/drivers/lightnvm/pblk-core.c
> @@ -1286,24 +1286,27 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
> 	list_del(&line->list);
> 
> 	ret = pblk_line_prepare(pblk, line);
> -	if (ret) {
> -		list_add(&line->list, &l_mg->free_list);
> -		spin_unlock(&l_mg->free_lock);
> -		return ret;
> -	}
> -	spin_unlock(&l_mg->free_lock);
> +	if (ret)
> +		goto out;
> 
> 	ret = pblk_line_alloc_bitmaps(pblk, line);
> 	if (ret)
> -		return ret;
> +		goto out;
> 
> 	if (!pblk_line_init_bb(pblk, line, 0)) {
> 		list_add(&line->list, &l_mg->free_list);
> +		spin_unlock(&l_mg->free_lock);
> 		return -EINTR;
> 	}
> +	spin_unlock(&l_mg->free_lock);
> 
> 	pblk_rl_free_lines_dec(&pblk->rl, line, true);
> 	return 0;
> +
> +out:
> +	list_add(&line->list, &l_mg->free_list);
> +	spin_unlock(&l_mg->free_lock);
> +	return ret;
> }
> 
> void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
> --
> 2.19.1

This path is only touched by the recovery path, which is single
threaded, so there is no race condition as is. Also, if recovery fails,
pblk will not create the instance at all. This said, it would be
good to protect the list_add on the pblk_line_init_bb() error path in
case this code is used for some other purpose in the future.

However, the implementation you propose has some issues due to the
extended critical zone, which covers pblk_line_alloc_bitmaps(). Here,
allocations are intentionally non-atomic (i.e., GFP_KERNEL).

You can instead do the following:

diff --git i/drivers/lightnvm/pblk-core.c w/drivers/lightnvm/pblk-core.c
index 615817bf97e3..02c4c1708c45 100644
--- i/drivers/lightnvm/pblk-core.c
+++ w/drivers/lightnvm/pblk-core.c
@@ -1301,15 +1301,22 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)

        ret = pblk_line_alloc_bitmaps(pblk, line);
        if (ret)
-               return ret;
+               goto fail;

        if (!pblk_line_init_bb(pblk, line, 0)) {
-               list_add(&line->list, &l_mg->free_list);
-               return -EINTR;
+               ret = -EINTR;
+               goto fail;
        }

        pblk_rl_free_lines_dec(&pblk->rl, line, true);
        return 0;
+
+fail:
+       spin_lock(&l_mg->free_lock);
+       list_add(&line->list, &l_mg->free_list);
+       spin_unlock(&l_mg->free_lock);
+
+       return ret;
 }

 void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)

Javier



Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)