[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Federico Serafini <federico.serafini@xxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 7 Feb 2024 17:19:21 +0100
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: consulting@xxxxxxxxxxx, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • Delivery-date: Wed, 07 Feb 2024 16:19:32 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.