[PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[Andy Shevchenko]

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);
 }
 
 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

Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[Claudio Imbrenda]

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)

Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[Michael Mueller]

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)
> 

Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[Andy Shevchenko]

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

Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[Yury Norov]

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.

Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[Andy Shevchenko]

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

Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[Yury Norov]

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.

Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[Andy Shevchenko]

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

Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[David Laight]

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)

Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations

[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