| Message ID | 20230904091406.942-3-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: fixes for UBSAN findings with SMMUv3 enabled | expand |
On 04.09.2023 11:14, Michal Orzel wrote: > --- a/xen/include/xen/bitops.h > +++ b/xen/include/xen/bitops.h > @@ -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; > Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care of as well, imo. If additionally you switch __inline__ to inline, things will become nicely symmetric with generic_f{f,l}sl(). Another aspect that may be nice to take care of at this occasion is their return values: None of them can return negative values, so return type would better be unsigned int. Jan
On 04/09/2023 15:28, Jan Beulich wrote: > > > On 04.09.2023 11:14, Michal Orzel wrote: >> --- a/xen/include/xen/bitops.h >> +++ b/xen/include/xen/bitops.h >> @@ -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; >> > > Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care > of as well, imo. If additionally you switch __inline__ to inline, things > will become nicely symmetric with generic_f{f,l}sl(). If others agree, I can switch generic_ffs() to take unsigned as well (together with s/__inline__/inline/). However, such change would no longer fall in fixes category (i.e. nothing wrong with ffs() taking int): - is it ok at this stage of release? (not sure but no problem for me to do this) - is it ok to mix a fix with non-fix change? (I always prefer fixes to be done as standalone changes) > > Another aspect that may be nice to take care of at this occasion is their > return values: None of them can return negative values, so return type > would better be unsigned int. Looking at all the variations (including arch asm specific) of helpers to find first set/clear in the codebase, returning int is the de-facto standard. So, this would result in some inconsistency or require changing at least the direct callers to also return unsigned. I'd prefer not to be required to add this extra churn at this release stage. ~Michal
On 04.09.2023 16:03, Michal Orzel wrote: > On 04/09/2023 15:28, Jan Beulich wrote: >> On 04.09.2023 11:14, Michal Orzel wrote: >>> --- a/xen/include/xen/bitops.h >>> +++ b/xen/include/xen/bitops.h >>> @@ -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; >>> >> >> Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care >> of as well, imo. If additionally you switch __inline__ to inline, things >> will become nicely symmetric with generic_f{f,l}sl(). > If others agree, I can switch generic_ffs() to take unsigned as well (together with s/__inline__/inline/). > However, such change would no longer fall in fixes category (i.e. nothing wrong with ffs() taking int): > - is it ok at this stage of release? (not sure but no problem for me to do this) I'd say yes, but the release manager would have the ultimate say. > - is it ok to mix a fix with non-fix change? (I always prefer fixes to be done as standalone changes) Imo it is, to avoid introducing an inconsistency. While such may not themselves be bugs, they increase the risk of introducing some. Jan
Hi Jan and Michal, > On Sep 4, 2023, at 23:13, Jan Beulich <jbeulich@suse.com> wrote: > > On 04.09.2023 16:03, Michal Orzel wrote: >> On 04/09/2023 15:28, Jan Beulich wrote: >>> On 04.09.2023 11:14, Michal Orzel wrote: >>>> --- a/xen/include/xen/bitops.h >>>> +++ b/xen/include/xen/bitops.h >>>> @@ -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; >>>> >>> >>> Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care >>> of as well, imo. If additionally you switch __inline__ to inline, things >>> will become nicely symmetric with generic_f{f,l}sl(). >> If others agree, I can switch generic_ffs() to take unsigned as well (together with s/__inline__/inline/). >> However, such change would no longer fall in fixes category (i.e. nothing wrong with ffs() taking int): >> - is it ok at this stage of release? (not sure but no problem for me to do this) > > I'd say yes, but the release manager would have the ultimate say. From the release aspect, I am fine with doing the above-mentioned extra changes, as we are not in the code freeze stage. From the development point of view, I think whether doing such extra changes or not is supposed to be an agreement between the developer and the maintainer. Also this patch itself looks good to me so: Reviewed-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry > >> - is it ok to mix a fix with non-fix change? (I always prefer fixes to be done as standalone changes) > > Imo it is, to avoid introducing an inconsistency. While such may > not themselves be bugs, they increase the risk of introducing some. > > Jan >
diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h index 654f525fb437..b2d7bbd66687 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -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") Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- 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. --- xen/include/xen/bitops.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)