[PATCH v2 0/2] public/io: xs_wire: Allow Xenstore to report EPERM

[PATCH v2 0/2] public/io: xs_wire: Allow Xenstore to report EPERM

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

This small patch series allows xenstore to report EPERM (see patch #2).
Patch #1 was introduced to avoid someone else making the mistake to
introduce a new error before EINVAL.

Cheers,

Julien Grall (2):
  public/io: xs_wire: Document that EINVAL should always be first in
    xsd_errors
  public/io: xs_wire: Allow Xenstore to report EPERM

 xen/include/public/io/xs_wire.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.32.0

[PATCH v2 1/2] public/io: xs_wire: Document that EINVAL should always be first in xsd_errors

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Some tools (e.g. xenstored) always expect EINVAL to be first in xsd_errors.

Document it so, one doesn't add a new entry before hand by mistake.

Signed-off-by: Julien Grall <jgrall@amazon.com>

----

I have tried to add a BUILD_BUG_ON() but GCC complained that the value
was not a constant. I couldn't figure out a way to make GCC happy.

Changes in v2:
    - New patch
---
 xen/include/public/io/xs_wire.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
index c1ec7c73e3b1..dd4c9c9b972d 100644
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
 __attribute__((unused))
 #endif
     = {
+    /* /!\ Some users (e.g. xenstored) expect EINVAL to be the first entry. */
     XSD_ERROR(EINVAL),
     XSD_ERROR(EACCES),
     XSD_ERROR(EEXIST),
-- 
2.32.0

[PATCH v2 2/2] public/io: xs_wire: Allow Xenstore to report EPERM

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

C Xenstored is using EPERM when the client is not allowed to change
the owner (see GET_PERMS). However, the xenstore protocol doesn't
describe EPERM so EINVAL will be sent to the client.

When writing test, it would be useful to differentiate between EINVAL
(e.g. parsing error) and EPERM (i.e. no permission). So extend
xsd_errors[] to support return EPERM.

Looking at previous time xsd_errors was extended (8b2c441a1b), it was
considered to be safe to add a new error because at least Linux driver
and libxenstore treat an unknown error code as EINVAL.

This statement doesn't cover other possible OSes, however I am not
aware of any breakage.

Signed-off-by: Julien Grall <jgrall@amazon.com>

----

Changes in v2:
    - Define EPERM at the end of xsd_errors
---
 xen/include/public/io/xs_wire.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
index dd4c9c9b972d..211770911d9b 100644
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -91,7 +91,8 @@ __attribute__((unused))
     XSD_ERROR(EBUSY),
     XSD_ERROR(EAGAIN),
     XSD_ERROR(EISCONN),
-    XSD_ERROR(E2BIG)
+    XSD_ERROR(E2BIG),
+    XSD_ERROR(EPERM),
 };
 #endif
 
-- 
2.32.0

Re: [PATCH v2 1/2] public/io: xs_wire: Document that EINVAL should always be first in xsd_errors

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2052 bytes --]

