linux-kernel - Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines

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

Message-Id: <7BAC034D-8992-41E4-87A9-378813C821F7@javigon.com>
Date:   Fri, 25 Jan 2019 10:37:14 +0100
From:   Javier González <javier@...igon.com>
To:     Hans Holmberg <hans.ml.holmberg@...tronix.com>
Cc:     Matias Bjørling <mb@...htnvm.io>,
        linux-block@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hans Holmberg <hans.holmberg@...xlabs.com>
Subject: Re: [PATCH] lightnvm: pblk: stop taking the free lock in in
 pblk_lines_free


> On 25 Jan 2019, at 10.15, Hans Holmberg <hans.ml.holmberg@...tronix.com> wrote:
> 
> On Thu, Jan 24, 2019 at 2:19 PM Javier González <javier@...igon.com> wrote:
>>> On 22 Jan 2019, at 11.15, hans@...tronix.com wrote:
>>> 
>>> From: Hans Holmberg <hans.holmberg@...xlabs.com>
>>> 
>>> pblk_line_meta_free might sleep (it can end up calling vfree, depending
>>> on how we allocate lba lists), and this can lead to a BUG()
>>> if we wake up on a different cpu and release the lock.
>>> 
>>> As there is no point of grabbing the free lock when pblk has shut down,
>>> remove the lock.
>>> 
>>> Signed-off-by: Hans Holmberg <hans.holmberg@...xlabs.com>
>>> ---
>>> drivers/lightnvm/pblk-init.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>> 
>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>> index f9a3e47b6a93..eb0135c77805 100644
>>> --- a/drivers/lightnvm/pblk-init.c
>>> +++ b/drivers/lightnvm/pblk-init.c
>>> @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk)
>>>      struct pblk_line *line;
>>>      int i;
>>> 
>>> -     spin_lock(&l_mg->free_lock);
>>>      for (i = 0; i < l_mg->nr_lines; i++) {
>>>              line = &pblk->lines[i];
>>> 
>>>              pblk_line_free(line);
>>>              pblk_line_meta_free(l_mg, line);
>>>      }
>>> -     spin_unlock(&l_mg->free_lock);
>>> 
>>>      pblk_line_mg_free(pblk);
>>> 
>>> --
>>> 2.17.1
>> 
>> Can you add a comment too indicating that this is only safe on a single
>> threaded shutdown?
> 
> To be able to free the lines, we need have stopped anything accessing
> the lines first. That seems obvious to me.
> 

The reason I mention this is that there is assumptions on the shutdown
logic that the line freeup will be single threaded - you can see that we
do not lock pblk_line_mg_free() either, for the same reason as you are
removing this lock. We can do this is only because we have a single open
line at the time (which in close down we fill up in parallel to speed up
the process BTW). If we have more open lines, it would be desirable to
close them in parallel. At this point we either have a sync point when
they are all closed and then free, or we allow them to free themselves.
In the second case, locking will be necessary.

IMHO, a comment does not hurt.

> It would be nice to make a pass over the code and document pblk's
> locking(and other concurrency handling, like the line krefs) though.
> 

True that - krefs specially would deserve some documentation. Let me see
if I can allocate some time the following weeks to write up some
documentation.

> Thanks,
> Hans
> 
>> Otherwise the patch looks good.
>> 
>> Reviewed-by: Javier González <javier@...igon.com>

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