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

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Mon, Jan 14, 2019 at 7:12 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> 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.

I've left this as is (ie. unsigned int) but I can change it if it should change.

>
> >>             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.

ack, done

>
> >> +
> >> +    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.

ack

Christopher

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