On 27.06.22 14:36, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Some tools (e.g. xenstored) always expect EINVAL to be first in xsd_errors.
> 
> Document it so, one doesn't add a new entry before hand by mistake.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ----
> 
> I have tried to add a BUILD_BUG_ON() but GCC complained that the value
> was not a constant. I couldn't figure out a way to make GCC happy.
> 
> Changes in v2:
>      - New patch
> ---
>   xen/include/public/io/xs_wire.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
> index c1ec7c73e3b1..dd4c9c9b972d 100644
> --- a/xen/include/public/io/xs_wire.h
> +++ b/xen/include/public/io/xs_wire.h
> @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
>   __attribute__((unused))
>   #endif
>       = {
> +    /* /!\ Some users (e.g. xenstored) expect EINVAL to be the first entry. */
>       XSD_ERROR(EINVAL),
>       XSD_ERROR(EACCES),
>       XSD_ERROR(EEXIST),

What about another approach, like:

--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -746,16 +746,17 @@ unsigned int get_strings(struct buffered_data *data,
  static void send_error(struct connection *conn, int error)
  {
         unsigned int i;
+       char *err = NULL;

         for (i = 0; error != xsd_errors[i].errnum; i++) {
                 if (i == ARRAY_SIZE(xsd_errors) - 1) {
                         eprintf("xenstored: error %i untranslatable", error);
-                       i = 0; /* EINVAL */
+                       err = "EINVAL";
                         break;
                 }
         }
-       send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
-                         strlen(xsd_errors[i].errstring) + 1);
+       err = err ? : xsd_errors[i].errstring;
+       send_reply(conn, XS_ERROR, err, strlen(err) + 1);
  }

  void send_reply(struct connection *conn, enum xsd_sockmsg_type type,


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 1/2] public/io: xs_wire: Document that EINVAL should always be first in xsd_errors

[Julien Grall]

Hi,

On 27/06/2022 15:31, Juergen Gross wrote:
> On 27.06.22 14:36, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Some tools (e.g. xenstored) always expect EINVAL to be first in 
>> xsd_errors.
>>
>> Document it so, one doesn't add a new entry before hand by mistake.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ----
>>
>> I have tried to add a BUILD_BUG_ON() but GCC complained that the value
>> was not a constant. I couldn't figure out a way to make GCC happy.
>>
>> Changes in v2:
>>      - New patch
>> ---
>>   xen/include/public/io/xs_wire.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/include/public/io/xs_wire.h 
>> b/xen/include/public/io/xs_wire.h
>> index c1ec7c73e3b1..dd4c9c9b972d 100644
>> --- a/xen/include/public/io/xs_wire.h
>> +++ b/xen/include/public/io/xs_wire.h
>> @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
>>   __attribute__((unused))
>>   #endif
>>       = {
>> +    /* /!\ Some users (e.g. xenstored) expect EINVAL to be the first 
>> entry. */
>>       XSD_ERROR(EINVAL),
>>       XSD_ERROR(EACCES),
>>       XSD_ERROR(EEXIST),
> 
> What about another approach, like:

In place of what? I still think we need the comment because this 
assumption is not part of the ABI (AFAICT xs_wire.h is meant to be stable).

At which point, I see limited reason to fix xenstored_core.c.

But I would have really prefer to use a BUILD_BUG_ON() (or similar) so 
we can catch any misue a build. Maybe I should write a small program 
that is executed at compile time?

Cheers,

-- 
Julien Grall

Re: [PATCH v2 1/2] public/io: xs_wire: Document that EINVAL should always be first in xsd_errors

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1795 bytes --]

On 27.06.22 16:48, Julien Grall wrote:
> Hi,
> 
> On 27/06/2022 15:31, Juergen Gross wrote:
>> On 27.06.22 14:36, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Some tools (e.g. xenstored) always expect EINVAL to be first in xsd_errors.
>>>
>>> Document it so, one doesn't add a new entry before hand by mistake.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>
>>> ----
>>>
>>> I have tried to add a BUILD_BUG_ON() but GCC complained that the value
>>> was not a constant. I couldn't figure out a way to make GCC happy.
>>>
>>> Changes in v2:
>>>      - New patch
>>> ---
>>>   xen/include/public/io/xs_wire.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
>>> index c1ec7c73e3b1..dd4c9c9b972d 100644
>>> --- a/xen/include/public/io/xs_wire.h
>>> +++ b/xen/include/public/io/xs_wire.h
>>> @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
>>>   __attribute__((unused))
>>>   #endif
>>>       = {
>>> +    /* /!\ Some users (e.g. xenstored) expect EINVAL to be the first entry. */
>>>       XSD_ERROR(EINVAL),
>>>       XSD_ERROR(EACCES),
>>>       XSD_ERROR(EEXIST),
>>
>> What about another approach, like:
> 
> In place of what? I still think we need the comment because this assumption is 
> not part of the ABI (AFAICT xs_wire.h is meant to be stable).
> 
> At which point, I see limited reason to fix xenstored_core.c.
> 
> But I would have really prefer to use a BUILD_BUG_ON() (or similar) so we can 
> catch any misue a build. Maybe I should write a small program that is executed 
> at compile time?

My suggestion removes the need for EINVAL being the first entry.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 2/2] public/io: xs_wire: Allow Xenstore to report EPERM

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 891 bytes --]

