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

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> 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.

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.)

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.

 -George

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