[PATCH 0/2] gnttab: hypervisor side XSA-448 follow-up

[PATCH 0/2] gnttab: hypervisor side XSA-448 follow-up

[Jan Beulich]

Let's try to better deal with effectively no-op requests.

1: fully ignore zero-size copy requests
2: almost fully ignore zero-size flush requests

Jan

[PATCH 1/2] gnttab: fully ignore zero-size copy requests

[Jan Beulich]

Along the line with observations in the context of XSA-448, no field in
struct gnttab_copy_ptr is relevant when no data is to be copied, much
like e.g. the pointers passed to memcpy() are irrelevant (and would
never be "validated") when the passed length is zero.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3061,6 +3061,9 @@ static int gnttab_copy_one(const struct
 {
     int rc;
 
+    if ( unlikely(!op->len) )
+        return GNTST_okay;
+
     if ( !src->domain || op->source.domid != src->ptr.domid ||
          !dest->domain || op->dest.domid != dest->ptr.domid )
     {

[PATCH 2/2] almost fully ignore zero-size flush requests

[Jan Beulich]

Along the line with observations in the context of XSA-448, besides
"op" no field is relevant when the range to be flushed is empty, much
like e.g. the pointers passed to memcpy() are irrelevant (and would
never be "validated") when the passed length is zero. Split the existing
condition validating "op", "offset", and "length", leaving only the "op"
part ahead of the check for length being zero (or no flushing to be
performed).

In the course of splitting also simplify the moved part of the condition
from 3 to 2 conditionals, potentially (depending on the architecture)
requiring one less (conditional) branch.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3528,15 +3528,16 @@ static int _cache_flush(const gnttab_cac
     void *v;
     int ret;
 
-    if ( (cflush->offset >= PAGE_SIZE) ||
-         (cflush->length > PAGE_SIZE) ||
-         (cflush->offset + cflush->length > PAGE_SIZE) ||
-         (cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN)) )
+    if ( cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN) )
         return -EINVAL;
 
     if ( cflush->length == 0 || cflush->op == 0 )
         return !*cur_ref ? 0 : -EILSEQ;
 
+    if ( (cflush->offset | cflush->length) > PAGE_SIZE ||
+         cflush->offset + cflush->length > PAGE_SIZE )
+        return -EINVAL;
+
     /* currently unimplemented */
     if ( cflush->op & GNTTAB_CACHE_SOURCE_GREF )
         return -EOPNOTSUPP;

Re: [PATCH 1/2] gnttab: fully ignore zero-size copy requests

[Julien Grall]

Hi Jan,

On 05/02/2024 11:03, Jan Beulich wrote:
> Along the line with observations in the context of XSA-448, no field in
> struct gnttab_copy_ptr is relevant when no data is to be copied, much
> like e.g. the pointers passed to memcpy() are irrelevant (and would
> never be "validated") when the passed length is zero.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[Julien Grall]

Hi Jan,

Title: I would add 'gnttab:' to clarify which subsystem you are modifying.

On 05/02/2024 11:03, Jan Beulich wrote:
> Along the line with observations in the context of XSA-448, besides
> "op" no field is relevant when the range to be flushed is empty, much
> like e.g. the pointers passed to memcpy() are irrelevant (and would
> never be "validated") when the passed length is zero. Split the existing
> condition validating "op", "offset", and "length", leaving only the "op"
> part ahead of the check for length being zero (or no flushing to be
> performed).

I am probably missing something here. I understand the theory behind 
reducing the number of checks when len == 0. But an OS cannot rely on it:
   1) older hypervisor would still return an error if the check doesn't 
pass)
   2) it does feel odd to allow "invalid" offset when len == 0 (at least.

So to me, it is better to keep those checks early. That said, I agree 
this is a matter of opinion, so I will not Nack it but also I will not 
Ack it.

> 
> In the course of splitting also simplify the moved part of the condition
> from 3 to 2 conditionals, potentially (depending on the architecture)
> requiring one less (conditional) branch.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3528,15 +3528,16 @@ static int _cache_flush(const gnttab_cac
>       void *v;
>       int ret;
>   
> -    if ( (cflush->offset >= PAGE_SIZE) ||
> -         (cflush->length > PAGE_SIZE) ||
> -         (cflush->offset + cflush->length > PAGE_SIZE) ||
> -         (cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN)) )
> +    if ( cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN) )
>           return -EINVAL;
>   
>       if ( cflush->length == 0 || cflush->op == 0 )
>           return !*cur_ref ? 0 : -EILSEQ;
>   
> +    if ( (cflush->offset | cflush->length) > PAGE_SIZE ||

This is confusing. I understand you are trying to force the compiler to 
optimize. But is it really worth it? After all, the rest of operation 
will outweight this check (cache flush are quite expensive).

We probably should take a more generic decision (and encode in our 
policy) because you seem to like this pattern and I dislike it :). Not 
sure what the others think.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[Jan Beulich]