On 27.06.22 14:36, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> C Xenstored is using EPERM when the client is not allowed to change
> the owner (see GET_PERMS). However, the xenstore protocol doesn't
> describe EPERM so EINVAL will be sent to the client.
> 
> When writing test, it would be useful to differentiate between EINVAL
> (e.g. parsing error) and EPERM (i.e. no permission). So extend
> xsd_errors[] to support return EPERM.
> 
> Looking at previous time xsd_errors was extended (8b2c441a1b), it was
> considered to be safe to add a new error because at least Linux driver
> and libxenstore treat an unknown error code as EINVAL.
> 
> This statement doesn't cover other possible OSes, however I am not
> aware of any breakage.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 1/2] public/io: xs_wire: Document that EINVAL should always be first in xsd_errors

[Julien Grall]

Hi Juergen,

On 27/06/2022 15:50, Juergen Gross wrote:
> On 27.06.22 16:48, Julien Grall wrote:
>> Hi,
>>
>> On 27/06/2022 15:31, Juergen Gross wrote:
>>> On 27.06.22 14:36, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Some tools (e.g. xenstored) always expect EINVAL to be first in 
>>>> xsd_errors.
>>>>
>>>> Document it so, one doesn't add a new entry before hand by mistake.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>
>>>> ----
>>>>
>>>> I have tried to add a BUILD_BUG_ON() but GCC complained that the value
>>>> was not a constant. I couldn't figure out a way to make GCC happy.
>>>>
>>>> Changes in v2:
>>>>      - New patch
>>>> ---
>>>>   xen/include/public/io/xs_wire.h | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/xen/include/public/io/xs_wire.h 
>>>> b/xen/include/public/io/xs_wire.h
>>>> index c1ec7c73e3b1..dd4c9c9b972d 100644
>>>> --- a/xen/include/public/io/xs_wire.h
>>>> +++ b/xen/include/public/io/xs_wire.h
>>>> @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
>>>>   __attribute__((unused))
>>>>   #endif
>>>>       = {
>>>> +    /* /!\ Some users (e.g. xenstored) expect EINVAL to be the 
>>>> first entry. */
>>>>       XSD_ERROR(EINVAL),
>>>>       XSD_ERROR(EACCES),
>>>>       XSD_ERROR(EEXIST),
>>>
>>> What about another approach, like:
>>
>> In place of what? I still think we need the comment because this 
>> assumption is not part of the ABI (AFAICT xs_wire.h is meant to be 
>> stable).
>>
>> At which point, I see limited reason to fix xenstored_core.c.
>>
>> But I would have really prefer to use a BUILD_BUG_ON() (or similar) so 
>> we can catch any misue a build. Maybe I should write a small program 
>> that is executed at compile time?
> 
> My suggestion removes the need for EINVAL being the first entry

xsd_errors[] is part of the stable ABI. If Xenstored is already 
"misusing" it, then I wouldn't be surprised if other software rely on 
this as well.

Therefore, I don't really see how fixing Xenstored would allow us to 
remove this restriction.

The only reason this was spotted is by Jan reviewing C Xenstored. 
Without that, it would have problably took a long time to notice
this change (I don't think there are many other errno used by Xenstored
and xsd_errors). So I think the risk is not worth the effort.

At least, this is not a patch I would be willing to have my name on 
(either as a signed-off-by or acked-by).

Cheers,

-- 
Julien Grall

Re: [PATCH v2 1/2] public/io: xs_wire: Document that EINVAL should always be first in xsd_errors

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3383 bytes --]

