Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[Roger Pau Monne]

On Fri, Apr 29, 2016 at 04:30:16PM +0100, Wei Liu wrote:
> On Fri, Apr 29, 2016 at 05:12:51PM +0200, Roger Pau Monne wrote:
> > On Fri, Apr 29, 2016 at 04:02:33PM +0100, Wei Liu wrote:
> > > I have a gut feeling that returning XEN_ errno to userspace program is
> > > layering violation. They should always be translated to OS level errno
> > > by privcmd driver.
> > 
> > Yes, the error value returned from the hypercall executed is indeed 
> > translated into the native OS error space. The problem here is that those 
> > error codes are returned _inside_ of the specific hypercall struct, which 
> > sadly privcmd doesn't know anything about.
> > 
> > And of course teaching privcmd about every possible hypercall struct is 
> > simply impossible, since some of them are not stable (eg: domctls)
> >  
> > > Aren't FreeBSD and NetBSD already doing that?
> > 
> > As said above, this is only done for direct return codes, everything inside 
> > of the struct passed to the hypercall is returned as-is.
> > 
> > This is a complete mess, and TBH, I don't have a clever idea about how to 
> > solve it.
> > 
> 
> Me neither. Maybe a new thread should be started to discuss this.

So here we are.

In order to put everyone into context: the issue here is that some 
hypercalls (those that batch several operations) return an array of error 
codes inside of the hypercall structure. This array of error codes is not 
standardized, so the privcmd driver doesn't know anything about it, and thus 
cannot translate it into the native OS error space.

It has also been suggested that the privcmd driver simply doesn't translate 
error codes at all, and then let the applications figure out if the error 
code comes from Xen or from the OS. IMHO, this is impossible to achieve, 
because the ioctl syscall can return an error code that's been forwarded 
by Xen or a native one, and the application has no way of knowing where is 
it coming from.

I've identified at least the following hypercall structs that store XEN_* 
error codes inside:

 - xen_add_to_physmap_batch
 - xen_xsplice_status

TBH, it's quite hard to spot them, so I might have missed some. 

xen_add_to_physmap_batch is part of the public ABI, and cannot be changed. 
On the bright side, xen_add_to_physmap_batch is implemented as a different 
ioctl in privcmd usually (in order to map memory from other domains), so the 
error translation should be handled correctly.

Then the xsplice struct that uses XEN_* values is:

struct xen_xsplice_status {
#define XSPLICE_STATE_CHECKED      1
#define XSPLICE_STATE_APPLIED      2
    uint32_t state;                /* OUT: XSPLICE_STATE_*. */
    int32_t rc;                    /* OUT: 0 if no error, otherwise -XEN_EXX. */
};

Which is in turn used by:

struct xen_sysctl_xsplice_list {
    uint32_t version;                       /* OUT: Hypervisor stamps value.
                                               If varies between calls, we are
                                             * getting stale data. */
    uint32_t idx;                           /* IN: Index into hypervisor list. */
    uint32_t nr;                            /* IN: How many status, name, and len
                                               should fill out. Can be zero to get
                                               amount of payloads and version.
                                               OUT: How many payloads left. */
    uint32_t pad;                           /* IN: Must be zero. */
    XEN_GUEST_HANDLE_64(xen_xsplice_status_t) status;  /* OUT. Must have enough
                                               space allocate for nr of them. */
    XEN_GUEST_HANDLE_64(char) name;         /* OUT: Array of names. Each member
                                               MUST XEN_XSPLICE_NAME_SIZE in size.
                                               Must have nr of them. */
    XEN_GUEST_HANDLE_64(uint32) len;        /* OUT: Array of lengths of name's.
                                               Must have nr of them. */
};

IMHO, the best way to solve this is to define a set of XSPLICE_ERROR_* that 
covers the error codes returned by xsplice, and use that instead of XEN_* 
errno values. This would make it much more easier to avoid mistakes when 
coding the toolstack part of xsplice.

Roger.

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

Re: Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[Konrad Rzeszutek Wilk]

