Re: [PATCH for-4.17?] x86/paging: return -EINVAL for paging domctls for dying domains

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 8 Nov 2022, at 16:43, Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
> 
> On Tue, Nov 08, 2022 at 05:14:40PM +0100, Jan Beulich wrote:
>> On 08.11.2022 12:38, Roger Pau Monne wrote:
>>> Like on the Arm side, return -EINVAL when attempting to do a p2m
>>> operation on dying domains.
>>> 
>>> The current logic returns 0 and leaves the domctl parameter
>>> uninitialized for any parameter fetching operations (like the
>>> GET_ALLOCATION operation), which is not helpful from a toolstack point
>>> of view, because there's no indication that the data hasn't been
>>> fetched.
>> 
>> While I can see how the present behavior is problematic when it comes
>> to consuming supposedly returned data, ...
>> 
>>> --- a/xen/arch/x86/mm/paging.c
>>> +++ b/xen/arch/x86/mm/paging.c
>>> @@ -694,9 +694,10 @@ int paging_domctl(struct domain *d, struct 
>>> xen_domctl_shadow_op *sc,
>>> 
>>>     if ( unlikely(d->is_dying) )
>>>     {
>>> -        gdprintk(XENLOG_INFO, "Ignoring paging op on dying domain %u\n",
>>> +        gdprintk(XENLOG_INFO,
>>> +                 "Tried to do a paging domctl op on dying domain %u\n",
>>>                  d->domain_id);
>>> -        return 0;
>>> +        return -EINVAL;
>>>     }
>> 
>> ... going from "success" to "failure" here has a meaningful risk of
>> regressing callers. It is my understanding that it was deliberate to
>> mimic success in this case (without meaning to assign "good" or "bad"
>> to that decision).
> 
> I would assume that was the original intention, yes, albeit the commit
> message doesn't go into details about why mimicking success is
> required, it's very well possible the code relying on this was xend.
> 
>> Can you instead fill the data to be returned in
>> some simple enough way? I assume a mere memset() isn't going to be
>> good enough, though (albeit public/domctl.h doesn't explicitly name
>> any input-only fields, so it may not be necessary to preserve
>> anything). Maybe zeroing ->mb and ->stats would do?
> 
> Hm, it still feels kind of wrong.

Without the fix in the bindings (and any fixup in other toolstacks as needed, I 
haven't checked): 
https://lore.kernel.org/xen-devel/94f93ee61a4d0bd2fac3f5a753cb935962be20bb.1667920496.git.edvin.torok@xxxxxxxxxx/T/#u
a value of 0 here might still cause things to go subtly wrong, i.e. cause the 
HVM shadow multiplier of a VM to decrease, although I think there are 
safeguards against it going below 1.0.
However a ->mb value of all zeroes is much easier to debug (and detect!) than a 
completely uninitialised value.

I'd prefer if the return value of domctls could be trusted to mean that 0 = all 
values are initialized on output, and to be potentially uninitialised only on 
failure.
There are a lot of other calls with similar pattern (particularly around vcpu, 
which will go down a different path) in the xenctrl bindings, and I haven't 
checked them all yet, but it'd be a good rule of thumb if the behaviour was 
consistent with other hypercalls/domctls.


>  We do return errors elsewhere for
> operations attempted against dying domains, and that seems all fine,
> not sure why paging operations need to be different in this regard.
> Arm does also return -EINVAL in that case.

How about we return EINVAL *and* in DEBUG builds fill the struct with an easily 
recognizable poison value?

> 
> So what about postponing this change to 4.18 in order to avoid
> surprises, but then taking it in its current form at the start of the
> development window, as to have time to detect any issues?
> 
>> As a minor remark: _If_ you're changing the printk(), then please
>> also switch to using %pd.
> 
> I've considered this, but then printing: "Tried to do a paging domctl
> op on dying domain dX" felt kind of repetitive to me because of the
> usage of domain and dX in the same sentence.  Anyway, will adjust.
> 
> Thanks, Roger.

Thanks,
--Edwin