On 27.06.22 17:03, Julien Grall wrote:
> Hi Juergen,
> 
> On 27/06/2022 15:50, Juergen Gross wrote:
>> On 27.06.22 16:48, Julien Grall wrote:
>>> Hi,
>>>
>>> On 27/06/2022 15:31, Juergen Gross wrote:
>>>> On 27.06.22 14:36, Julien Grall wrote:
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>
>>>>> Some tools (e.g. xenstored) always expect EINVAL to be first in xsd_errors.
>>>>>
>>>>> Document it so, one doesn't add a new entry before hand by mistake.
>>>>>
>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>>
>>>>> ----
>>>>>
>>>>> I have tried to add a BUILD_BUG_ON() but GCC complained that the value
>>>>> was not a constant. I couldn't figure out a way to make GCC happy.
>>>>>
>>>>> Changes in v2:
>>>>>      - New patch
>>>>> ---
>>>>>   xen/include/public/io/xs_wire.h | 1 +
>>>>>   1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
>>>>> index c1ec7c73e3b1..dd4c9c9b972d 100644
>>>>> --- a/xen/include/public/io/xs_wire.h
>>>>> +++ b/xen/include/public/io/xs_wire.h
>>>>> @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
>>>>>   __attribute__((unused))
>>>>>   #endif
>>>>>       = {
>>>>> +    /* /!\ Some users (e.g. xenstored) expect EINVAL to be the first 
>>>>> entry. */
>>>>>       XSD_ERROR(EINVAL),
>>>>>       XSD_ERROR(EACCES),
>>>>>       XSD_ERROR(EEXIST),
>>>>
>>>> What about another approach, like:
>>>
>>> In place of what? I still think we need the comment because this assumption 
>>> is not part of the ABI (AFAICT xs_wire.h is meant to be stable).
>>>
>>> At which point, I see limited reason to fix xenstored_core.c.
>>>
>>> But I would have really prefer to use a BUILD_BUG_ON() (or similar) so we can 
>>> catch any misue a build. Maybe I should write a small program that is 
>>> executed at compile time?
>>
>> My suggestion removes the need for EINVAL being the first entry
> 
> xsd_errors[] is part of the stable ABI. If Xenstored is already "misusing" it, 
> then I wouldn't be surprised if other software rely on this as well.

Xenstored is the only instance which needs a translation from value to
string, while all other users should need only the opposite direction.
The only other candidate would be oxenstored, but that seems not to use
xsd_errors[].

And in fact libxenstore will just return a plain EINVAL in case it
can't find a translation, while hvmloader will return EIO in that case.

With your reasoning and the hvmloader use case you could argue that
the EIO entry needs to stay at the same position, too.

> Therefore, I don't really see how fixing Xenstored would allow us to remove this 
> restriction.
> 
> The only reason this was spotted is by Jan reviewing C Xenstored. Without that, 
> it would have problably took a long time to notice
> this change (I don't think there are many other errno used by Xenstored
> and xsd_errors). So I think the risk is not worth the effort.

I don't see a real risk here, but if there is consensus that the risk should
not be taken, then I'd rather add a comment that new entries are only allowed
to be added at the end of the array.

> At least, this is not a patch I would be willing to have my name on (either as a 
> signed-off-by or acked-by).

Fair enough. :-)


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 1/2] public/io: xs_wire: Document that EINVAL should always be first in xsd_errors

[Julien Grall]

Hi Juergen,

