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