Re: [PATCH 9/9] gnttab: don't silently truncate GFNs in compat setup-table handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.10.2022 21:57, Andrew Cooper wrote:
> On 26/08/2021 11:15, Jan Beulich wrote:
>> --- a/xen/common/compat/grant_table.c
>> +++ b/xen/common/compat/grant_table.c
>> @@ -175,8 +175,15 @@ int compat_grant_table_op(unsigned int c
>>                                   i < (_s_)->nr_frames; ++i ) \
>>                      { \
>>                          compat_pfn_t frame = (_s_)->frame_list.p[i]; \
>> -                        if ( __copy_to_compat_offset((_d_)->frame_list, \
>> -                                                     i, &frame, 1) ) \
>> +                        if ( frame != (_s_)->frame_list.p[i] ) \
>> +                        { \
>> +                            if ( VALID_M2P((_s_)->frame_list.p[i]) ) \
>> +                                (_s_)->status = GNTST_address_too_big; \
>> +                            else \
>> +                                frame |= 0x80000000U;\
> 
> Space before the \.  (This is one reason why I hate this style.  The
> borderline illegibility makes it almost impossible to spot style problems.)

There is a (imo severe) downsides to backslashes on the far right as well:
It's easier to miss adding one on a newly added line, which may or may not
result in a build failure.

> With the adjustment from the previous patch, you'll need a break in here.

Can do. Question then is whether to go further right here and adjust
the loop header and the other setting of (_s_)->status at the same time.

> But for !valid case, shouldn't we saturate to ~0u ?  I recall from the
> migration work that various kernels disagree on what constitutes an
> invalid MFN.
> 
> Then again, I can't see what legitimate case we'd have for a truncation
> and an invalid M2P entry needing translating.

I've dropped the use of VALID_M2P(). It's a bogus check anyway (I don't
actually recall how I came to think of doing this sort of check), and
there's indeed no reason to report back an (overflow) error in this way.
Furthermore I've noticed that the updating of "frame" was actually dead
code - the updated variable wasn't really used for anything; we would
have left both ->status and the array slot untouched, misguiding the
caller.

Jan