Re: [PATCH v3 02/10] xen/version: Introduce non-truncating deterministically-signed XENVER_* subops

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/08/2023 1:19 am, Stefano Stabellini wrote:
> On Tue, 15 Aug 2023, Andrew Cooper wrote:
>> @@ -498,6 +499,59 @@ static int __init cf_check param_init(void)
>>
>> +    sz = strlen(str);
>> +
>> +    if ( sz > KB(64) ) /* Arbitrary limit.  Avoid long-running operations. 
>> */
>> +        return -E2BIG;
> Realistically do we want this buffer to cross page boundaries?

A 1-byte answer can cross a page boundary, even if the hypercall
argument is correctly aligned (and even that is unspecified in the Xen ABI).

But importantly, this series is also prep work to fixing the cmdline
limit.  1k is already causing problems, and 64k is a whole lot less bad
than 4k when we enter such corner cases.

>> diff --git a/xen/include/public/version.h b/xen/include/public/version.h
>> index cbc4ef7a46e6..0dd6bbcb43cc 100644
>> --- a/xen/include/public/version.h
>> +++ b/xen/include/public/version.h
>> @@ -19,12 +19,20 @@
>>  /* arg == NULL; returns major:minor (16:16). */
>>  #define XENVER_version      0
>>  
>> -/* arg == xen_extraversion_t. */
>> +/*
>> + * arg == xen_extraversion_t.
>> + *
>> + * This API/ABI is broken.  Use XENVER_extraversion2 where possible.
> Like Jan and Julien I also don't like the word "broken" especially
> without explanation of why it is broken next to it.

The word "broken" is an accurate and appropriate word in the English
language to describe the situation.  I'm sorry you don't like it, but
unless any of you can articulate why you think it is inappropriate
phraseology, the complaint is unactionable.

Specifically ...

> Instead, I would say:
>
> "XENVER_extraversion is deprecated. Please use XENVER_extraversion2."

... depreciated is misleading.

It would be acceptable for a case where we'd introduced a foo2 to add a
new feature to the interface, but we're being forced to make the new
interface to fix two bugs and a mis-feature in existing interface.

> If you want to convey the message that the API has problems, then I would
> say:
>
> "XENVER_extraversion might cause truncation. Please use XENVER_extraversion2."
>
> Or even:
>
> "XENVER_extraversion has problems. Please use XENVER_extraversion2."

If "broken" is too nondescript, then "might" is bad too and "has
problems" is out of the question.


There is a partial description of the ABI problems in the comment block
beside the new ops.  I could be persuaded to extend it to be a full list
of the ABI breakages.

These header files are a succinct technical reference for proficient
programmers to interact with Xen.  They are not a tutorial on writing C,
nor are they appropriate places to sentences of no useful value.

Through this series, I have done the hard work of updating the
commonly-used interfaces such that downstreams can stop working around
real problems caused by real errors in these APIs.  For everyone else
re-syncing the headers, it is important that the message come across as
an instruction and not a suggestion ...

... People will probably ignore it irrespective, but that's then firmly
on them, and not on Xen trying to downplay problems in the public interface.

>> + */
>>  #define XENVER_commandline 9
>>  typedef char xen_commandline_t[1024];
>>  
>> @@ -110,6 +133,42 @@ struct xen_build_id {
>>  };
>>  typedef struct xen_build_id xen_build_id_t;
>>  
>> +/*
>> + * Container for an arbitrary variable length buffer.
>> + */
>> +struct xen_varbuf {
>> +    uint32_t len;                          /* IN:  size of buf[] in bytes. 
>> */
>> +    unsigned char buf[XEN_FLEX_ARRAY_DIM]; /* OUT: requested data.         
>> */
> I realize that you just copied struct xen_build_id

No - it was originally an ambiguously-signed char in v1.  It became
unsigned through review.

But being unsigned char is relevant to the non-ABI-changingness of later
patches in the series.

> but I recall from
> MISRA C training that we should use plain "char" for strings for good
> reasons, not "unsigned char"?

I don't recall anything to that effect, nor can I see anything obvious
when scanning through the standard.

MISRA can't plausibly prohibit the use of char for arbitrary data.  It's
the one and only thing the C spec states is safe for the task.

~Andrew