| Message ID | 9cf852da1c03b614bf5010132c58a18adc2a4161.1703155225.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,RFC] x86/uaccess: remove __{put,get}_user_bad() | expand |
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@bugseng.com> > --- > 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. Jan
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@bugseng.com> >> --- >> 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. ~Andrew
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@bugseng.com> >>> --- >>> 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. > > ~Andrew 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;
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@bugseng.com> >>>> --- >>>> 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. Jan
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@bugseng.com> >>>>> --- >>>>> 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?
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@bugseng.com> > > > > > > --- > > > > > > 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.
On 05.01.2024 17:19, 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@bugseng.com> >>>>>> --- >>>>>> 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' Nice idea. To take it one step further, why not simply use the .error assembler directive then? > 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? Amend the new macro's expansion by unreachable()? Jan
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@bugseng.com> >>>>>>> --- >>>>>>> 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
On 08/01/24 09:02, Jan Beulich wrote: > On 05.01.2024 17:19, 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@bugseng.com> >>>>>>> --- >>>>>>> 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' > > Nice idea. To take it one step further, why not simply use the .error > assembler directive then? It seems good. > >> 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? > > Amend the new macro's expansion by unreachable()? It would work only if we also add macro unreachable() to the allowed statements that can terminate a switch-clause. I'll take this opportunity to clarify the Rule 16.3 and the deviation system of ECLAIR for this rule (adding Julien in CC, he might be interested in this). The rationale of 16.3 is the avoidance of unintentional fall through. To do this, the rule says to put an unconditional break statement at the end of every switch-clause. Nothing is said about the semantics of the code within the switch-clause, e.g., the rule does not take into account if the fall through cannot happen because the code returns in every feasible path. The reason behind this is to keep the rule as simple as possible and above all, keep the rule to be decidable. Given the fact that 16.3 is a purely syntactic (and hence decidable) rule, the deviations that can be configured within ECLAIR are consequently purely syntactic. Currently, we configured the tool to allow also unconditional return, unconditional goto and unconditional continue as terminating statements. This means that, if you want also to deviate switch-clauses terminating with: if ( cond ) return x; else return y; then we need to explicitly configure the tool to consider also an if statement having this particular shape as allowed terminal statement (which is something I would not suggest since a rewriting would address the violation). The same applies to unreachable(). No semantics checks are performed for Rule 16.3, hence we will need to add it to the allowed terminal statements.
On 08.01.2024 12:16, Federico Serafini wrote: > On 08/01/24 09:02, Jan Beulich wrote: >> On 05.01.2024 17:19, 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@bugseng.com> >>>>>>>> --- >>>>>>>> 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' >> >> Nice idea. To take it one step further, why not simply use the .error >> assembler directive then? > > It seems good. > >> >>> 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? >> >> Amend the new macro's expansion by unreachable()? > > It would work only if we also add macro unreachable() to the allowed > statements that can terminate a switch-clause. Isn't this, or something substantially similar, necessary anyway, to avoid ... > I'll take this opportunity to clarify the Rule 16.3 and the deviation > system of ECLAIR for this rule (adding Julien in CC, he might be > interested in this). > The rationale of 16.3 is the avoidance of unintentional fall through. > To do this, the rule says to put an unconditional break statement at > the end of every switch-clause. > > Nothing is said about the semantics of the code within the > switch-clause, e.g., the rule does not take into account if the fall > through cannot happen because the code returns in every feasible path. > The reason behind this is to keep the rule as simple as possible and > above all, keep the rule to be decidable. such "break" then violating the "no unreachable code" rule? Jan > Given the fact that 16.3 is a purely syntactic (and hence decidable) > rule, the deviations that can be configured within ECLAIR are > consequently purely syntactic. > Currently, we configured the tool to allow also unconditional return, > unconditional goto and unconditional continue as terminating statements. > This means that, if you want also to deviate switch-clauses terminating > with: > > if ( cond ) return x; else return y; > > then we need to explicitly configure the tool to consider also > an if statement having this particular shape as allowed terminal > statement (which is something I would not suggest since a rewriting > would address the violation). > > The same applies to unreachable(). > No semantics checks are performed for Rule 16.3, > hence we will need to add it to the allowed terminal statements. >
On 08/01/24 12:36, Jan Beulich wrote: > On 08.01.2024 12:16, Federico Serafini wrote: >> On 08/01/24 09:02, Jan Beulich wrote: >>> On 05.01.2024 17:19, 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@bugseng.com> >>>>>>>>> --- >>>>>>>>> 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' >>> >>> Nice idea. To take it one step further, why not simply use the .error >>> assembler directive then? >> >> It seems good. >> >>> >>>> 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? >>> >>> Amend the new macro's expansion by unreachable()? >> >> It would work only if we also add macro unreachable() to the allowed >> statements that can terminate a switch-clause. > > Isn't this, or something substantially similar, necessary anyway, to > avoid ... > >> I'll take this opportunity to clarify the Rule 16.3 and the deviation >> system of ECLAIR for this rule (adding Julien in CC, he might be >> interested in this). >> The rationale of 16.3 is the avoidance of unintentional fall through. >> To do this, the rule says to put an unconditional break statement at >> the end of every switch-clause. >> >> Nothing is said about the semantics of the code within the >> switch-clause, e.g., the rule does not take into account if the fall >> through cannot happen because the code returns in every feasible path. >> The reason behind this is to keep the rule as simple as possible and >> above all, keep the rule to be decidable. > > such "break" then violating the "no unreachable code" rule? For such cases of Rule 2.1 ("A project shall not contain unreachable code.") there is already a deviation documented in deviations.rst: "The compiler implementation guarantees that the unreachable code is removed. Constant expressions and unreachable branches of if and switch statements are expected." So, following your suggestion we can consider unreachable() as allowed terminal for 16.3 and use it within the definition of static_assert_unreachable(). Additionally, looking at violations of 16.3 on X86 [1], I think we should also consider generate_exception(), ASSERT_UNREACHABLE() and PARSE_ERR_RET() as allowed terminals for a switch-clause, do you agree? [1] https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-2023/466/PROJECT.ecd;/by_service/MC3R1.R16.3.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":false,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"message","inputs":[{"enabled":true,"text":"^.*put_unsafe_size'"},{"enabled":true,"text":"^.*get_unsafe_size'"}]}}}
On 08.01.2024 15:01, Federico Serafini wrote: > Additionally, looking at violations of 16.3 on X86 [1], > I think we should also consider generate_exception(), > ASSERT_UNREACHABLE() and PARSE_ERR_RET() as allowed terminals > for a switch-clause, do you agree? No, and iirc this was discussed before. ASSERT_UNREACHABLE() is a debug-build-only construct, so unsuitable. The other two aren't global constructs, and hence shouldn't be deviated globally. generate_exception() at least ends in a goto anyway, so why would it need special treatment? Jan
On 08/01/24 15:44, Jan Beulich wrote: > On 08.01.2024 15:01, Federico Serafini wrote: >> Additionally, looking at violations of 16.3 on X86 [1], >> I think we should also consider generate_exception(), >> ASSERT_UNREACHABLE() and PARSE_ERR_RET() as allowed terminals >> for a switch-clause, do you agree? > > No, and iirc this was discussed before. ASSERT_UNREACHABLE() is a > debug-build-only construct, so unsuitable. The other two aren't > global constructs, and hence shouldn't be deviated globally. > generate_exception() at least ends in a goto anyway, so why would > it need special treatment? This is related to what I said before regarding the fact that Rule 16.3 is a purely syntactic rule. The goto is within an if statement; the tool is able to considered the fact that the condition is always true but an explicit configuration is needed. Sorry for the late reply.
diff --git a/xen/arch/x86/include/asm/uaccess.h b/xen/arch/x86/include/asm/uaccess.h index 7443519d5b..15b747d0c7 100644 --- a/xen/arch/x86/include/asm/uaccess.h +++ b/xen/arch/x86/include/asm/uaccess.h @@ -21,9 +21,6 @@ unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int unsigned int copy_to_unsafe_ll(void *to, const void *from, unsigned int n); unsigned int copy_from_unsafe_ll(void *to, const void *from, unsigned int n); -extern long __get_user_bad(void); -extern void __put_user_bad(void); - #define UA_KEEP(args...) args #define UA_DROP(args...) @@ -208,7 +205,7 @@ do { \ case 8: \ put_unsafe_asm(x, ptr, grd, retval, "q", "", "ir", errret); \ break; \ - default: __put_user_bad(); \ + default: break; \ } \ 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: break; \ } \ clac(); \ } while ( false )
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@bugseng.com> --- 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. --- xen/arch/x86/include/asm/uaccess.h | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)