On Fri, Apr 29, 2016 at 06:16:19PM +0200, Roger Pau Monne wrote:
> On Fri, Apr 29, 2016 at 04:30:16PM +0100, Wei Liu wrote:
> > On Fri, Apr 29, 2016 at 05:12:51PM +0200, Roger Pau Monne wrote:
> > > On Fri, Apr 29, 2016 at 04:02:33PM +0100, Wei Liu wrote:
> > > > I have a gut feeling that returning XEN_ errno to userspace program is
> > > > layering violation. They should always be translated to OS level errno
> > > > by privcmd driver.
> > > 
> > > Yes, the error value returned from the hypercall executed is indeed 
> > > translated into the native OS error space. The problem here is that those 
> > > error codes are returned _inside_ of the specific hypercall struct, which 
> > > sadly privcmd doesn't know anything about.
> > > 
> > > And of course teaching privcmd about every possible hypercall struct is 
> > > simply impossible, since some of them are not stable (eg: domctls)
> > >  
> > > > Aren't FreeBSD and NetBSD already doing that?
> > > 
> > > As said above, this is only done for direct return codes, everything inside 
> > > of the struct passed to the hypercall is returned as-is.
> > > 
> > > This is a complete mess, and TBH, I don't have a clever idea about how to 
> > > solve it.
> > > 
> > 
> > Me neither. Maybe a new thread should be started to discuss this.
> 
> So here we are.
> 
> In order to put everyone into context: the issue here is that some 
> hypercalls (those that batch several operations) return an array of error 
> codes inside of the hypercall structure. This array of error codes is not 
> standardized, so the privcmd driver doesn't know anything about it, and thus 
> cannot translate it into the native OS error space.
> 
> It has also been suggested that the privcmd driver simply doesn't translate 
> error codes at all, and then let the applications figure out if the error 
> code comes from Xen or from the OS. IMHO, this is impossible to achieve, 
> because the ioctl syscall can return an error code that's been forwarded 
> by Xen or a native one, and the application has no way of knowing where is 
> it coming from.
> 
> I've identified at least the following hypercall structs that store XEN_* 
> error codes inside:
> 
>  - xen_add_to_physmap_batch
>  - xen_xsplice_status
> 
> TBH, it's quite hard to spot them, so I might have missed some. 
> 
> xen_add_to_physmap_batch is part of the public ABI, and cannot be changed. 
> On the bright side, xen_add_to_physmap_batch is implemented as a different 
> ioctl in privcmd usually (in order to map memory from other domains), so the 
> error translation should be handled correctly.
> 
> Then the xsplice struct that uses XEN_* values is:
> 
> struct xen_xsplice_status {
> #define XSPLICE_STATE_CHECKED      1
> #define XSPLICE_STATE_APPLIED      2
>     uint32_t state;                /* OUT: XSPLICE_STATE_*. */
>     int32_t rc;                    /* OUT: 0 if no error, otherwise -XEN_EXX. */
> };
> 
> Which is in turn used by:
> 
> struct xen_sysctl_xsplice_list {
>     uint32_t version;                       /* OUT: Hypervisor stamps value.
>                                                If varies between calls, we are
>                                              * getting stale data. */
>     uint32_t idx;                           /* IN: Index into hypervisor list. */
>     uint32_t nr;                            /* IN: How many status, name, and len
>                                                should fill out. Can be zero to get
>                                                amount of payloads and version.
>                                                OUT: How many payloads left. */
>     uint32_t pad;                           /* IN: Must be zero. */
>     XEN_GUEST_HANDLE_64(xen_xsplice_status_t) status;  /* OUT. Must have enough
>                                                space allocate for nr of them. */
>     XEN_GUEST_HANDLE_64(char) name;         /* OUT: Array of names. Each member
>                                                MUST XEN_XSPLICE_NAME_SIZE in size.
>                                                Must have nr of them. */
>     XEN_GUEST_HANDLE_64(uint32) len;        /* OUT: Array of lengths of name's.
>                                                Must have nr of them. */
> };
> 
> IMHO, the best way to solve this is to define a set of XSPLICE_ERROR_* that 
> covers the error codes returned by xsplice, and use that instead of XEN_* 
> errno values. This would make it much more easier to avoid mistakes when 
> coding the toolstack part of xsplice.

