Re: [Xen-devel] [PATCH v3 6/8] p2m: change write_p2m_entry to return an error code

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 15, 2019 at 06:48:25PM +0100, George Dunlap wrote:
> 
> > On Feb 15, 2019, at 2:18 PM, Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
> > 
> > This is in preparation for also changing p2m_entry_modify to return an
> > error code.
> > 
> > No functional change intended.
> 
> I think you need to explain wheny/why you’re using BUG_ON() rather than 
> ASSERT() or passing the caller up the stack.
> 
> 
> Just in general:
> 
> * Passing things up the stack should be used when the caller is already 
> expecting to handle errors, and the state when the error was discovered isn’t 
> broken, or too hard to fix.
> 
> * BUG_ON() should be used when you can’t pass things up the stack, and 
> continuing would certainly cause a vulnerability.
> 
> * ASSERT() should be used when continuing might work, or might have an effect 
> later whose badness is equal or less than that of a host crash; OR whose 
> truth can be clearly observed from the code directly surrounding it.
> 
> For instance...
> 
> > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> > index 04e9d81cf6..44abd65999 100644
> > --- a/xen/arch/x86/mm/p2m-pt.c
> > +++ b/xen/arch/x86/mm/p2m-pt.c
> > @@ -202,7 +202,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
> >         new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
> > 
> >         p2m_add_iommu_flags(&new_entry, level, 
> > IOMMUF_readable|IOMMUF_writable);
> > -        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> > +        BUG_ON(p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level 
> > + 1));
> 
> In this case, a few lines above we have `return -ENOMEM`; so:
> 1. The caller is expecting to handle error values, and
> 2. There’s no unusual state to try to clean up.

I'm not that familiar with the p2m code, but there's a call to
p2m_alloc_ptp just further up, and while I think failing here would
only imply that a page has been added to p2m->pages but has not
actually been used in the p2m page tables because the addition of the
entry failed.

I think adding an entry that expands the page table structure should
never fail, and hence I think the following:

rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
if ( rc )
{
    ASSERT_UNREACHABLE();
    return rc;
}

Would be the best way to handle the return code from write_p2m_entry
in p2m_next_level.

> It seems like the `rc = … / if ( rc ) return rc` pattern would be better here.
> 
> >     }
> >     else if ( flags & _PAGE_PSE )
> >     {
> > @@ -250,14 +250,14 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
> >         {
> >             new_entry = l1e_from_pfn(pfn | (i << ((level - 1) * 
> > PAGETABLE_ORDER)),
> >                                      flags);
> > -            p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
> > +            BUG_ON(p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 
> > level));
> >         }
> 
> Here, we can’t return an error value, because we’re halfway through an 
> operation and don’t want to have to bother to clean up.  Ignoring the error 
> and leaving it half-done would certainly be worse than a host crash.
> 
> On the other hand… we know that the previous write_entry succeeded; is it 
> really possible for this one to fail?
> 
> p2m_entry_modify() doesn’t do anything for entries > 4k, and (by the end of 
> this series) will BUG_ON() if its "
> 
> I’m tempted to say that what we should do is use ASSERT() here (and many 
> other places) put a comment in p2m_entry_modify() to say that when changing 
> it, we need to revisit all the direct callers to make sure that ASSERT() is 
> still suitable.
> 
> > 
> >         unmap_domain_page(l1_entry);
> > 
> >         new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
> >         p2m_add_iommu_flags(&new_entry, level, 
> > IOMMUF_readable|IOMMUF_writable);
> > -        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> > +        BUG_ON(p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level 
> > + 1));
> 
> In this case, we can see from the context that what’s been written is a 
> mid-level entry that has just been allocated; there’s no code change that 
> should be able to cause this to fail.  I’m inclined to say this should only 
> be an ASSERT().
> 
> >     }
> >     else
> >         ASSERT(flags & _PAGE_PRESENT);
> > @@ -321,7 +321,7 @@ static int p2m_pt_set_recalc_range(struct p2m_domain 
> > *p2m,
> >             if ( (l1e_get_flags(e) & _PAGE_PRESENT) && !needs_recalc(l1, e) 
> > )
> >             {
> >                 set_recalc(l1, e);
> > -                p2m->write_p2m_entry(p2m, first_gfn, pent, e, level);
> > +                BUG_ON(p2m->write_p2m_entry(p2m, first_gfn, pent, e, 
> > level));
> >             }
> 
> Again here; theoretically, the only change has been that RECALC_FLAGS have 
> been added.
> 
> And so on.
> 
> Thoughts?  (Looking for input from Jan here as well.)

As you say here and above, the only caller that's expected to fail is
p2m_pt_set_entry, the rest just do recalc or add intermediate entries
to the p2m, which must always succeed.

> If not, it seems like we should also be modifying the places in p2m-ept.c to 
> do BUG_ON() rather than ASSERT()’ing that the page write succeeded.

I can apply the same treatment that's done in p2m-ept.c, so that we
have consistency between both implementations.

Thanks, Roger.

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