Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2024 10:50, Federico Serafini wrote:
> On 08/02/24 12:14, Jan Beulich wrote:
>> On 08.02.2024 11:45, Federico Serafini wrote:
>>> On 07/02/24 17:19, Jan Beulich wrote:
>>>> On 07.02.2024 16:58, Federico Serafini wrote:
>>>>> On 07/02/24 16:24, Jan Beulich wrote:
>>>>>> On 07.02.2024 16:08, Federico Serafini wrote:
>>>>>>> On 07/02/24 15:16, Jan Beulich wrote:
>>>>>>>> On 07.02.2024 14:51, Federico Serafini wrote:
>>>>>>>>> On 07/02/24 08:38, Jan Beulich wrote:
>>>>>>>>>> On 07.02.2024 02:08, Stefano Stabellini wrote:
>>>>>>>>>>> On Tue, 6 Feb 2024, Jan Beulich wrote:
>>>>>>>>>>>> On 26.01.2024 11:05, Federico Serafini wrote:
>>>>>>>>>>>>> @@ -208,7 +205,7 @@ do {                                          
>>>>>>>>>>>>>                              \
>>>>>>>>>>>>>           case 8:                                                 
>>>>>>>>>>>>>                \
>>>>>>>>>>>>>               put_unsafe_asm(x, ptr, grd, retval, "q",  "", "ir", 
>>>>>>>>>>>>> errret);       \
>>>>>>>>>>>>>               break;                                              
>>>>>>>>>>>>>                \
>>>>>>>>>>>>> -    default: __put_user_bad();                                   
>>>>>>>>>>>>>           \
>>>>>>>>>>>>> +    default: STATIC_ASSERT_UNREACHABLE();                        
>>>>>>>>>>>>>           \
>>>>>>>>>>>>>           }                                                       
>>>>>>>>>>>>>                \
>>>>>>>>>>>>>           clac();                                                 
>>>>>>>>>>>>>                \
>>>>>>>>>>>>>       } while ( false )
>>>>>>>>>>>>> @@ -227,7 +224,7 @@ do {                                          
>>>>>>>>>>>>>                              \
>>>>>>>>>>>>>           case 2: get_unsafe_asm(x, ptr, grd, retval, "w", "=r", 
>>>>>>>>>>>>> errret); break; \
>>>>>>>>>>>>>           case 4: get_unsafe_asm(x, ptr, grd, retval, "k", "=r", 
>>>>>>>>>>>>> errret); break; \
>>>>>>>>>>>>>           case 8: get_unsafe_asm(x, ptr, grd, retval,  "", "=r", 
>>>>>>>>>>>>> errret); break; \
>>>>>>>>>>>>> -    default: __get_user_bad();                                   
>>>>>>>>>>>>>           \
>>>>>>>>>>>>> +    default: STATIC_ASSERT_UNREACHABLE();                        
>>>>>>>>>>>>>           \
>>>>>>>>>>>>>           }                                                       
>>>>>>>>>>>>>                \
>>>>>>>>>>>>>           clac();                                                 
>>>>>>>>>>>>>                \
>>>>>>>>>>>>>       } while ( false )
>>>>>>>>>>>>
>>>>>>>>>>>> Related to my remark on patch 1 - how is one to know the macro 
>>>>>>>>>>>> this was
>>>>>>>>>>>> invoked from, when seeing the resulting diagnostic?
>>>>>>>>>>>
>>>>>>>>>>> I am not sure what do you mean here... we do get an error like the
>>>>>>>>>>> following (I added a STATIC_ASSERT_UNREACHABLE for case 4):
>>>>>>>>>>>
>>>>>>>>>>> ./arch/x86/include/asm/uaccess.h:262: Error: static assertion 
>>>>>>>>>>> failed: unreachable
>>>>>>>>>>
>>>>>>>>>> Right - and how do I know what _user_ of the macro actually triggered
>>>>>>>>>> it? ISTR suggesting to use one or more of __FILE__ / __LINE__ /
>>>>>>>>>> __FUNCTION__ here, for that specific purpose ...
>>>>>>>>>
>>>>>>>>> To test the macro and its diagnostics,
>>>>>>>>> I modified the first "git grep" occurrence of ASSERT_UNREACHABLE()
>>>>>>>>> on the x86 code with STATIC_ASSERT_UNREACHABLE(),
>>>>>>>>> that is in file arch/x86/alternative.c, line 312,
>>>>>>>>> function _apply_alternatives().
>>>>>>>>>
>>>>>>>>> What I got is the following build error:
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>> arch/x86/alternative.c: Assembler messages:
>>>>>>>>> arch/x86/alternative.c:312: Error: static assertion failed: 
>>>>>>>>> unreachable
>>>>>>>>>        CC      arch/x86/copy_page.o
>>>>>>>>> make[2]: *** [Rules.mk:247: arch/x86/alternative.o] Error 1
>>>>>>>>
>>>>>>>> But that's not what my request was about. Here sufficient context is
>>>>>>>> given, even if it would be nice if the function was also visible right
>>>>>>>> away. But that's not the same as the case above, where the new macro
>>>>>>>> is used inside another macro.
>>>>>>>
>>>>>>> An example of that is the get_unsafe_size() macro,
>>>>>>> whose body uses STATIC_ASSERT_UNREACHABLE().
>>>>>>> A wrong use of get_unsafe_size() at line n
>>>>>>> leads to a build error pointing to the line n,
>>>>>>> isn't this the desired behavior?
>>>>>>
>>>>>> Aiui this would point to the line in the header file, when what you need
>>>>>> to spot the bad use of the macro is the line in the source file actually
>>>>>> using the macro. Quoting from an earlier mail of yours:
>>>>>>
>>>>>> ./arch/x86/include/asm/uaccess.h:262: Error: static assertion failed: 
>>>>>> unreachable
>>>>>
>>>>> It points to the header file uaccess.h because at line 262 there is
>>>>> an intentional wrong use of put_guest_size(), within the body of
>>>>> __copy_to_guest_pv() function.
>>>>
>>>> Yet that's again only a helper function being inlined into the ultimate
>>>> caller. That ultimate caller is what wants identifying in the diag. Not
>>>> the least because of ...
>>>>
>>>>> This example can be misleading because {get,put}_unsafe_size() are
>>>>> defined in the same file but the diagnostics is doing the
>>>>> right thing.
>>>>
>>>> ... this. And really __copy_to_guest_pv() is the wrong place to put a
>>>> wrong put_guest_size() in, to try out how diagnostics would look like
>>>> in reality: That function falls back to copy_to_guest_ll() for all
>>>> cases it can't handle directly. You want to place a bogus put_guest()
>>>> somewhere in a .c file to see what results.
>>>
>>> I added a bogus call to put_guest() at line 387 of
>>> file xen/arch/x86/mm.c, inside function page_is_ram_type().
>>> Assuming I did not choose another wrong place,
>>> the diagnostic seems appropriate:
>>>
>>> arch/x86/mm.c: Assembler messages:
>>> arch/x86/mm.c:387: Error: static assertion failed: unreachable
>>
>> Oh, okay, this looks appropriate then as to identifying where the
>> source construct is. However, we then still don't know where the
>> assertion in question is (there could be multiple in what the
>> original construct expands to). So I'm still inclined to ask that
>> __FILE__ / __LINE__ and/or the name of the invoking construct
>> (macro or function) be made visible in the diagnostic.
> 
> Any use of __FILE__ and __LINE__ results in obtaining
> the same information already reported by the assembler error message.

Hmm, yes, since put_guest() is itself a macro.

> We could add an argument to the new macro to manually add some context
> at every use of the macro, but I think this would be annoying.

That's a last resort. An alternative would be to see about converting
from macros to inline functions, where this would make a difference
here.

Jan