[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] ARM fixes for my improved privcmd patch.



On 19/12/12 15:45, Ian Campbell wrote:
On Wed, 2012-12-19 at 15:08 +0000, Mats Petersson wrote:
On 19/12/12 12:22, Ian Campbell wrote:
On Wed, 2012-12-19 at 12:10 +0000, Mats Petersson wrote:

+                  only likely to return EFAULT or some other "things are very
+                  bad" error code, which the rest of the calling code won't
+                  be able to fix up. So we just exit with the error we got.
It expect it is more important to accumulate the individual errors from
remap_pte_fn into err_ptr.
Yes, but since that exits on error with EFAULT, the calling code won't
"accept" the errors, and thus the whole house of cards fall apart at
this point.

There should probably be a task to fix this up properly, hence the
comment. But right now, any error besides ENOENT is "unacceptable" by
the callers of this code. If you want me to add this to the comment, I'm
happy to. But as long as remap_pte_fn returns EFAULT on error, nothing
will work after an error.
Are you sure? privcmd.c has some special casing for ENOENT but it looks
like it should just pass through other errors back to userspace.

In any case surely this needs fixing?

On the X86 side err_ptr is the result of the mmupdate hypercall which
can already be other than ENOENT, including EFAULT, ESRCH etc.
Yes, but the ONLY error that is "acceptable" (as in, doesn't lead to the
calling code revoking the mapping and returning an error) is ENOENT.
Hr, Probably the right thing is for map_foreign_page to propagate the
result of XENMEM_add_to_physmap_range and for remap_pte_fn to store it
in err_ptr. That -EFAULT thing just looks wrong to me.
Ok, so you want me to fix that up, I suppose? I mean, I just copied the behaviour that was already there - just massaged the code around a bit...

--
Mats

Or at least, that's how I believe it should SHOULD be - since only
ENOENT is a "success" error code, anything else pretty much means that
the operation requested didn't work properly. If you are aware of any
use-case where EFAULT, ESRCH or other error codes would still result in
a valid, usable memory mapping - I have a fair understanding of the xc_*
code, and although I may not know every piece of that code, I'm fairly
certainly that is the expected behaviour.

--
Mats
Ian.




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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.