On 19.02.2024 23:22, Julien Grall wrote:
> Title: I would add 'gnttab:' to clarify which subsystem you are modifying.

That's how I actually have it here; it's not clear to me why I lost the
prefix when sending.

> On 05/02/2024 11:03, Jan Beulich wrote:
>> Along the line with observations in the context of XSA-448, besides
>> "op" no field is relevant when the range to be flushed is empty, much
>> like e.g. the pointers passed to memcpy() are irrelevant (and would
>> never be "validated") when the passed length is zero. Split the existing
>> condition validating "op", "offset", and "length", leaving only the "op"
>> part ahead of the check for length being zero (or no flushing to be
>> performed).
> 
> I am probably missing something here. I understand the theory behind 
> reducing the number of checks when len == 0. But an OS cannot rely on it:
>    1) older hypervisor would still return an error if the check doesn't 
> pass)

Right, but that's no reason to keep the bogus earlier behavior.

>    2) it does feel odd to allow "invalid" offset when len == 0 (at least.

I'm puzzled: You've given R-b for patch 1 (thanks), where exactly the
same reasoning is used, i.e. similarly referring to memcpy() to
justify the (new / supposed) behavior.

>> In the course of splitting also simplify the moved part of the condition
>> from 3 to 2 conditionals, potentially (depending on the architecture)
>> requiring one less (conditional) branch.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -3528,15 +3528,16 @@ static int _cache_flush(const gnttab_cac
>>       void *v;
>>       int ret;
>>   
>> -    if ( (cflush->offset >= PAGE_SIZE) ||
>> -         (cflush->length > PAGE_SIZE) ||
>> -         (cflush->offset + cflush->length > PAGE_SIZE) ||
>> -         (cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN)) )
>> +    if ( cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN) )
>>           return -EINVAL;
>>   
>>       if ( cflush->length == 0 || cflush->op == 0 )
>>           return !*cur_ref ? 0 : -EILSEQ;
>>   
>> +    if ( (cflush->offset | cflush->length) > PAGE_SIZE ||
> 
> This is confusing. I understand you are trying to force the compiler to 
> optimize. But is it really worth it? After all, the rest of operation 
> will outweight this check (cache flush are quite expensive).

From purely a performance point of view it may not be worth it. From
code size angle (taken globally) I already view this differently.
Plus I think that we ought to aim at avoiding undesirable patterns,
just because people tend to clone existing code when they can. Thing
is that (as per below) the two of us apparently disagree on what
"undesirable" is in cases like this one.

> We probably should take a more generic decision (and encode in our 
> policy) because you seem to like this pattern and I dislike it :). Not 
> sure what the others think.

Perhaps. If the folding alone was the problem, I'd accept to split (or
even undo) that part. But the earlier aspect you raised also needs
sorting before I can decide whether to adjust or whether to consider
the patch rejected.

Jan

Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[Julien Grall]

Hi Jan,

On 20/02/2024 08:26, Jan Beulich wrote:
> On 19.02.2024 23:22, Julien Grall wrote:
>> Title: I would add 'gnttab:' to clarify which subsystem you are modifying.
> 
> That's how I actually have it here; it's not clear to me why I lost the
> prefix when sending.
> 
>> On 05/02/2024 11:03, Jan Beulich wrote:
>>> Along the line with observations in the context of XSA-448, besides
>>> "op" no field is relevant when the range to be flushed is empty, much
>>> like e.g. the pointers passed to memcpy() are irrelevant (and would
>>> never be "validated") when the passed length is zero. Split the existing
>>> condition validating "op", "offset", and "length", leaving only the "op"
>>> part ahead of the check for length being zero (or no flushing to be
>>> performed).
>>
>> I am probably missing something here. I understand the theory behind
>> reducing the number of checks when len == 0. But an OS cannot rely on it:
>>     1) older hypervisor would still return an error if the check doesn't
>> pass)
> 
> Right, but that's no reason to keep the bogus earlier behavior.

