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 <[email protected]>
---
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);
}
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)
--
2.34.1
On 24.02.22 12:36, Claudio Imbrenda wrote:
> On Wed, 23 Feb 2022 18:44:20 +0200
> Andy Shevchenko <[email protected]> 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 <[email protected]>
>> ---
>> 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 Wed, 23 Feb 2022 18:44:20 +0200
Andy Shevchenko <[email protected]> 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 <[email protected]>
> ---
> 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 Thu, Feb 24, 2022 at 2:51 PM Claudio Imbrenda <[email protected]> wrote:
>
> On Wed, 23 Feb 2022 18:44:20 +0200
> Andy Shevchenko <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
From: Andy Shevchenko
> Sent: 24 February 2022 19:51
>
> On Thu, Feb 24, 2022 at 2:51 PM Claudio Imbrenda <[email protected]> wrote:
> >
> > On Wed, 23 Feb 2022 18:44:20 +0200
> > Andy Shevchenko <[email protected]> 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 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.
--
With Best Regards,
Andy Shevchenko
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 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?
--
With Best Regards,
Andy Shevchenko
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?
--
With Best Regards,
Andy Shevchenko
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!
--
With Best Regards,
Andy Shevchenko