Re: xen | Failed pipeline for staging | 6a47ba2f

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2023 05:59, Stefano Stabellini wrote:
> xen: fix broken XEN_SYSCTL_getdomaininfolist hypercall
> 
> XEN_SYSCTL_getdomaininfolist doesn't actually update the guest
> num_domains field, only its local copy. Fix that.

This isn't true, at least not always / unconditionally. "copyback" is
what controls copying back of the entire struct, and in the success
case this looks to be happening fine. Yet for the failure case it's
unclear whether any copying back is actually intended. (If the op was
to return merely the number of active domains, I think that ought to
be restricted to max_domains == 0 and the handle also being a null
one.

I'm also having a hard time seeing what failure case the test ended
up encountering: There are only two errors which can occur - one
from the XSM hook (which is mishandled, and I'll make a separate
patch for that) and the other from failing to copy back the info for
the domain being looked at. I hope we can exclude the former, so are
you suggesting the info struct copy-back is failing in your case?

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>

In any event, if anything needs fixing here, a Fixes: tag would be
nice.

Jan

> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 02505ab044..0e1097be96 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -107,10 +107,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>          
>          rcu_read_unlock(&domlist_read_lock);
>          
> -        if ( ret != 0 )
> -            break;
> -        
>          op->u.getdomaininfolist.num_domains = num_domains;
> +        __copy_field_to_guest(u_sysctl, op, u.getdomaininfolist.num_domains);
>      }
>      break;
>