Hmmm... I am not sure why you say the behavior is bogus. From the commit 
message, it seems this is just an optimization that have side effect 
(ignoring the other fields).

> 
>>     2) it does feel odd to allow "invalid" offset when len == 0 (at least.
> 
> I'm puzzled: You've given R-b for patch 1 (thanks), where exactly the
> same reasoning is used, i.e. similarly referring to memcpy() to
> justify the (new / supposed) behavior.

I realize it. But I viewed it slightly different as you are adding the 
check. I think it is a good idea to add the check and ideally it should 
be after.

Here you don't seem to add any check and only re-order it. Hence why I 
treated it differently.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[Jan Beulich]

On 20.02.2024 12:52, Julien Grall wrote:
> Hi Jan,
> 
> On 20/02/2024 08:26, Jan Beulich wrote:
>> On 19.02.2024 23:22, Julien Grall wrote:
>>> Title: I would add 'gnttab:' to clarify which subsystem you are modifying.
>>
>> That's how I actually have it here; it's not clear to me why I lost the
>> prefix when sending.
>>
>>> On 05/02/2024 11:03, Jan Beulich wrote:
>>>> Along the line with observations in the context of XSA-448, besides
>>>> "op" no field is relevant when the range to be flushed is empty, much
>>>> like e.g. the pointers passed to memcpy() are irrelevant (and would
>>>> never be "validated") when the passed length is zero. Split the existing
>>>> condition validating "op", "offset", and "length", leaving only the "op"
>>>> part ahead of the check for length being zero (or no flushing to be
>>>> performed).
>>>
>>> I am probably missing something here. I understand the theory behind
>>> reducing the number of checks when len == 0. But an OS cannot rely on it:
>>>     1) older hypervisor would still return an error if the check doesn't
>>> pass)
>>
>> Right, but that's no reason to keep the bogus earlier behavior.
> 
> Hmmm... I am not sure why you say the behavior is bogus. From the commit 
> message, it seems this is just an optimization that have side effect 
> (ignoring the other fields).

I don't view this as primarily an optimization; I'm in particular after
not raising errors for cases where there is no error to be raised.
Hence the comparison to memcpy(), which you can pass "bogus" pointers
so long as you pass zero size.

