| Message ID | 20230905073135.12360-3-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: fixes for UBSAN findings with SMMUv3 enabled | expand |
On 05.09.2023 09:31, Michal Orzel wrote: > When running with SMMUv3 and UBSAN enabled on arm64, there are a lot of > warnings printed related to shifting into sign bit in generic_fls() > as it takes parameter of type int. > > Example: > (XEN) UBSAN: Undefined behaviour in ./include/xen/bitops.h:69:11 > (XEN) left shift of 134217728 by 4 places cannot be represented in type 'int' > > It does not make a lot of sense to ask for the last set bit of a negative > value. We don't have a direct user of this helper and all the wrappers > pass value of type unsigned {int,long}. > > Linux did the same as part of commit: > 3fc2579e6f16 ("fls: change parameter to unsigned int") > > To keep consistency between the helpers, take the opportunity to: > - replace __inline__ with inline, > - modify generic_ffs() to take parameter of type unsigned int as well > (currently no user and the only wrapper generic_ffsl() passes unsigned > long). > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Hi Michal, > On Sep 5, 2023, at 15:31, Michal Orzel <michal.orzel@amd.com> wrote: > > When running with SMMUv3 and UBSAN enabled on arm64, there are a lot of > warnings printed related to shifting into sign bit in generic_fls() > as it takes parameter of type int. > > Example: > (XEN) UBSAN: Undefined behaviour in ./include/xen/bitops.h:69:11 > (XEN) left shift of 134217728 by 4 places cannot be represented in type 'int' > > It does not make a lot of sense to ask for the last set bit of a negative > value. We don't have a direct user of this helper and all the wrappers > pass value of type unsigned {int,long}. > > Linux did the same as part of commit: > 3fc2579e6f16 ("fls: change parameter to unsigned int") > > To keep consistency between the helpers, take the opportunity to: > - replace __inline__ with inline, > - modify generic_ffs() to take parameter of type unsigned int as well > (currently no user and the only wrapper generic_ffsl() passes unsigned > long). > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Reviewed-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h index 654f525fb437..edd6817d5356 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -18,7 +18,7 @@ * differs in spirit from the above ffz (man ffs). */ -static inline int generic_ffs(int x) +static inline int generic_ffs(unsigned int x) { int r = 1; @@ -51,7 +51,7 @@ static inline int generic_ffs(int x) * fls: find last bit set. */ -static __inline__ int generic_fls(int x) +static inline int generic_fls(unsigned int x) { int r = 32;
When running with SMMUv3 and UBSAN enabled on arm64, there are a lot of warnings printed related to shifting into sign bit in generic_fls() as it takes parameter of type int. Example: (XEN) UBSAN: Undefined behaviour in ./include/xen/bitops.h:69:11 (XEN) left shift of 134217728 by 4 places cannot be represented in type 'int' It does not make a lot of sense to ask for the last set bit of a negative value. We don't have a direct user of this helper and all the wrappers pass value of type unsigned {int,long}. Linux did the same as part of commit: 3fc2579e6f16 ("fls: change parameter to unsigned int") To keep consistency between the helpers, take the opportunity to: - replace __inline__ with inline, - modify generic_ffs() to take parameter of type unsigned int as well (currently no user and the only wrapper generic_ffsl() passes unsigned long). Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- Changes in v2: - as requested by Jan, s/__inline__/inline/ and modify generic_ffs() for consistency It looks like generic_fls() is only used by Arm and invoked only if the arguement passed is a compile time constant. This is true for SMMUv3 which makes use of ffs64() in FIELD_{PREP,GET} macros. generic_ffs() has no user at the moment. Note for the future: The return type could be unsigned as well. However, looking at all variations of the helpers to find first set/clear in the codebase, returning int is the de-facto standard. Changing the return type of the title helpers would result in inconsistency or require to do modifications in other places which is not something I want to do at this stage of release. --- xen/include/xen/bitops.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)