Re: [PATCH v2 3/5] xen/version: Introduce non-truncating XENVER_* subops

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.01.2023 22:47, Andrew Cooper wrote:
> On 16/01/2023 4:06 pm, Jan Beulich wrote:
>> On 14.01.2023 00:08, Andrew Cooper wrote:
>>> @@ -470,6 +471,59 @@ static int __init cf_check param_init(void)
>>>  __initcall(param_init);
>>>  #endif
>>>  
>>> +static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>> +{
>>> +    struct xen_varbuf user_str;
>>> +    const char *str = NULL;
>> This takes away from the compiler any chance of reporting "str" as
>> uninitialized
> 
> Yes...
> 
> It is also the classic false positive pattern in GCC 4.x which is still
> supported despite attempts to retire it.

Hmm, I didn't think this was the classic pattern, but instead it was when
there are two variables, where the value of one identifies whether the
other was (also) initialized. But yes, if proven to be needed for 4.x,
then keeping it there would be unavoidable for the time being (but we
should then remind ourselves to drop this once we've raised the baseline,
perhaps short of adding a gcc version conditional around the initializer
right away).

>>> +    if ( sz > KB(64) ) /* Arbitrary limit.  Avoid long-running operations. 
>>> */
>>> +        return -E2BIG;
>>> +
>>> +    if ( guest_handle_is_null(arg) ) /* Length request */
>>> +        return sz;
>>> +
>>> +    if ( copy_from_guest(&user_str, arg, 1) )
>>> +        return -EFAULT;
>>> +
>>> +    if ( user_str.len == 0 )
>>> +        return -EINVAL;
>>> +
>>> +    if ( sz > user_str.len )
>>> +        return -ENOBUFS;
>> The earlier of these last two checks makes it that one can't successfully
>> call this function when the size query has returned 0.
> 
> This is actually a check that the build_id path already has.  I did
> consider it somewhat dubious to special case 0 here, but it needs to
> stay for the following patch to have no functional change.

Would such a minor functional change actually be a problem there?

>>> --- 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 instead.
>> Personally I don't like these "broken" that you're adding. These interfaces
>> simply are the way they are, with certain limitations. We also won't be
>> able to remove the old variants (except in the new ABI), so telling people
>> to avoid them provides us about nothing.
> 
> Incorrect.
> 
> First, the breakage here isn't only truncation; it's char-signedness
> with data that's not guaranteed to be ASCII text.  Yet another
> demonstration of why C is an inappropriate way of defining an ABI.
> 
> Secondly, it is unreasonable for ABI errors and correction information
> such as this not to be documented *somewhere*.  It should live with the
> API technical reference, which happens to be exactly (and only) here.

I agree with spelling out shortcomings and restrictions. The only thing
I do not agree with is the use of the word "broken" (or anything to the
same effect). Instead of using that word, how about you actually spell
out what the limitations are, so that people have grounds to pick
between these and the new interfaces you're adding (and being nudged
towards the latter)?

Jan