But why?

I must be missing something here - but the return from the hypercall
can return say 0 but the status->rc can be -XEN_EAGAIN.

Why does it need to be XSPLICE_ERROR_?

> 
> Roger.

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

Re: Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[Roger Pau Monne]

On Fri, Apr 29, 2016 at 12:22:32PM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Apr 29, 2016 at 06:16:19PM +0200, Roger Pau Monne wrote:
> > On Fri, Apr 29, 2016 at 04:30:16PM +0100, Wei Liu wrote:
> > > On Fri, Apr 29, 2016 at 05:12:51PM +0200, Roger Pau Monne wrote:
> > > > On Fri, Apr 29, 2016 at 04:02:33PM +0100, Wei Liu wrote:
> > > > > I have a gut feeling that returning XEN_ errno to userspace program is
> > > > > layering violation. They should always be translated to OS level errno
> > > > > by privcmd driver.
> > > > 
> > > > Yes, the error value returned from the hypercall executed is indeed 
> > > > translated into the native OS error space. The problem here is that those 
> > > > error codes are returned _inside_ of the specific hypercall struct, which 
> > > > sadly privcmd doesn't know anything about.
> > > > 
> > > > And of course teaching privcmd about every possible hypercall struct is 
> > > > simply impossible, since some of them are not stable (eg: domctls)
> > > >  
> > > > > Aren't FreeBSD and NetBSD already doing that?
> > > > 
> > > > As said above, this is only done for direct return codes, everything inside 
> > > > of the struct passed to the hypercall is returned as-is.
> > > > 
> > > > This is a complete mess, and TBH, I don't have a clever idea about how to 
> > > > solve it.
> > > > 
> > > 
> > > Me neither. Maybe a new thread should be started to discuss this.
> > 
> > So here we are.
> > 
> > In order to put everyone into context: the issue here is that some 
> > hypercalls (those that batch several operations) return an array of error 
> > codes inside of the hypercall structure. This array of error codes is not 
> > standardized, so the privcmd driver doesn't know anything about it, and thus 
> > cannot translate it into the native OS error space.
> > 
> > It has also been suggested that the privcmd driver simply doesn't translate 
> > error codes at all, and then let the applications figure out if the error 
> > code comes from Xen or from the OS. IMHO, this is impossible to achieve, 
> > because the ioctl syscall can return an error code that's been forwarded 
> > by Xen or a native one, and the application has no way of knowing where is 
> > it coming from.
> > 
> > I've identified at least the following hypercall structs that store XEN_* 
> > error codes inside:
> > 
> >  - xen_add_to_physmap_batch
> >  - xen_xsplice_status
> > 
> > TBH, it's quite hard to spot them, so I might have missed some. 
> > 
> > xen_add_to_physmap_batch is part of the public ABI, and cannot be changed. 
> > On the bright side, xen_add_to_physmap_batch is implemented as a different 
> > ioctl in privcmd usually (in order to map memory from other domains), so the 
> > error translation should be handled correctly.
> > 
> > Then the xsplice struct that uses XEN_* values is:
> > 
> > struct xen_xsplice_status {
> > #define XSPLICE_STATE_CHECKED      1
> > #define XSPLICE_STATE_APPLIED      2
> >     uint32_t state;                /* OUT: XSPLICE_STATE_*. */
> >     int32_t rc;                    /* OUT: 0 if no error, otherwise -XEN_EXX. */
> > };
> > 
> > Which is in turn used by:
> > 
> > struct xen_sysctl_xsplice_list {
> >     uint32_t version;                       /* OUT: Hypervisor stamps value.
> >                                                If varies between calls, we are
> >                                              * getting stale data. */
> >     uint32_t idx;                           /* IN: Index into hypervisor list. */
> >     uint32_t nr;                            /* IN: How many status, name, and len
> >                                                should fill out. Can be zero to get
> >                                                amount of payloads and version.
> >                                                OUT: How many payloads left. */
> >     uint32_t pad;                           /* IN: Must be zero. */
> >     XEN_GUEST_HANDLE_64(xen_xsplice_status_t) status;  /* OUT. Must have enough
> >                                                space allocate for nr of them. */
> >     XEN_GUEST_HANDLE_64(char) name;         /* OUT: Array of names. Each member
> >                                                MUST XEN_XSPLICE_NAME_SIZE in size.
> >                                                Must have nr of them. */
> >     XEN_GUEST_HANDLE_64(uint32) len;        /* OUT: Array of lengths of name's.
> >                                                Must have nr of them. */
> > };
> > 
> > IMHO, the best way to solve this is to define a set of XSPLICE_ERROR_* that 
> > covers the error codes returned by xsplice, and use that instead of XEN_* 
> > errno values. This would make it much more easier to avoid mistakes when 
> > coding the toolstack part of xsplice.
> 
> But why?
> 
> I must be missing something here - but the return from the hypercall
> can return say 0 but the status->rc can be -XEN_EAGAIN.
> 
> Why does it need to be XSPLICE_ERROR_?

