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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan