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

Date:   Fri, 25 Jan 2019 10:15:31 +0100
From:   Hans Holmberg <hans.ml.holmberg@...tronix.com>
To:     Javier González <javier@...igon.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 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.

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.

Thanks,
Hans

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