Because nobody uses or enforces the correct usage of XEN_E* in the tools, so 
people just use native error codes, which works on Linux, but breaks on 
other OSes.

Using something like XSPLICE_ERROR_* prevents people from having the bad 
habit of directly using native OS error codes, by making it more obvious 
that the error code is in a different space.

Roger.

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

Re: Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[Konrad Rzeszutek Wilk]

> > > IMHO, the best way to solve this is to define a set of XSPLICE_ERROR_* that 
> > > covers the error codes returned by xsplice, and use that instead of XEN_* 
> > > errno values. This would make it much more easier to avoid mistakes when 
> > > coding the toolstack part of xsplice.
> > 
> > But why?
> > 
> > I must be missing something here - but the return from the hypercall
> > can return say 0 but the status->rc can be -XEN_EAGAIN.
> > 
> > Why does it need to be XSPLICE_ERROR_?
> 
> Because nobody uses or enforces the correct usage of XEN_E* in the tools, so 
> people just use native error codes, which works on Linux, but breaks on 
> other OSes.

That was an oversigh on my part. I think changing the error code handling
in xen-xsplice to look for XEN_EXX is the right way.

But are you saying the BSD does not enforce the POSIX errors? errno.h is POSIX right?

> 
> Using something like XSPLICE_ERROR_* prevents people from having the bad 
> habit of directly using native OS error codes, by making it more obvious 
> that the error code is in a different space.

> 
> Roger.

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

Re: Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[Roger Pau Monne]

On Fri, Apr 29, 2016 at 12:59:30PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > IMHO, the best way to solve this is to define a set of XSPLICE_ERROR_* that 
> > > > covers the error codes returned by xsplice, and use that instead of XEN_* 
> > > > errno values. This would make it much more easier to avoid mistakes when 
> > > > coding the toolstack part of xsplice.
> > > 
> > > But why?
> > > 
> > > I must be missing something here - but the return from the hypercall
> > > can return say 0 but the status->rc can be -XEN_EAGAIN.
> > > 
> > > Why does it need to be XSPLICE_ERROR_?
> > 
> > Because nobody uses or enforces the correct usage of XEN_E* in the tools, so 
> > people just use native error codes, which works on Linux, but breaks on 
> > other OSes.
> 
> That was an oversigh on my part. I think changing the error code handling
> in xen-xsplice to look for XEN_EXX is the right way.
> 
> But are you saying the BSD does not enforce the POSIX errors? errno.h is POSIX right?

Yes, BSD uses POSIX error codes just as Linux, the issue is that the values 
differ. For example on Xen and Linux EAGAIN is 11, on FreeBSD EAGAIN is 35.

Roger.

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

Re: Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[Konrad Rzeszutek Wilk]