On 27/06/2022 16:13, Juergen Gross wrote:
> On 27.06.22 17:03, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 27/06/2022 15:50, Juergen Gross wrote:
>>> On 27.06.22 16:48, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 27/06/2022 15:31, Juergen Gross wrote:
>>>>> On 27.06.22 14:36, Julien Grall wrote:
>>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>>
>>>>>> Some tools (e.g. xenstored) always expect EINVAL to be first in 
>>>>>> xsd_errors.
>>>>>>
>>>>>> Document it so, one doesn't add a new entry before hand by mistake.
>>>>>>
>>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>>>
>>>>>> ----
>>>>>>
>>>>>> I have tried to add a BUILD_BUG_ON() but GCC complained that the 
>>>>>> value
>>>>>> was not a constant. I couldn't figure out a way to make GCC happy.
>>>>>>
>>>>>> Changes in v2:
>>>>>>      - New patch
>>>>>> ---
>>>>>>   xen/include/public/io/xs_wire.h | 1 +
>>>>>>   1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/xen/include/public/io/xs_wire.h 
>>>>>> b/xen/include/public/io/xs_wire.h
>>>>>> index c1ec7c73e3b1..dd4c9c9b972d 100644
>>>>>> --- a/xen/include/public/io/xs_wire.h
>>>>>> +++ b/xen/include/public/io/xs_wire.h
>>>>>> @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
>>>>>>   __attribute__((unused))
>>>>>>   #endif
>>>>>>       = {
>>>>>> +    /* /!\ Some users (e.g. xenstored) expect EINVAL to be the 
>>>>>> first entry. */
>>>>>>       XSD_ERROR(EINVAL),
>>>>>>       XSD_ERROR(EACCES),
>>>>>>       XSD_ERROR(EEXIST),
>>>>>
>>>>> What about another approach, like:
>>>>
>>>> In place of what? I still think we need the comment because this 
>>>> assumption is not part of the ABI (AFAICT xs_wire.h is meant to be 
>>>> stable).
>>>>
>>>> At which point, I see limited reason to fix xenstored_core.c.
>>>>
>>>> But I would have really prefer to use a BUILD_BUG_ON() (or similar) 
>>>> so we can catch any misue a build. Maybe I should write a small 
>>>> program that is executed at compile time?
>>>
>>> My suggestion removes the need for EINVAL being the first entry
>>
>> xsd_errors[] is part of the stable ABI. If Xenstored is already 
>> "misusing" it, then I wouldn't be surprised if other software rely on 
>> this as well.
> 
> Xenstored is the only instance which needs a translation from value to
> string, while all other users should need only the opposite direction.
> The only other candidate would be oxenstored, but that seems not to use
> xsd_errors[].

That's assuming this is the only two implementation of Xenstored 
existing ;).

> 
> And in fact libxenstore will just return a plain EINVAL in case it
> can't find a translation, while hvmloader will return EIO in that case. >
> With your reasoning and the hvmloader use case you could argue that
> the EIO entry needs to stay at the same position, too.

I have looked at the hmvloader code. It doesn't seem to expect EIO to be 
at a specific position.

However, I do agree that it is probably best to keep each error at the 
same position.

> 
>> Therefore, I don't really see how fixing Xenstored would allow us to 
>> remove this restriction.
>>
>> The only reason this was spotted is by Jan reviewing C Xenstored. 
>> Without that, it would have problably took a long time to notice
>> this change (I don't think there are many other errno used by Xenstored
>> and xsd_errors). So I think the risk is not worth the effort.
> 
> I don't see a real risk here, but if there is consensus that the risk 
> should
> not be taken, then I'd rather add a comment that new entries are only 
> allowed
> to be added at the end of the array.

I would be fine to mandate that new errors should be added at the end of 
the array.

Cheersm

-- 
Julien Grall

Re: [PATCH v2 2/2] public/io: xs_wire: Allow Xenstore to report EPERM

[Julien Grall]

Hi Juergen,

On 27/06/2022 15:52, Juergen Gross wrote:
> On 27.06.22 14:36, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> C Xenstored is using EPERM when the client is not allowed to change
>> the owner (see GET_PERMS). However, the xenstore protocol doesn't
>> describe EPERM so EINVAL will be sent to the client.
>>
>> When writing test, it would be useful to differentiate between EINVAL
>> (e.g. parsing error) and EPERM (i.e. no permission). So extend
>> xsd_errors[] to support return EPERM.
>>
>> Looking at previous time xsd_errors was extended (8b2c441a1b), it was
>> considered to be safe to add a new error because at least Linux driver
>> and libxenstore treat an unknown error code as EINVAL.
>>
>> This statement doesn't cover other possible OSes, however I am not
>> aware of any breakage.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks. I have committed this patch and respin the first one.

Cheers,

-- 
Julien Grall