| Message ID | 20220223164420.45344-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] KVM: s390: Don't cast parameter in bit operations | expand |
On Wed, 23 Feb 2022 18:44:20 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > While in this particular case it would not be a (critical) issue, > the pattern itself is bad and error prone in case somebody blindly > copies to their code. > > Don't cast parameter to unsigned long pointer in the bit operations. > Instead copy to a local variable on stack of a proper type and use. > > Fixes: d77e64141e32 ("KVM: s390: implement GISA IPM related primitives") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > arch/s390/include/asm/kvm_host.h | 5 ++++- > arch/s390/kvm/interrupt.c | 6 +++--- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index a22c9266ea05..f1c4a1b9b360 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -867,7 +867,10 @@ struct kvm_s390_gisa { > u8 reserved03[11]; > u32 airq_count; > } g1; > - struct { > + struct { /* as a 256-bit bitmap */ > + DECLARE_BITMAP(b, 256); > + } bitmap; > + struct { /* as a set of 64-bit words */ > u64 word[4]; > } u64; > }; > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c > index db933c252dbc..04e055cbd080 100644 > --- a/arch/s390/kvm/interrupt.c > +++ b/arch/s390/kvm/interrupt.c > @@ -304,7 +304,7 @@ static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa) > > static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) > { > - set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); > + set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); wouldn't it be enough to pass gisa->u64.word here? then no cast would be necessary > } > > static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa) > @@ -314,12 +314,12 @@ static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa) > > static inline void gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) > { > - clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); > + clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); > } > > static inline int gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) > { > - return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); > + return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); > } > > static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
On 24.02.22 12:36, Claudio Imbrenda wrote: > On Wed, 23 Feb 2022 18:44:20 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> While in this particular case it would not be a (critical) issue, >> the pattern itself is bad and error prone in case somebody blindly >> copies to their code. >> >> Don't cast parameter to unsigned long pointer in the bit operations. >> Instead copy to a local variable on stack of a proper type and use. >> >> Fixes: d77e64141e32 ("KVM: s390: implement GISA IPM related primitives") >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> arch/s390/include/asm/kvm_host.h | 5 ++++- >> arch/s390/kvm/interrupt.c | 6 +++--- >> 2 files changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index a22c9266ea05..f1c4a1b9b360 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -867,7 +867,10 @@ struct kvm_s390_gisa { >> u8 reserved03[11]; >> u32 airq_count; >> } g1; >> - struct { >> + struct { /* as a 256-bit bitmap */ >> + DECLARE_BITMAP(b, 256); >> + } bitmap; >> + struct { /* as a set of 64-bit words */ >> u64 word[4]; >> } u64; >> }; >> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c >> index db933c252dbc..04e055cbd080 100644 >> --- a/arch/s390/kvm/interrupt.c >> +++ b/arch/s390/kvm/interrupt.c >> @@ -304,7 +304,7 @@ static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa) >> >> static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) >> { >> - set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); >> + set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); > > wouldn't it be enough to pass gisa->u64.word here? > then no cast would be necessary we do that at several places arch/s390/kernel/processor.c: for_each_set_bit_inv(bit, (long *)&stfle_fac_list, MAX_FACILITY_BIT) arch/s390/kvm/interrupt.c: set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) sca->mcn); arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) &sca->mcn); > >> } >> >> static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa) >> @@ -314,12 +314,12 @@ static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa) >> >> static inline void gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) >> { >> - clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); >> + clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); >> } >> >> static inline int gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) >> { >> - return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); >> + return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); >> } >> >> static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu) >
On Thu, Feb 24, 2022 at 2:51 PM Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > > On Wed, 23 Feb 2022 18:44:20 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > While in this particular case it would not be a (critical) issue, > > the pattern itself is bad and error prone in case somebody blindly > > copies to their code. > > > > Don't cast parameter to unsigned long pointer in the bit operations. > > Instead copy to a local variable on stack of a proper type and use. ... > > + struct { /* as a 256-bit bitmap */ > > + DECLARE_BITMAP(b, 256); > > + } bitmap; > > + struct { /* as a set of 64-bit words */ > > u64 word[4]; > > } u64; > > - set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); > > + set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); > > wouldn't it be enough to pass gisa->u64.word here? > then no cast would be necessary No, it will have the same hidden bugs. As I stated in the commit message, the pattern is quite bad even if in particular code it would work. Thanks, Michael, for pointing out other places. They all need to be fixed.
From: Andy Shevchenko > Sent: 24 February 2022 19:51 > > On Thu, Feb 24, 2022 at 2:51 PM Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > > > > On Wed, 23 Feb 2022 18:44:20 +0200 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > While in this particular case it would not be a (critical) issue, > > > the pattern itself is bad and error prone in case somebody blindly > > > copies to their code. > > > > > > Don't cast parameter to unsigned long pointer in the bit operations. > > > Instead copy to a local variable on stack of a proper type and use. > > ... > > > > + struct { /* as a 256-bit bitmap */ > > > + DECLARE_BITMAP(b, 256); > > > + } bitmap; > > > + struct { /* as a set of 64-bit words */ > > > u64 word[4]; > > > } u64; > > > > - set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); > > > + set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); > > > > wouldn't it be enough to pass gisa->u64.word here? > > then no cast would be necessary > > No, it will have the same hidden bugs. As I stated in the commit > message, the pattern is quite bad even if in particular code it would > work. > > Thanks, Michael, for pointing out other places. They all need to be fixed. It may even be worth writing some alternate bitmap functions that use u64[] and unlocked operations? Although I think I'd still want to encapsulate the actual array (somehow) so that what is defined has to be the bitmap type. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote: > On 24.02.22 12:36, Claudio Imbrenda wrote: ... > we do that at several places Thanks for pointing out. > arch/s390/kernel/processor.c: for_each_set_bit_inv(bit, (long > *)&stfle_fac_list, MAX_FACILITY_BIT) This one requires a separate change, not related to this patch. > arch/s390/kvm/interrupt.c: set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long > *) gisa); This is done in the patch. Not sure how it appears in your list. > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > sca->mcn); > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > &sca->mcn); These two should be fixed in a separate change. Also this kind of stuff: bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat, KVM_S390_VM_CPU_FEAT_NR_BITS); might require a new API like bitmap_from_u64_array() bitmap_to_u64_array() Yury?
On Wed, Feb 23, 2022 at 06:44:20PM +0200, Andy Shevchenko wrote: > While in this particular case it would not be a (critical) issue, > the pattern itself is bad and error prone in case somebody blindly > copies to their code. > > Don't cast parameter to unsigned long pointer in the bit operations. > Instead copy to a local variable on stack of a proper type and use. After looking into other similar cases I may conclude they - need to be fixed - out of scope of this change Hence, can this fix be applied?
On Wed, Mar 02, 2022 at 05:44:03PM +0200, Andy Shevchenko wrote: > On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote: > > On 24.02.22 12:36, Claudio Imbrenda wrote: > > ... > > > we do that at several places > > Thanks for pointing out. > > > arch/s390/kernel/processor.c: for_each_set_bit_inv(bit, (long > > *)&stfle_fac_list, MAX_FACILITY_BIT) > > This one requires a separate change, not related to this patch. > > > arch/s390/kvm/interrupt.c: set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long > > *) gisa); > > This is done in the patch. Not sure how it appears in your list. > > > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > > sca->mcn); > > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > > &sca->mcn); > > These two should be fixed in a separate change. > > Also this kind of stuff: > > bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat, > KVM_S390_VM_CPU_FEAT_NR_BITS); > > might require a new API like > > bitmap_from_u64_array() > bitmap_to_u64_array() > > Yury? If BE32 is still the case then yes.
On Wed, Mar 02, 2022 at 09:18:35AM -0800, Yury Norov wrote: > On Wed, Mar 02, 2022 at 05:44:03PM +0200, Andy Shevchenko wrote: > > On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote: > > > On 24.02.22 12:36, Claudio Imbrenda wrote: > > > > ... > > > > > we do that at several places > > > > Thanks for pointing out. > > > > > arch/s390/kernel/processor.c: for_each_set_bit_inv(bit, (long > > > *)&stfle_fac_list, MAX_FACILITY_BIT) > > > > This one requires a separate change, not related to this patch. > > > > > arch/s390/kvm/interrupt.c: set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long > > > *) gisa); > > > > This is done in the patch. Not sure how it appears in your list. > > > > > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > > > sca->mcn); > > > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > > > &sca->mcn); > > > > These two should be fixed in a separate change. > > > > Also this kind of stuff: > > > > bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat, > > KVM_S390_VM_CPU_FEAT_NR_BITS); > > > > might require a new API like > > > > bitmap_from_u64_array() > > bitmap_to_u64_array() > > > > Yury? > > If BE32 is still the case then yes. The whole point is to get rid of the bad pattern, while it may still work in the particular case.
On Wed, Mar 02, 2022 at 07:31:58PM +0200, Andy Shevchenko wrote: > On Wed, Mar 02, 2022 at 09:18:35AM -0800, Yury Norov wrote: > > On Wed, Mar 02, 2022 at 05:44:03PM +0200, Andy Shevchenko wrote: > > > On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote: > > > > On 24.02.22 12:36, Claudio Imbrenda wrote: > > > > > > ... > > > > > > > we do that at several places > > > > > > Thanks for pointing out. > > > > > > > arch/s390/kernel/processor.c: for_each_set_bit_inv(bit, (long > > > > *)&stfle_fac_list, MAX_FACILITY_BIT) > > > > > > This one requires a separate change, not related to this patch. > > > > > > > arch/s390/kvm/interrupt.c: set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long > > > > *) gisa); > > > > > > This is done in the patch. Not sure how it appears in your list. > > > > > > > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > > > > sca->mcn); > > > > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > > > > &sca->mcn); > > > > > > These two should be fixed in a separate change. > > > > > > Also this kind of stuff: > > > > > > bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat, > > > KVM_S390_VM_CPU_FEAT_NR_BITS); > > > > > > might require a new API like > > > > > > bitmap_from_u64_array() > > > bitmap_to_u64_array() > > > > > > Yury? > > > > If BE32 is still the case then yes. > > The whole point is to get rid of the bad pattern, while it may still work > in the particular case. Then yes unconditionally. Is it already on table of s390 folks? If no, I can do it myself. We have bitmap_from_arr32 and bitmap_to_arr32, so for 64-bit versions, we'd start from that.
On Wed, Mar 02, 2022 at 10:43:54AM -0800, Yury Norov wrote: > On Wed, Mar 02, 2022 at 07:31:58PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 02, 2022 at 09:18:35AM -0800, Yury Norov wrote: > > > On Wed, Mar 02, 2022 at 05:44:03PM +0200, Andy Shevchenko wrote: > > > > On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote: > > > > > On 24.02.22 12:36, Claudio Imbrenda wrote: > > > > > > > > ... > > > > > > > > > we do that at several places > > > > > > > > Thanks for pointing out. > > > > > > > > > arch/s390/kernel/processor.c: for_each_set_bit_inv(bit, (long > > > > > *)&stfle_fac_list, MAX_FACILITY_BIT) > > > > > > > > This one requires a separate change, not related to this patch. > > > > > > > > > arch/s390/kvm/interrupt.c: set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long > > > > > *) gisa); > > > > > > > > This is done in the patch. Not sure how it appears in your list. > > > > > > > > > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > > > > > sca->mcn); > > > > > arch/s390/kvm/kvm-s390.c: set_bit_inv(vcpu->vcpu_id, (unsigned long *) > > > > > &sca->mcn); > > > > > > > > These two should be fixed in a separate change. > > > > > > > > Also this kind of stuff: > > > > > > > > bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat, > > > > KVM_S390_VM_CPU_FEAT_NR_BITS); > > > > > > > > might require a new API like > > > > > > > > bitmap_from_u64_array() > > > > bitmap_to_u64_array() > > > > > > > > Yury? > > > > > > If BE32 is still the case then yes. > > > > The whole point is to get rid of the bad pattern, while it may still work > > in the particular case. > > Then yes unconditionally. Is it already on table of s390 folks? If no, > I can do it myself. > > We have bitmap_from_arr32 and bitmap_to_arr32, so for 64-bit versions, > we'd start from that. Yep, thanks!
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index a22c9266ea05..f1c4a1b9b360 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -867,7 +867,10 @@ struct kvm_s390_gisa { u8 reserved03[11]; u32 airq_count; } g1; - struct { + struct { /* as a 256-bit bitmap */ + DECLARE_BITMAP(b, 256); + } bitmap; + struct { /* as a set of 64-bit words */ u64 word[4]; } u64; }; diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c index db933c252dbc..04e055cbd080 100644 --- a/arch/s390/kvm/interrupt.c +++ b/arch/s390/kvm/interrupt.c @@ -304,7 +304,7 @@ static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa) static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) { - set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); + set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); } static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa) @@ -314,12 +314,12 @@ static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa) static inline void gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) { - clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); + clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); } static inline int gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc) { - return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa); + return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b); } static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
While in this particular case it would not be a (critical) issue, the pattern itself is bad and error prone in case somebody blindly copies to their code. Don't cast parameter to unsigned long pointer in the bit operations. Instead copy to a local variable on stack of a proper type and use. Fixes: d77e64141e32 ("KVM: s390: implement GISA IPM related primitives") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- arch/s390/include/asm/kvm_host.h | 5 ++++- arch/s390/kvm/interrupt.c | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-)