On Fri, Apr 29, 2016 at 07:07:44PM +0200, Roger Pau Monne wrote:
> On Fri, Apr 29, 2016 at 12:59:30PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > > IMHO, the best way to solve this is to define a set of XSPLICE_ERROR_* that 
> > > > > covers the error codes returned by xsplice, and use that instead of XEN_* 
> > > > > errno values. This would make it much more easier to avoid mistakes when 
> > > > > coding the toolstack part of xsplice.
> > > > 
> > > > But why?
> > > > 
> > > > I must be missing something here - but the return from the hypercall
> > > > can return say 0 but the status->rc can be -XEN_EAGAIN.
> > > > 
> > > > Why does it need to be XSPLICE_ERROR_?
> > > 
> > > Because nobody uses or enforces the correct usage of XEN_E* in the tools, so 
> > > people just use native error codes, which works on Linux, but breaks on 
> > > other OSes.
> > 
> > That was an oversigh on my part. I think changing the error code handling
> > in xen-xsplice to look for XEN_EXX is the right way.
> > 
> > But are you saying the BSD does not enforce the POSIX errors? errno.h is POSIX right?
> 
> Yes, BSD uses POSIX error codes just as Linux, the issue is that the values 
> differ. For example on Xen and Linux EAGAIN is 11, on FreeBSD EAGAIN is 35.

Wow! That is messed up. And would explain the odd errors you had seen.

But if the xen-xsplice would use XEN_EXX error handling this would work for you right?
(Which BTW, thank you for doing!)

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

Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[Jan Beulich]

>>> On 29.04.16 at 18:16, <roger.pau@citrix.com> wrote:
> On Fri, Apr 29, 2016 at 04:30:16PM +0100, Wei Liu wrote:
>> On Fri, Apr 29, 2016 at 05:12:51PM +0200, Roger Pau Monne wrote:
>> > On Fri, Apr 29, 2016 at 04:02:33PM +0100, Wei Liu wrote:
>> > > I have a gut feeling that returning XEN_ errno to userspace program is
>> > > layering violation. They should always be translated to OS level errno
>> > > by privcmd driver.
>> > 
>> > Yes, the error value returned from the hypercall executed is indeed 
>> > translated into the native OS error space. The problem here is that those 
>> > error codes are returned _inside_ of the specific hypercall struct, which 
>> > sadly privcmd doesn't know anything about.
>> > 
>> > And of course teaching privcmd about every possible hypercall struct is 
>> > simply impossible, since some of them are not stable (eg: domctls)
>> >  
>> > > Aren't FreeBSD and NetBSD already doing that?
>> > 
>> > As said above, this is only done for direct return codes, everything inside 
> 
>> > of the struct passed to the hypercall is returned as-is.
>> > 
>> > This is a complete mess, and TBH, I don't have a clever idea about how to 
>> > solve it.
>> > 
>> 
>> Me neither. Maybe a new thread should be started to discuss this.
> 
> So here we are.
> 
> In order to put everyone into context: the issue here is that some 
> hypercalls (those that batch several operations) return an array of error 
> codes inside of the hypercall structure. This array of error codes is not 
> standardized, so the privcmd driver doesn't know anything about it, and thus 
> cannot translate it into the native OS error space.

I don't see why these would need translation: Anything in a structure
field simply is in XEN_E* space, and the caller can be easily aware.

> It has also been suggested that the privcmd driver simply doesn't translate 
> error codes at all, and then let the applications figure out if the error 
> code comes from Xen or from the OS. IMHO, this is impossible to achieve, 
> because the ioctl syscall can return an error code that's been forwarded 
> by Xen or a native one, and the application has no way of knowing where is 
> it coming from.

But that's a separate issue from that of passing such values in
structure fields.

Jan


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

Re: Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[David Vrabel]

On 29/04/16 17:16, Roger Pau Monne wrote:
> 
> It has also been suggested that the privcmd driver simply doesn't translate 
> error codes at all, and then let the applications figure out if the error 
> code comes from Xen or from the OS. IMHO, this is impossible to achieve, 
> because the ioctl syscall can return an error code that's been forwarded 
> by Xen or a native one, and the application has no way of knowing where is 
> it coming from.

The privcmd driver could return the hypercall error in a parameter and
the ioctl return value is for driver errors.  For example:

IOCTL_PRIVCMD_HYPERCALL_V2 takes a

struct xen_privcmd_hypercall_v2 {
    uint64_t op
    uint64_t arg[5]
    int64_t  ret;
};

This moves all the error code translation into userspace.

David

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