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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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 )


I prefer static_assert_unreachable(default) over __put_user_bad()
because it is even clearer about its intent and still generates a
build-time error.

Regarding the addition of the break, I think that's OK for me. But I am
guessing that Jan will prefer to add static_assert_unreachable to
docs/misra/deviations.rst like we did for BUG() so that we don't need to
add the break.