diff mbox series

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

Message ID 20190523153436.19102-10-jgg@ziepe.ca (mailing list archive)
State Superseded
Headers show
Series mm/hmm: Various revisions from a locking/code review | expand

Commit Message

Jason Gunthorpe May 23, 2019, 3:34 p.m. UTC
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(-)

Comments

Jason Gunthorpe June 7, 2019, 7:37 p.m. UTC | #1
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
Souptick Joarder June 7, 2019, 7:38 p.m. UTC | #2
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
> --- a/mm/hmm.c
> +++ 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().


>         down_write(&hmm->mirrors_sem);
>         list_del_init(&mirror->list);
> -       /* To protect us against double unregister ... */
> -       mirror->hmm = NULL;
>         up_write(&hmm->mirrors_sem);
> -
>         hmm_put(hmm);
> +       memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
>  }
>  EXPORT_SYMBOL(hmm_mirror_unregister);
>
> --
> 2.21.0
>
Souptick Joarder June 7, 2019, 7:55 p.m. UTC | #3
On Sat, Jun 8, 2019 at 1:07 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> 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?

No, Patch 10 of this series has modified hmm_range_unregister().
>
> 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 :)

I prefer, s/hmm/mirror->hmm and remove struct hmm *hmm :)
diff mbox series

Patch

diff --git a/mm/hmm.c b/mm/hmm.c
index 46872306f922bb..6c3b7398672c29 100644
--- a/mm/hmm.c
+++ 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;
 
 	down_write(&hmm->mirrors_sem);
 	list_del_init(&mirror->list);
-	/* To protect us against double unregister ... */
-	mirror->hmm = NULL;
 	up_write(&hmm->mirrors_sem);
-
 	hmm_put(hmm);
+	memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
 }
 EXPORT_SYMBOL(hmm_mirror_unregister);