Re: [PATCH v2] x86/mm: Add mem access rights to NPT

[Tamas K Lengyel <tamas@tklengyel.com>]

On Tue, Jun 19, 2018 at 2:14 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
> On Lu, 2018-06-18 at 09:39 -0600, Tamas K Lengyel wrote:
> > On Mon, Jun 18, 2018 at 9:19 AM Alexandru Isaila
> > <aisaila@bitdefender.com> wrote:
> > >
> > >
> > > +static p2m_access_t p2m_get_access(struct p2m_domain *p2m,
> > > unsigned long gfn)
> > > +{
> > > +    void *ptr;
> > > +
> > > +    ptr = radix_tree_lookup(&p2m->mem_access_settings, gfn);
> > > +    if ( !ptr )
> > > +        return p2m_access_rwx;
> > > +    else
> > > +        return radix_tree_ptr_to_int(ptr);
> > > +}
> > > +
> > > +static void p2m_set_access(struct p2m_domain *p2m, unsigned long
> > > gfn,
> > > +                                      p2m_access_t a)
> > > +{
> > > +    int rc;
> > > +
> > Shouldn't there be some locking around the radix tree operations
> > here?
> > If not, why not?
> The lock is in the p2m_set_mem_access() so that one entry is set at a
> time. The radix tree operations are similar to the ones on ARM but if
> we missed something I will appreciate the help in correcting the issue.

There are calls to this function that are not from p2m_set_mem_access
though, so I just wanted to make sure that each of those call-sites
already has a lock in place. Perhaps a comment to this function should
make that clear that a caller has to perform its own locking, the
function as-is doesn't do that. Or perhaps an ASSERT would be even
better if possible?

Thanks,
Tamas

> >
> > >
> > > +    if ( p2m_access_rwx == a )
> > > +        radix_tree_delete(&p2m->mem_access_settings, gfn);
> > > +
> > > +    rc = radix_tree_insert(&p2m->mem_access_settings, gfn,
> > > +                           radix_tree_int_to_ptr(a));
> > > +    if ( rc == -EEXIST )
> > > +        /* If a setting already exists, change it to the new one
> > > */
> > > +        radix_tree_replace_slot(
> > > +            radix_tree_lookup_slot(
> > > +                &p2m->mem_access_settings, gfn),
> > > +            radix_tree_int_to_ptr(a));
> > > +}
> > > +
>
> ________________________
> This email was scanned by Bitdefender

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel