Re: [RFC PATCH 09/11] mm/hmm: Remove racy protection against double-unregistration

[Jason Gunthorpe <jgg@ziepe.ca>]

On Sat, Jun 08, 2019 at 01:08:37AM +0530, Souptick Joarder wrote:
> On Thu, May 23, 2019 at 9:05 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > From: Jason Gunthorpe <jgg@mellanox.com>
> >
> > No other register/unregister kernel API attempts to provide this kind of
> > protection as it is inherently racy, so just drop it.
> >
> > Callers should provide their own protection, it appears nouveau already
> > does, but just in case drop a debugging POISON.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> >  mm/hmm.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index 46872306f922bb..6c3b7398672c29 100644
> > +++ b/mm/hmm.c
> > @@ -286,18 +286,13 @@ EXPORT_SYMBOL(hmm_mirror_register);
> >   */
> >  void hmm_mirror_unregister(struct hmm_mirror *mirror)
> >  {
> > -       struct hmm *hmm = READ_ONCE(mirror->hmm);
> > -
> > -       if (hmm == NULL)
> > -               return;
> > +       struct hmm *hmm = mirror->hmm;
> 
> How about remove struct hmm *hmm and replace the code like below -
> 
> down_write(&mirror->hmm->mirrors_sem);
> list_del_init(&mirror->list);
> up_write(&mirror->hmm->mirrors_sem);
> hmm_put(hmm);
> memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
> 
> Similar to hmm_mirror_register().

I think we get there in patch 10, right?

When the series is all done the function looks like this:

void hmm_mirror_unregister(struct hmm_mirror *mirror)
{
        struct hmm *hmm = mirror->hmm;

        down_write(&hmm->mirrors_sem);
        list_del(&mirror->list);
        up_write(&hmm->mirrors_sem);
        hmm_put(hmm);
        memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
}

I think this mostly matches what you wrote above, or do you think we
should s/hmm/mirror->hmm/ anyhow? I think Ralph just added that :)

Regards,
Jason