Re: [Xen-devel] [PATCH v3 04/15] argo: init, destroy and soft-reset, with enable command line opt

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.01.19 at 15:58, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/01/2019 07:42, Christopher Clark wrote:
>> --- a/xen/common/argo.c
>> +++ b/xen/common/argo.c
>>  long
>>  do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> 
> I know I'm commenting on the wrong patch, but please use unsigned long
> cmd, so the type definition here doesn't truncate the caller provided
> value.  We have similar buggy code all over Xen, but its too late to fix
> that, and I'd prefer not to propagate the error.

Why buggy? It all depends on how the interface is specified. If
the input is 32 bits wide, it is clear that higher bits are supposed
to be ignored. Nothing says that the full register width is
significant.

>>             XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
>>             unsigned long arg4)
>>  {
>> -    return -ENOSYS;
>> +    struct domain *currd = current->domain;
>> +    long rc = -EFAULT;
>> +
>> +    argo_dprintk("->do_argo_op(%u,%p,%p,%d,%d)\n", cmd,
>> +                 (void *)arg1.p, (void *)arg2.p, (int) arg3, (int) arg4);
> 
> For debugging purposes, you don't want to truncate any of these values,
> or you'll have a print message which doesn't match what the guest
> provided.  I'd use %ld for arg3 and arg4.

Perhaps better %lx, for the output being easier to recognize
for both bitmaps (e.g. flag values) and sufficiently large values.

>> +
>> +    if ( unlikely(!opt_argo_enabled) )
>> +    {
>> +        rc = -EOPNOTSUPP;
> 
> Shouldn't this be ENOSYS instead?  There isn't a conceptual difference
> between CONFIG_ARGO compiled out, and opt_argo clear on the command
> line, and I don't think a guest should be able to tell the difference.

I admit it's a boundary case, but I think ENOSYS should strictly
only ever be (and have been) returned for unrecognized major
hypercall numbers.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel