Re: [Xen-devel] [PATCH v1 7/8]: PVH privcmd changes

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-10-03 at 23:31 +0100, Mukesh Rathor wrote:
> On Wed, 3 Oct 2012 14:21:35 +0100
> Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 
> > On Fri, 2012-09-21 at 20:21 +0100, Mukesh Rathor wrote:
> > > +static int pvh_privcmd_resv_pfns(struct vm_area_struct *vma, int
> > > numpgs)
> > ...
> > > +       pvhp->pi_num_pgs = numpgs;
> > > +       BUG_ON(vma->vm_private_data != (void *)1);
> > > +       vma->vm_private_data = pvhp; 
> > 
> > How does this interact with:
> > 
> > static int privcmd_enforce_singleshot_mapping(struct vm_area_struct
> > *vma) {
> >     return (xchg(&vma->vm_private_data, (void *)1) == NULL);
> > }
> > 
> > If someone tries to map a second time then won't this correct the pvhp
> > in vm_private_data by resetting it to 1? Then when the original
> > mapping is torn down things all fall apart?
> > 
> > Perhaps we need a cmpxchg here? Or to rework the callers a little bit
> > perhaps.
> 
> Right, that's why I had it originally checking for auto xlated and
> doing something different. I think that is better than to change this
> and change again. I'll change it back to just putting the ptr here.

Won't that break because on the second call you will pass in the freshly
allocated pointer and overwrite the exiting (useful) one with it?

I think at a minimum you need a cmpxchg or some higher level locking
strategy.

None of the existing VM_* flags look suitable for this interlock, but I
wonder if the idea of forcing a singleton mapping is generic enough to
be worth such a flag?

There's no lock in the struct vm_area_struct, unless one is available
via vm_file or something? I've no idea what the locking hierarchy looks
like around this stuff sadly.

Perhaps the right answer is to allocate the struct in privcmd_mmap for
all modes and include the mapped flag in that?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel