Re: [PATCH v7 04/10] xen/memory: Add a vmtrace_buf resource type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.01.2021 10:58, Andrew Cooper wrote:
> On 26/01/2021 07:37, Jan Beulich wrote:
>> On 25.01.2021 17:31, Jan Beulich wrote:
>>> On 21.01.2021 22:27, Andrew Cooper wrote:
>>>> +static int acquire_vmtrace_buf(
>>>> +    struct domain *d, unsigned int id, unsigned long frame,
>>>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>>>> +{
>>>> +    const struct vcpu *v = domain_vcpu(d, id);
>>>> +    unsigned int i;
>>>> +    mfn_t mfn;
>>>> +
>>>> +    if ( !v || !v->vmtrace.buf ||
>>>> +         nr_frames > d->vmtrace_frames ||
>>>> +         (frame + nr_frames) > d->vmtrace_frames )
>>>> +        return -EINVAL;
>>>
>>> I think that for this to guard against overflow, the first nr_frames
>>> needs to be replaced by frame (as having the wider type), or else a
>>> very large value of frame coming in will not yield the intended
>>> -EINVAL.
>> Actually, besides this then wanting to be >= instead of >, this
>> wouldn't take care of the 32-bit case (or more generally the
>> sizeof(long) == sizeof(int) one). So I think you want
>>
>>     if ( !v || !v->vmtrace.buf ||
>>          (frame + nr_frames) < frame ||
>>          (frame + nr_frames) > d->vmtrace_frames )
>>         return -EINVAL;
>>
>>> If you agree, with this changed,
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> This holds.
> 
> I slipped this buggy version in to prove a point.

IOW you've been intentionally submitting buggy code. Very
interesting.

> You're now 3 or 4 attempts into "simplifying" my original version, and
> have on at least 2 attempts made your R-b conditional on a buggy version.

In which way is the last proposed version buggy, and in which
way was the intermediate proposal problematic beyond the
aspects I did recognize myself? (I also see no problem with
taking a number of iterations to arrive at the correct result,
and I also wouldn't view this happening as an indication that
an initial comment was wrong then, unless the final result of
this iterative process matches what there was originally.)

> This form is clearly too complicated to reason about correctly, and it
> is definitely more complicated than I am happy taking.
> 
> 
> I am either going to go with my original version, which is trivially and
> obviously correct,

I've just tried to locate your "original version" in my mailbox.
I don't have an earlier patch there with this same title.
Without being able to locate the prior suggestion of mine, I'm
afraid I won't be able to verify if indeed I did suggest the
variant above before; I wouldn't consider it very likely though.
In any event I think it would have helped more if you had
proven to me where I'm wrong; I can be convinced, but calling
something "trivially and obviously correct" is not a technical
statement in such a situation. It instead feels more like a
killer phrase.

By implication, you saying "trivially and obviously correct"
can really mean only one of two things if indeed I had found a
need to comment on this same piece of code (under a different
title) earlier on: I'm trivially and obviously stupid (and
would better go away), or you're wrong with the statement (at
least in assuming what's trivial and obvious to you also
necessarily is to everyone else). I'm sorry to say it this
bluntly, but your reply above feels pretty blunt as well.

> or I'm considering reducing frame to 32 bits at the
> top level to fix this width nonsense throughout Xen.

I wouldn't mind this (and I've been wondering about the
"unsigned long" a number of times), but I'm afraid I don't see
how your construct above would be correctly rejecting all
overflowing cases then.

Jan