Re: [XEN RFC] x86/uaccess: remove __{put,get}_user_bad()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.01.2024 23:48, Stefano Stabellini wrote:
> On Fri, 5 Jan 2024, Federico Serafini wrote:
>> Hello everyone,
>>
>> On 21/12/23 13:41, Jan Beulich wrote:
>>> On 21.12.2023 13:01, Nicola Vetrini wrote:
>>>> Hi Andrew,
>>>>
>>>> On 2023-12-21 12:03, Andrew Cooper wrote:
>>>>> On 21/12/2023 10:58 am, Jan Beulich wrote:
>>>>>> On 21.12.2023 11:53, Federico Serafini wrote:
>>>>>>> Remove declarations of __put_user_bad() and __get_user_bad()
>>>>>>> since they have no definition.
>>>>>>> Replace their uses with a break statement to address violations of
>>>>>>> MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall
>>>>>>> terminate every switch-clause").
>>>>>>> No functional change.
>>>>>>>
>>>>>>> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
>>>>>>> ---
>>>>>>> Several violations of Rule 16.3 come from uses of macros
>>>>>>> get_unsafe_size() and put_unsafe_size().
>>>>>>> Looking at the macro definitions I found __get_user_bad() and
>>>>>>> __put_user_bad().
>>>>>>> I was wondering if instead of just adding the break statement I can
>>>>>>> also remove
>>>>>>> such functions which seem to not have a definition.
>>>>>> No, you can't. Try introducing a caller which "accidentally" uses the
>>>>>> wrong size. Without your change you'll observe the build failing (in
>>>>>> a somewhat obscure way, but still), while with your change bad code
>>>>>> will silently be generated.
>>>>>
>>>>> The construct here is deliberate.  It's a build time assertion that bad
>>>>> sizes aren't used.
>>>>>
>>>>> __bitop_bad_size() and __xsm_action_mismatch_detected() are the same
>>>>> pattern in other areas of code too, with the latter being more explicit
>>>>> because of how it's wrapped by LINKER_BUG_ON().
>>>>>
>>>>>
>>>>> It is slightly horrible, and not the most obvious construct for
>>>>> newcomers.  If there's an alternative way to get a build assertion, we
>>>>> could consider switching to a new pattern.
>>>>
>>>> would you be in favour of a solution with a BUILD_BUG_ON in the default
>>>> branch followed by a break?
>>>>
>>>> default:
>>>>       BUILD_BUG_ON(!size || size >=8 || (size & (size - 1)));
>>>>       break;
>>>
>>> I don't think this would compile - BUILD_BUG_ON() wants a compile-time
>>> constant passed.
>>
>> What do you think about adding the following macro to compiler.h:
>>
>> #define static_assert_unreachable(identifier) \
>>     asm("unreachable " #identifier " reached")
>>
>> It expands to an invalid assembly instruction that will lead to a
>> customizable error message generated by the assembler instead of the
>> linker (anticipating the error detection).
>>
>> The use of this macro will indicate a program point considered
>> unreachable (and as such removed) by the static analysis performed by the
>> compiler, even at an optimization level -O0.
>>
>> An example of use is in the default case of put_unsafe_size():
>>
>> default: static_assert_unreachable(default);
>>
>> In case a wrong size will be used, the following message will be
>> generated:
>>
>> ./arch/x86/include/asm/uaccess.h: Assembler messages:
>> ./arch/x86/include/asm/uaccess.h:257: Error: no such instruction: 
>> `unreachable
>> default reached'
>>
>>
>> Note that adopting the macro and discussing its definition are two
>> separate things:
>> I think we can all agree on the fact that the use of such macro improves
>> readability, so I would suggest its adoption.
>> Whereas for its definition, if you don't like the invalid asm
>> instruction, we could discuss for a different solution, for example,
>> the following is something similar to what you are doing now:
>>
>> #define static_assert_unreachable(identifier) \
>>     extern void identifier(void);             \
>>     identifier()
>>
>>
>> Note also that the problem of the missing break statement (that violates
>> Rule 16.3) is still present, it could be addressed by adding the break
>> or deviating for such special cases, do you have any preferences?
> 
> So overall for clarity you are suggesting:
> 
> 
> diff --git a/xen/arch/x86/include/asm/uaccess.h 
> b/xen/arch/x86/include/asm/uaccess.h
> index 7443519d5b..7e7ef77e49 100644
> --- a/xen/arch/x86/include/asm/uaccess.h
> +++ b/xen/arch/x86/include/asm/uaccess.h
> @@ -208,7 +205,9 @@ do {                                                      
>                  \
>      case 8:                                                                \
>          put_unsafe_asm(x, ptr, grd, retval, "q",  "", "ir", errret);       \
>          break;                                                             \
> -    default: __put_user_bad();                                             \
> +    default:                                                               \
> +        static_assert_unreachable(default);                                \
> +        break;                                                             \
>      }                                                                      \
>      clac();                                                                \
>  } while ( false )

While this is an improvement over __put_user_bad(), as we're re-working
this I think it would be helpful to make the resulting diagnostic point
people in the right direction: The way it is above, two different switch()
statements would both yield the same diagnostic, leaving one to guess. So
at least the function name and/or source file/line would likely better be
part of the diagnostic.

Jan