Re: Re: [PATCH 05/15] HMM: introduce heterogeneous memory management v4.

[GIRISH K S <ks.giri@samsung.com>]

------- Original Message -------
Sender : Jerome Glisse<j.glisse@gmail.com>
Date : Aug 03, 2015 17:26 (GMT+05:30)
Title : Re: [PATCH 05/15] HMM: introduce heterogeneous memory management v4.

On Mon, Aug 03, 2015 at 01:20:13PM +0530, Girish KS wrote:
> On 18-Jul-2015 12:47 am, "J��e Glisse" wrote:
> >

[...]

> > +int hmm_mirror_register(struct hmm_mirror *mirror)
> > +{
> > +       struct mm_struct *mm = current->mm;
> > +       struct hmm *hmm = NULL;
> > +       int ret = 0;
> > +
> > +       /* Sanity checks. */
> > +       BUG_ON(!mirror);
> > +       BUG_ON(!mirror->device);
> > +       BUG_ON(!mm);
> > +
> > +       /*
> > +        * Initialize the mirror struct fields, the mlist init and del
> dance is
> > +        * necessary to make the error path easier for driver and for hmm.
> > +        */
> > +       kref_init(&mirror->kref);
> > +       INIT_HLIST_NODE(&mirror->mlist);
> > +       INIT_LIST_HEAD(&mirror->dlist);
> > +       spin_lock(&mirror->device->lock);
> > +       list_add(&mirror->dlist, &mirror->device->mirrors);
> > +       spin_unlock(&mirror->device->lock);
> > +
> > +       down_write(&mm->mmap_sem);
> > +
> > +       hmm = mm->hmm ? hmm_ref(hmm) : NULL;
> 
> Instead of hmm mm->hmm would be the right param to be passed.  Here even
> though mm->hmm is true hmm_ref returns NULL. Because hmm is not updated
> after initialization in the beginning.

ENOPARSE ? While this can be simplified to hmm = hmm_ref(mm->hmm); I do not
see what you mean. The mm struct might already have a valid hmm field set,
and that valid hmm struct might also already be in the process of being
destroy. So hmm_ref() might either return the same hmm pointer if the hmm
object is not about to be release or NULL. But at this point there is no
certainty on the return value of hmm_ref().

I didn't mean hmm = hmm_ref(mm->hmm);. I ll try to put it in a better way. The hmm local variable is initialized to NULL in the start of the function (struct hmm *hmm = NULL;), and this is not modified till it is passed to  hmm_ref.  So hmm_ref would always return a NULL irrespective of mm->hmm is NULL or valid address.  
So  the statement hmm = mm->hmm ? hmm_ref(hmm) : NULL; should be replaced as hmm = mm->hmm ? hmm_ref(mm->hmm) : NULL;. 
Also Assume mm->hmm had a hmm object assigned to it before entering this function. Since the hmm_ref(hmm) returns NULL always, previously assigned mm->hmm address would be overwritten by the  allocation and assignment that happen below in this function.

Note that because we have the mmap sem in write mode we know it is safe
to dereference mm->hmm and even to overwrite that field it if it is being
destroy concurently.

Cheers,
J��e