>>>     2) it does feel odd to allow "invalid" offset when len == 0 (at least.
>>
>> I'm puzzled: You've given R-b for patch 1 (thanks), where exactly the
>> same reasoning is used, i.e. similarly referring to memcpy() to
>> justify the (new / supposed) behavior.
> 
> I realize it. But I viewed it slightly different as you are adding the 
> check. I think it is a good idea to add the check and ideally it should 
> be after.
> 
> Here you don't seem to add any check and only re-order it. Hence why I 
> treated it differently.

Right, there already was a zero-length check here. Just that zero
length requests still could have an error returned for no reason. So
the "optimization" part that you're talking about above was already
there, but as said, that's secondary to me.

Jan

Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[George Dunlap]

On Tue, Feb 20, 2024 at 4:26 PM Jan Beulich <jbeulich@suse.com> wrote:
> >> +    if ( (cflush->offset | cflush->length) > PAGE_SIZE ||
> >
> > This is confusing. I understand you are trying to force the compiler to
> > optimize. But is it really worth it? After all, the rest of operation
> > will outweight this check (cache flush are quite expensive).
>
> From purely a performance point of view it may not be worth it. From
> code size angle (taken globally) I already view this differently.
> Plus I think that we ought to aim at avoiding undesirable patterns,
> just because people tend to clone existing code when they can. Thing
> is that (as per below) the two of us apparently disagree on what
> "undesirable" is in cases like this one.
>
> > We probably should take a more generic decision (and encode in our
> > policy) because you seem to like this pattern and I dislike it :). Not
> > sure what the others think.

This is similar to the policy question I raised among the x86
committers a few weeks ago: You're manually specifying a more specific
behavior than is required, rather than specifying what you want and
then letting the compiler optimize things.  The problem with this is
twofold:

1. It's harder for humans to read and understand the intent

2. It ties the compiler's hands.  If you write your intent, then the
compiler is free to apply the optimization or not, or apply a
different optimization.  If you specify this optimization, then the
compiler has fewer ways that it's allowed to compile the code.

#1 by itself is probably enough to counterindicate this kind of
behavior.  Add them together, and I'm inclined to say that we should
write a policy against such optimizations, without specific
justifications.

 -George

Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[Jan Beulich]

On 21.02.2024 03:32, George Dunlap wrote:
> On Tue, Feb 20, 2024 at 4:26 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>> +    if ( (cflush->offset | cflush->length) > PAGE_SIZE ||
>>>
>>> This is confusing. I understand you are trying to force the compiler to
>>> optimize. But is it really worth it? After all, the rest of operation
>>> will outweight this check (cache flush are quite expensive).
>>
>> From purely a performance point of view it may not be worth it. From
>> code size angle (taken globally) I already view this differently.
>> Plus I think that we ought to aim at avoiding undesirable patterns,
>> just because people tend to clone existing code when they can. Thing
>> is that (as per below) the two of us apparently disagree on what
>> "undesirable" is in cases like this one.
>>
>>> We probably should take a more generic decision (and encode in our
>>> policy) because you seem to like this pattern and I dislike it :). Not
>>> sure what the others think.
> 
> This is similar to the policy question I raised among the x86
> committers a few weeks ago: You're manually specifying a more specific
> behavior than is required, rather than specifying what you want and
> then letting the compiler optimize things.  The problem with this is
> twofold:
> 
> 1. It's harder for humans to read and understand the intent

Depends.

> 2. It ties the compiler's hands.  If you write your intent, then the
> compiler is free to apply the optimization or not, or apply a
> different optimization.  If you specify this optimization, then the
> compiler has fewer ways that it's allowed to compile the code.

I'm inclined to believe that no compiler will do this kind of
optimization, unless a specific request was raised against it. The
pattern may not seem overly complex, but to recognize it would
require effort that on the whole may simply not be justified by
the gains (from the compiler's perspective).

> #1 by itself is probably enough to counterindicate this kind of
> behavior.  Add them together, and I'm inclined to say that we should
> write a policy against such optimizations, without specific
> justifications.

It's not like I didn't give any justification. So I guess you mean
without better (whatever that means) justification.

But yes, I'll undo that part of the change then and submit a v2,
albeit with not overly much hope for it to then be accepted.

Jan

Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[George Dunlap]

On Wed, Feb 21, 2024 at 3:17 PM Jan Beulich <jbeulich@suse.com> wrote:
> > #1 by itself is probably enough to counterindicate this kind of
> > behavior.  Add them together, and I'm inclined to say that we should
> > write a policy against such optimizations, without specific
> > justifications.
>
> It's not like I didn't give any justification. So I guess you mean
> without better (whatever that means) justification.

Sorry, what I meant was that the policy would have to include a sketch
for what sorts of justifications would be acceptable.

For instance, here's a justification I would consider for this sort of thing:

A. In use-case X, there is hard limit Y on the binary size.  For X's
configuration, with a reasonably small number of features enabled, we
are already close to 90% of the way there.  If we were to consistently
use this sort of manual code size optimization techniques across the
codebase, we could cut down the total size of the code base by 25%.

Here's a situation I would absolutely not consider worth it:

B. If we consistently use this sort of code size optimization
techniques across the codebase, we could cut down the entire size of
the codebase by 0.1%.  There are no hard limits, we're just trying to
generally keep things smaller.

Filling our codebase with these sorts of logic puzzles ("Why are we
binary or-ing the offset and the length?") makes it more difficult for
people to understand the code base and increases the risk of someone
making a mistake as they try to change it.  For instance, is this
change really equivalent, given that previously one of the comparisons
had >=?  It turns out yes, but only because we filter out situations
where the length is 0; what if we were to move things around again,
such that we actually can get here with length 0?

Making the binary 0.1% smaller is absolutely not worth the cost of
that.  I'm not sure even 5% would be worth that cost, given that we
don't really have any hard limits we're in danger of exceeding (at
least that I'm aware of).

But a minimum justification for allowing these sorts of things would
need to include a concrete prediction of the improvement we would get
by applying these sorts of things all over the place; not simply, "in
this instance it goes from three to two branches".

 -George

Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[Julien Grall]

Hi,

On 20/02/2024 12:25, Jan Beulich wrote:
> On 20.02.2024 12:52, Julien Grall wrote:
>> Hi Jan,
>>
>> On 20/02/2024 08:26, Jan Beulich wrote:
>>> On 19.02.2024 23:22, Julien Grall wrote:
>>>> Title: I would add 'gnttab:' to clarify which subsystem you are modifying.
>>>
>>> That's how I actually have it here; it's not clear to me why I lost the
>>> prefix when sending.
>>>
>>>> On 05/02/2024 11:03, Jan Beulich wrote:
>>>>> Along the line with observations in the context of XSA-448, besides
>>>>> "op" no field is relevant when the range to be flushed is empty, much
>>>>> like e.g. the pointers passed to memcpy() are irrelevant (and would
>>>>> never be "validated") when the passed length is zero. Split the existing
>>>>> condition validating "op", "offset", and "length", leaving only the "op"
>>>>> part ahead of the check for length being zero (or no flushing to be
>>>>> performed).
>>>>
>>>> I am probably missing something here. I understand the theory behind
>>>> reducing the number of checks when len == 0. But an OS cannot rely on it:
>>>>      1) older hypervisor would still return an error if the check doesn't
>>>> pass)
>>>
>>> Right, but that's no reason to keep the bogus earlier behavior.
>>
>> Hmmm... I am not sure why you say the behavior is bogus. From the commit
>> message, it seems this is just an optimization that have side effect
>> (ignoring the other fields).
> 
> I don't view this as primarily an optimization; I'm in particular after
> not raising errors for cases where there is no error to be raised.
> Hence the comparison to memcpy(), which you can pass "bogus" pointers
> so long as you pass zero size.

The part I am missing is why this approach is better than what we have. 
So far what you described is just a matter of taste.

To give a concrete example, if tomorrow a contributor decides to send a 
patch undoing what you did (IOW enforcing the check for zero-length or 
replace | with two branches), then on what grounds I will be able to 
refuse their patch?

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[Jan Beulich]

On 21.02.2024 10:34, Julien Grall wrote:
> Hi,
> 
> On 20/02/2024 12:25, Jan Beulich wrote:
>> On 20.02.2024 12:52, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 20/02/2024 08:26, Jan Beulich wrote:
>>>> On 19.02.2024 23:22, Julien Grall wrote:
>>>>> Title: I would add 'gnttab:' to clarify which subsystem you are modifying.
>>>>
>>>> That's how I actually have it here; it's not clear to me why I lost the
>>>> prefix when sending.
>>>>
>>>>> On 05/02/2024 11:03, Jan Beulich wrote:
>>>>>> Along the line with observations in the context of XSA-448, besides
>>>>>> "op" no field is relevant when the range to be flushed is empty, much
>>>>>> like e.g. the pointers passed to memcpy() are irrelevant (and would
>>>>>> never be "validated") when the passed length is zero. Split the existing
>>>>>> condition validating "op", "offset", and "length", leaving only the "op"
>>>>>> part ahead of the check for length being zero (or no flushing to be
>>>>>> performed).
>>>>>
>>>>> I am probably missing something here. I understand the theory behind
>>>>> reducing the number of checks when len == 0. But an OS cannot rely on it:
>>>>>      1) older hypervisor would still return an error if the check doesn't
>>>>> pass)
>>>>
>>>> Right, but that's no reason to keep the bogus earlier behavior.
>>>
>>> Hmmm... I am not sure why you say the behavior is bogus. From the commit
>>> message, it seems this is just an optimization that have side effect
>>> (ignoring the other fields).
>>
>> I don't view this as primarily an optimization; I'm in particular after
>> not raising errors for cases where there is no error to be raised.
>> Hence the comparison to memcpy(), which you can pass "bogus" pointers
>> so long as you pass zero size.
> 
> The part I am missing is why this approach is better than what we have. 
> So far what you described is just a matter of taste.
> 
> To give a concrete example, if tomorrow a contributor decides to send a 
> patch undoing what you did (IOW enforcing the check for zero-length or 
> replace | with two branches), then on what grounds I will be able to 
> refuse their patch?

On the grounds of the argument I gave before: Consistency with other
more or less similar operations, where length 0 simply means "no-op",
up to and including "no errors from arguments specifying the address(es)
to operate on".

Jan