[PATCH 0/4] KVM-S390: Fine-tuning for kvm_s390_import_bp_data()

[PATCH 0/4] KVM-S390: Fine-tuning for kvm_s390_import_bp_data()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 19:49:29 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Improve determination of sizes
  Use memdup_user()
  Less function calls after error detection
  Delete an unnecessary initialisation

 arch/s390/kvm/guestdbg.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

-- 
2.9.3

[PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 18:29:04 +0200

Replace the specification of data structures by pointer dereferences
to make the corresponding size determination a bit safer according to
the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kvm/guestdbg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index d1f8241..b68db4b 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
 		return -EINVAL;
 
-	size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint);
+	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
 	bp_data = kmalloc(size, GFP_KERNEL);
 	if (!bp_data) {
 		ret = -ENOMEM;
@@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 		}
 	}
 
-	size = nr_wp * sizeof(struct kvm_hw_wp_info_arch);
+	size = nr_wp * sizeof(*wp_info);
 	if (size > 0) {
 		wp_info = kmalloc(size, GFP_KERNEL);
 		if (!wp_info) {
@@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 			goto error;
 		}
 	}
-	size = nr_bp * sizeof(struct kvm_hw_bp_info_arch);
+	size = nr_bp * sizeof(*bp_info);
 	if (size > 0) {
 		bp_info = kmalloc(size, GFP_KERNEL);
 		if (!bp_info) {
-- 
2.9.3

Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data()

[walter harms]

Am 17.08.2016 20:06, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 18:29:04 +0200
> 
> Replace the specification of data structures by pointer dereferences
> to make the corresponding size determination a bit safer according to
> the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index d1f8241..b68db4b 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
>  		return -EINVAL;
>  
> -	size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint);
> +	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
>  	bp_data = kmalloc(size, GFP_KERNEL);
>  	if (!bp_data) {
>  		ret = -ENOMEM;
> @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  		}
>  	}
>  
> -	size = nr_wp * sizeof(struct kvm_hw_wp_info_arch);
> +	size = nr_wp * sizeof(*wp_info);
>  	if (size > 0) {
>  		wp_info = kmalloc(size, GFP_KERNEL);
>  		if (!wp_info) {
> @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  			goto error;
>  		}
>  	}
> -	size = nr_bp * sizeof(struct kvm_hw_bp_info_arch);
> +	size = nr_bp * sizeof(*bp_info);
>  	if (size > 0) {
>  		bp_info = kmalloc(size, GFP_KERNEL);
>  		if (!bp_info) {


IMHO the common pattern for kmalloc is
  bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL);

i can not remember code with a check for size < 0, i guess it is here
to avoid an overflow ? since kmalloc takes a size_t argument this would cause
a malloc failure an can be ignored.


just my 2 cents.
re,
 wh

Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data()

[Julia Lawall]

On Thu, 18 Aug 2016, walter harms wrote:

>
>
> Am 17.08.2016 20:06, schrieb SF Markus Elfring:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Wed, 17 Aug 2016 18:29:04 +0200
> >
> > Replace the specification of data structures by pointer dereferences
> > to make the corresponding size determination a bit safer according to
> > the Linux coding style convention.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  arch/s390/kvm/guestdbg.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> > index d1f8241..b68db4b 100644
> > --- a/arch/s390/kvm/guestdbg.c
> > +++ b/arch/s390/kvm/guestdbg.c
> > @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
> >  	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
> >  		return -EINVAL;
> >
> > -	size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint);
> > +	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
> >  	bp_data = kmalloc(size, GFP_KERNEL);
> >  	if (!bp_data) {
> >  		ret = -ENOMEM;
> > @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
> >  		}
> >  	}
> >
> > -	size = nr_wp * sizeof(struct kvm_hw_wp_info_arch);
> > +	size = nr_wp * sizeof(*wp_info);
> >  	if (size > 0) {
> >  		wp_info = kmalloc(size, GFP_KERNEL);
> >  		if (!wp_info) {
> > @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
> >  			goto error;
> >  		}
> >  	}
> > -	size = nr_bp * sizeof(struct kvm_hw_bp_info_arch);
> > +	size = nr_bp * sizeof(*bp_info);
> >  	if (size > 0) {
> >  		bp_info = kmalloc(size, GFP_KERNEL);
> >  		if (!bp_info) {
>
>
> IMHO the common pattern for kmalloc is
>   bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL);
>
> i can not remember code with a check for size < 0, i guess it is here
> to avoid an overflow ? since kmalloc takes a size_t argument this would cause
> a malloc failure an can be ignored.

Shoudn't it be kcalloc?

julia

>
>
> just my 2 cents.
> re,
>  wh
>

Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data()

[Paolo Bonzini]

On 18/08/2016 11:02, Julia Lawall wrote:
> 
> 
> On Thu, 18 Aug 2016, walter harms wrote:
> 
>>
>>
>> Am 17.08.2016 20:06, schrieb SF Markus Elfring:
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> Date: Wed, 17 Aug 2016 18:29:04 +0200
>>>
>>> Replace the specification of data structures by pointer dereferences
>>> to make the corresponding size determination a bit safer according to
>>> the Linux coding style convention.
>>>
>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>> ---
>>>  arch/s390/kvm/guestdbg.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
>>> index d1f8241..b68db4b 100644
>>> --- a/arch/s390/kvm/guestdbg.c
>>> +++ b/arch/s390/kvm/guestdbg.c
>>> @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>  	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
>>>  		return -EINVAL;
>>>
>>> -	size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint);
>>> +	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
>>>  	bp_data = kmalloc(size, GFP_KERNEL);
>>>  	if (!bp_data) {
>>>  		ret = -ENOMEM;
>>> @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>  		}
>>>  	}
>>>
>>> -	size = nr_wp * sizeof(struct kvm_hw_wp_info_arch);
>>> +	size = nr_wp * sizeof(*wp_info);
>>>  	if (size > 0) {
>>>  		wp_info = kmalloc(size, GFP_KERNEL);
>>>  		if (!wp_info) {
>>> @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>  			goto error;
>>>  		}
>>>  	}
>>> -	size = nr_bp * sizeof(struct kvm_hw_bp_info_arch);
>>> +	size = nr_bp * sizeof(*bp_info);
>>>  	if (size > 0) {
>>>  		bp_info = kmalloc(size, GFP_KERNEL);
>>>  		if (!bp_info) {
>>
>>
>> IMHO the common pattern for kmalloc is
>>   bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL);
>>
>> i can not remember code with a check for size < 0, i guess it is here
>> to avoid an overflow ? since kmalloc takes a size_t argument this would cause
>> a malloc failure an can be ignored.
> 
> Shoudn't it be kcalloc?

Or kmalloc_array, since zeroing is not necessary.  Might be an idea for
a new Coccinelle script, like

- kmalloc (N * sizeof T, GFP)
+ kmalloc_array(N, sizeof T, GFP)

Thanks,

Paolo

> 
> julia
> 
>>
>>
>> just my 2 cents.
>> re,
>>  wh
>>
> 

Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data()

[walter harms]

Am 18.08.2016 11:48, schrieb Paolo Bonzini:
> 
> 
> On 18/08/2016 11:02, Julia Lawall wrote:
>>
>>
>> On Thu, 18 Aug 2016, walter harms wrote:
>>
>>>
>>>
>>> Am 17.08.2016 20:06, schrieb SF Markus Elfring:
>>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>>> Date: Wed, 17 Aug 2016 18:29:04 +0200
>>>>
>>>> Replace the specification of data structures by pointer dereferences
>>>> to make the corresponding size determination a bit safer according to
>>>> the Linux coding style convention.
>>>>
>>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>>> ---
>>>>  arch/s390/kvm/guestdbg.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
>>>> index d1f8241..b68db4b 100644
>>>> --- a/arch/s390/kvm/guestdbg.c
>>>> +++ b/arch/s390/kvm/guestdbg.c
>>>> @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>>  	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
>>>>  		return -EINVAL;
>>>>
>>>> -	size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint);
>>>> +	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
>>>>  	bp_data = kmalloc(size, GFP_KERNEL);
>>>>  	if (!bp_data) {
>>>>  		ret = -ENOMEM;
>>>> @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>>  		}
>>>>  	}
>>>>
>>>> -	size = nr_wp * sizeof(struct kvm_hw_wp_info_arch);
>>>> +	size = nr_wp * sizeof(*wp_info);
>>>>  	if (size > 0) {
>>>>  		wp_info = kmalloc(size, GFP_KERNEL);
>>>>  		if (!wp_info) {
>>>> @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>>  			goto error;
>>>>  		}
>>>>  	}
>>>> -	size = nr_bp * sizeof(struct kvm_hw_bp_info_arch);
>>>> +	size = nr_bp * sizeof(*bp_info);
>>>>  	if (size > 0) {
>>>>  		bp_info = kmalloc(size, GFP_KERNEL);
>>>>  		if (!bp_info) {
>>>
>>>
>>> IMHO the common pattern for kmalloc is
>>>   bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL);
>>>
>>> i can not remember code with a check for size < 0, i guess it is here
>>> to avoid an overflow ? since kmalloc takes a size_t argument this would cause
>>> a malloc failure an can be ignored.
>>
>> Shoudn't it be kcalloc?
> 
> Or kmalloc_array, since zeroing is not necessary.  Might be an idea for
> a new Coccinelle script, like
> 
> - kmalloc (N * sizeof T, GFP)
> + kmalloc_array(N, sizeof T, GFP)
> 


my personal taste is to stay close to the libc functions.
technical there is no difference

static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
 {
        return kmalloc_array(n, size, flags | __GFP_ZERO);
 }

and i do not see any time critical things here,


re,
 wh




> Thanks,
> 
> Paolo
> 
>>
>> julia
>>
>>>
>>>
>>> just my 2 cents.
>>> re,
>>>  wh
>>>
>>
> 

Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data()

[Paolo Bonzini]

On 18/08/2016 12:52, walter harms wrote:
> 
> 
> Am 18.08.2016 11:48, schrieb Paolo Bonzini:
>>
>>
>> On 18/08/2016 11:02, Julia Lawall wrote:
>>>
>>>
>>> On Thu, 18 Aug 2016, walter harms wrote:
>>>
>>>>
>>>>
>>>> Am 17.08.2016 20:06, schrieb SF Markus Elfring:
>>>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>>>> Date: Wed, 17 Aug 2016 18:29:04 +0200
>>>>>
>>>>> Replace the specification of data structures by pointer dereferences
>>>>> to make the corresponding size determination a bit safer according to
>>>>> the Linux coding style convention.
>>>>>
>>>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>>>> ---
>>>>>  arch/s390/kvm/guestdbg.c | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
>>>>> index d1f8241..b68db4b 100644
>>>>> --- a/arch/s390/kvm/guestdbg.c
>>>>> +++ b/arch/s390/kvm/guestdbg.c
>>>>> @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>>>  	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
>>>>>  		return -EINVAL;
>>>>>
>>>>> -	size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint);
>>>>> +	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
>>>>>  	bp_data = kmalloc(size, GFP_KERNEL);
>>>>>  	if (!bp_data) {
>>>>>  		ret = -ENOMEM;
>>>>> @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>>>  		}
>>>>>  	}
>>>>>
>>>>> -	size = nr_wp * sizeof(struct kvm_hw_wp_info_arch);
>>>>> +	size = nr_wp * sizeof(*wp_info);
>>>>>  	if (size > 0) {
>>>>>  		wp_info = kmalloc(size, GFP_KERNEL);
>>>>>  		if (!wp_info) {
>>>>> @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>>>  			goto error;
>>>>>  		}
>>>>>  	}
>>>>> -	size = nr_bp * sizeof(struct kvm_hw_bp_info_arch);
>>>>> +	size = nr_bp * sizeof(*bp_info);
>>>>>  	if (size > 0) {
>>>>>  		bp_info = kmalloc(size, GFP_KERNEL);
>>>>>  		if (!bp_info) {
>>>>
>>>>
>>>> IMHO the common pattern for kmalloc is
>>>>   bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL);
>>>>
>>>> i can not remember code with a check for size < 0, i guess it is here
>>>> to avoid an overflow ? since kmalloc takes a size_t argument this would cause
>>>> a malloc failure an can be ignored.
>>>
>>> Shoudn't it be kcalloc?
>>
>> Or kmalloc_array, since zeroing is not necessary.  Might be an idea for
>> a new Coccinelle script, like
>>
>> - kmalloc (N * sizeof T, GFP)
>> + kmalloc_array(N, sizeof T, GFP)
>>
> 
> 
> my personal taste is to stay close to the libc functions.
> technical there is no difference
> 
> static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
>  {
>         return kmalloc_array(n, size, flags | __GFP_ZERO);
>  }
> 
> and i do not see any time critical things here,

This is _not_ premature optimization.  (k)calloc tells the reader that
it's safe not to initialize part of the array.  kmalloc_array says the
opposite.  Using the right function adds important hints in the
code---which unlike comments cannot get stale without also introducing
visible bugs.

Paolo

Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data()

[Cornelia Huck]

On Thu, 18 Aug 2016 12:55:11 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This is _not_ premature optimization.  (k)calloc tells the reader that
> it's safe not to initialize part of the array.  kmalloc_array says the
> opposite.  Using the right function adds important hints in the
> code---which unlike comments cannot get stale without also introducing
> visible bugs.

Ack. I'd accept a patch changing this to use kmalloc_array.

Re: Replacing specific kmalloc() calls by kmalloc_array()?

[SF Markus Elfring]

> Or kmalloc_array, since zeroing is not necessary.  Might be an idea for
> a new Coccinelle script, like
> 
> - kmalloc (N * sizeof T, GFP)
> + kmalloc_array(N, sizeof T, GFP)

I have picked your idea up. The corresponding script for the semantic
patch language became longer than your general suggestion
(if additional source code control flow aspects are integrated).

Would it make sense to check any more function combinations
in a similar way?

Regards,
Markus

Re: Replacing specific kmalloc() calls by kmalloc_array()?

[Paolo Bonzini]

----- Original Message -----
> From: "SF Markus Elfring" <elfring@users.sourceforge.net>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Julia Lawall" <julia.lawall@lip6.fr>, "walter harms" <wharms@bfs.de>, kvm@vger.kernel.org,
> linux-s390@vger.kernel.org, "Christian Bornträger" <borntraeger@de.ibm.com>, "Cornelia Huck"
> <cornelia.huck@de.ibm.com>, "David Hildenbrand" <dahi@linux.vnet.ibm.com>, "Heiko Carstens"
> <heiko.carstens@de.ibm.com>, "Martin Schwidefsky" <schwidefsky@de.ibm.com>, "Radim Krčmář" <rkrcmar@redhat.com>,
> "LKML" <linux-kernel@vger.kernel.org>, kernel-janitors@vger.kernel.org
> Sent: Wednesday, August 24, 2016 2:10:13 PM
> Subject: Re: Replacing specific kmalloc() calls by kmalloc_array()?
> 
> > Or kmalloc_array, since zeroing is not necessary.  Might be an idea for
> > a new Coccinelle script, like
> > 
> > - kmalloc (N * sizeof T, GFP)
> > + kmalloc_array(N, sizeof T, GFP)
> 
> I have picked your idea up. The corresponding script for the semantic
> patch language became longer than your general suggestion
> (if additional source code control flow aspects are integrated).
> 
> Would it make sense to check any more function combinations
> in a similar way?

I don't know :) but I'm interested in seeing the semantic patch!

Paolo

[PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 18:41:43 +0200

* Reuse existing functionality from memdup_user() instead of keeping
  duplicate source code.

  This issue was detected by using the Coccinelle software.

* Return directly if this copy operation failed.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kvm/guestdbg.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index b68db4b..8f886ee 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -217,16 +217,9 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
-	bp_data = kmalloc(size, GFP_KERNEL);
-	if (!bp_data) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	if (copy_from_user(bp_data, dbg->arch.hw_bp, size)) {
-		ret = -EFAULT;
-		goto error;
-	}
+	bp_data = memdup_user(dbg->arch.hw_bp, size);
+	if (IS_ERR(bp_data))
+		return PTR_ERR(bp_data);
 
 	for (i = 0; i < dbg->arch.nr_hw_bp; i++) {
 		switch (bp_data[i].type) {
-- 
2.9.3

Re: [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating its implementation

[David Hildenbrand]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 18:41:43 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

I like that. Thanks for running that analysis software against s390 KVM code!

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

David

Re: [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating its implementation

[Cornelia Huck]

On Wed, 17 Aug 2016 20:08:49 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 18:41:43 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.

Looks sane, but please take a bit of care about the subject: It's a bit
long, and the prefix should be "KVM: s390:".

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

if Christian wants to apply this (unless I beat him to it).

Re: [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 08/17/2016 02:08 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 18:41:43 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

I was going to apply this patch, but it probably makes sense to respin the
first patch first as suggested to avoid me fixing up the conflicts and then
you fixing up the new conflict.

So can you respin patch 1,2 as suggested and add the acks/rb for patch 2?

> ---
>  arch/s390/kvm/guestdbg.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index b68db4b..8f886ee 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -217,16 +217,9 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  		return -EINVAL;
> 
>  	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
> -	bp_data = kmalloc(size, GFP_KERNEL);
> -	if (!bp_data) {
> -		ret = -ENOMEM;
> -		goto error;
> -	}
> -
> -	if (copy_from_user(bp_data, dbg->arch.hw_bp, size)) {
> -		ret = -EFAULT;
> -		goto error;
> -	}
> +	bp_data = memdup_user(dbg->arch.hw_bp, size);
> +	if (IS_ERR(bp_data))
> +		return PTR_ERR(bp_data);
> 
>  	for (i = 0; i < dbg->arch.nr_hw_bp; i++) {
>  		switch (bp_data[i].type) {
> 

[PATCH v2 0/2] KVM: s390: Fine-tuning for kvm_s390_import_bp_data()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 24 Aug 2016 20:20:02 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Improve determination of sizes
  Use memdup_user() rather than duplicating code

 arch/s390/kvm/guestdbg.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

-- 
2.9.3

[PATCH v2 1/2] KVM: s390: Improve determination of sizes in kvm_s390_import_bp_data()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 24 Aug 2016 19:45:23 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus reuse the corresponding function "kmalloc_array".

  Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

  This issue was detected also by using the Coccinelle software.

* Replace the specification of data structures by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

* Delete the local variable "size" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2: Rebased on source files from "Linux next-20160824".
    Advices were integrated from source code review.

 arch/s390/kvm/guestdbg.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index d1f8241..70b71ac 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -206,7 +206,7 @@ static int __import_wp_info(struct kvm_vcpu *vcpu,
 int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 			    struct kvm_guest_debug *dbg)
 {
-	int ret = 0, nr_wp = 0, nr_bp = 0, i, size;
+	int ret = 0, nr_wp = 0, nr_bp = 0, i;
 	struct kvm_hw_breakpoint *bp_data = NULL;
 	struct kvm_hw_wp_info_arch *wp_info = NULL;
 	struct kvm_hw_bp_info_arch *bp_info = NULL;
@@ -216,14 +216,17 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
 		return -EINVAL;
 
-	size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint);
-	bp_data = kmalloc(size, GFP_KERNEL);
+	bp_data = kmalloc_array(dbg->arch.nr_hw_bp,
+				sizeof(*bp_data),
+				GFP_KERNEL);
 	if (!bp_data) {
 		ret = -ENOMEM;
 		goto error;
 	}
 
-	if (copy_from_user(bp_data, dbg->arch.hw_bp, size)) {
+	if (copy_from_user(bp_data,
+			   dbg->arch.hw_bp,
+			   sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
 		ret = -EFAULT;
 		goto error;
 	}
@@ -241,17 +244,19 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 		}
 	}
 
-	size = nr_wp * sizeof(struct kvm_hw_wp_info_arch);
-	if (size > 0) {
-		wp_info = kmalloc(size, GFP_KERNEL);
+	if (nr_wp > 0) {
+		wp_info = kmalloc_array(nr_wp,
+					sizeof(*wp_info),
+					GFP_KERNEL);
 		if (!wp_info) {
 			ret = -ENOMEM;
 			goto error;
 		}
 	}
-	size = nr_bp * sizeof(struct kvm_hw_bp_info_arch);
-	if (size > 0) {
-		bp_info = kmalloc(size, GFP_KERNEL);
+	if (nr_bp > 0) {
+		bp_info = kmalloc_array(nr_bp,
+					sizeof(*bp_info),
+					GFP_KERNEL);
 		if (!bp_info) {
 			ret = -ENOMEM;
 			goto error;
-- 
2.9.3

Re: [PATCH v2 1/2] KVM: s390: Improve determination of sizes in kvm_s390_import_bp_data()

[Cornelia Huck]

On Wed, 24 Aug 2016 20:36:26 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 24 Aug 2016 19:45:23 +0200
> 
> * A multiplication for the size determination of a memory allocation
>   indicated that an array data structure should be processed.
>   Thus reuse the corresponding function "kmalloc_array".
> 
>   Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> 
>   This issue was detected also by using the Coccinelle software.

Do you have the scripts you use published somewhere?

> 
> * Replace the specification of data structures by pointer dereferences
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
> 
> * Delete the local variable "size" which became unnecessary with
>   this refactoring.

I think your description is a bit on the verbose side, but not enough
to gripe more.

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> 
> v2: Rebased on source files from "Linux next-20160824".
>     Advices were integrated from source code review.
> 
>  arch/s390/kvm/guestdbg.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: KVM: s390: Improve determination of sizes in kvm_s390_import_bp_data()

[SF Markus Elfring]

>>   This issue was detected also by using the Coccinelle software.
> 
> Do you have the scripts you use published somewhere?

Not yet.

I hope that I can clarify a few more implementation details around
my evolving scripts for the semantic patch language.
I guess that this approach can be published with a higher confidence
in the near future.

Regards,
Markus

Re: Software evolution around scripts for the semantic patch langugae

[SF Markus Elfring]

>>   This issue was detected also by using the Coccinelle software.
> 
> Do you have the scripts you use published somewhere?

I would like to add another view for the corresponding software development.

The complete answer depends also on the kind of "scripts"
you are looking for. Would you like to clarify any details
a bit more here?

Regards,
Markus

Re: Software evolution around scripts for the semantic patch langugae

[Cornelia Huck]

On Thu, 25 Aug 2016 19:34:29 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >>   This issue was detected also by using the Coccinelle software.
> > 
> > Do you have the scripts you use published somewhere?
> 
> I would like to add another view for the corresponding software development.
> 
> The complete answer depends also on the kind of "scripts"
> you are looking for. Would you like to clarify any details
> a bit more here?

You obviously run some kind of semantic patching. It would really help
ease review if you could publish the semantic patches that generate
your patches - that is probably more helpful in review than just
posting the generated patches.

And it does not need to be "complete", I'd think everyone on the cc:
list here is able to handle a cocchinelle patch, for example.

Re: Software evolution around scripts for the semantic patch langugae

[SF Markus Elfring]

> You obviously run some kind of semantic patching.

Yes. - I developed some software for search patterns I became interested in.


> It would really help ease review if you could publish the semantic patches
> that generate your patches

This is reasonable.


> - that is probably more helpful in review than just posting the generated patches.

Which of the available scripts (or SmPL source files) would you like to discuss further?


> And it does not need to be "complete",

I would feel more comfortable with publishing further scripts here when I can become
more confident about relevant safety checks in this software area.


> I'd think everyone on the cc: list here is able to handle a cocchinelle patch,
> for example.

Interesting view …

I did not expect this kind of expertise by default.

Regards,
Markus

Re: Software evolution around scripts for the semantic patch langugae

[Julia Lawall]

On Thu, 25 Aug 2016, SF Markus Elfring wrote:

> >>   This issue was detected also by using the Coccinelle software.
> >
> > Do you have the scripts you use published somewhere?
>
> I would like to add another view for the corresponding software development.
>
> The complete answer depends also on the kind of "scripts"
> you are looking for. Would you like to clarify any details
> a bit more here?

I imagine that she is asking for:

@@
expression e1,e2,e3;
@@

- kmalloc(e1 * e2, e3)
+ kmalloc_array(e1, e2, e3)

Or some close variant.  It seems pretty straightforward to provide and
can help orient the reader to what is going on.

julia

Re: Software evolution around scripts for the semantic patch langugae

[SF Markus Elfring]

> Or some close variant.

I have got more script variants evolving in my software collection.

There are further approaches available from various contributors,
aren't there?

Regards,
Markus

Re: Software evolution around scripts for the semantic patch langugae

[Julia Lawall]

On Thu, 25 Aug 2016, SF Markus Elfring wrote:

> > Or some close variant.
>
> I have got more script variants evolving in my software collection.
>
> There are further approaches available from various contributors,
> aren't there?

What she is asking for is a concise and precise decription of what you
have done.  If you have some other variants, eg controlling where the
sizeof argument is (left or right of *), you don't necessarily have to
include it in the patch, if such a rule was not used for the specific
patch anyway.

julia

>
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: Software evolution around scripts for the semantic patch langugae

[Cornelia Huck]

On Thu, 25 Aug 2016 14:23:35 -0400 (EDT)
Julia Lawall <julia.lawall@lip6.fr> wrote:

> On Thu, 25 Aug 2016, SF Markus Elfring wrote:
> 
> > > Or some close variant.
> >
> > I have got more script variants evolving in my software collection.
> >
> > There are further approaches available from various contributors,
> > aren't there?
> 
> What she is asking for is a concise and precise decription of what you
> have done.  If you have some other variants, eg controlling where the
> sizeof argument is (left or right of *), you don't necessarily have to
> include it in the patch, if such a rule was not used for the specific
> patch anyway.

*nod*

If I see a patch that says "I've run the following cocchinelle patch to
perform $TRANSFORMATION, and here's the result", I can be reasonably
sure that the result will be what is intended to be changed in the
first place (and I can assess whether the change makes sense at all.)
If I see only the resulting patch, I won't know whether you have
performed the changes manually (and possibly introduced bugs, as
happens to all of us.)

Moreover, a good semantic patch is useful to others as well and might
even be reused in other contexts that have similar requirements. You
really lose value if you don't publish them.

[PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating code

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 24 Aug 2016 20:10:09 +0200

* Reuse existing functionality from memdup_user() instead of keeping
  duplicate source code.

  This issue was detected by using the Coccinelle software.

* Return directly if this copy operation failed.

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2: Rebased on source files from "Linux next-20160824".

 arch/s390/kvm/guestdbg.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index 70b71ac..d7c6a7f 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -216,20 +216,10 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
 		return -EINVAL;
 
-	bp_data = kmalloc_array(dbg->arch.nr_hw_bp,
-				sizeof(*bp_data),
-				GFP_KERNEL);
-	if (!bp_data) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	if (copy_from_user(bp_data,
-			   dbg->arch.hw_bp,
-			   sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
-		ret = -EFAULT;
-		goto error;
-	}
+	bp_data = memdup_user(dbg->arch.hw_bp,
+			      sizeof(*bp_data) * dbg->arch.nr_hw_bp);
+	if (IS_ERR(bp_data))
+		return PTR_ERR(bp_data);
 
 	for (i = 0; i < dbg->arch.nr_hw_bp; i++) {
 		switch (bp_data[i].type) {
-- 
2.9.3

Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating code

[Cornelia Huck]

On Wed, 24 Aug 2016 20:40:03 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 24 Aug 2016 20:10:09 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.
> 
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> 
> v2: Rebased on source files from "Linux next-20160824".
> 
>  arch/s390/kvm/guestdbg.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating code

[Geert Uytterhoeven]

Hi Markus,

On Wed, Aug 24, 2016 at 8:40 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 24 Aug 2016 20:10:09 +0200
>
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
>
>   This issue was detected by using the Coccinelle software.
>
> * Return directly if this copy operation failed.
>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>
> v2: Rebased on source files from "Linux next-20160824".
>
>  arch/s390/kvm/guestdbg.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index 70b71ac..d7c6a7f 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -216,20 +216,10 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>         else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
>                 return -EINVAL;
>
> -       bp_data = kmalloc_array(dbg->arch.nr_hw_bp,
> -                               sizeof(*bp_data),
> -                               GFP_KERNEL);

Probably not an issue here, but if "sizeof(*bp_data) * dbg->arch.nr_hw_bp"
overflows, kmalloc_array() would have returned NULL here...

> -       if (!bp_data) {
> -               ret = -ENOMEM;
> -               goto error;
> -       }
> -
> -       if (copy_from_user(bp_data,
> -                          dbg->arch.hw_bp,
> -                          sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
> -               ret = -EFAULT;
> -               goto error;
> -       }
> +       bp_data = memdup_user(dbg->arch.hw_bp,
> +                             sizeof(*bp_data) * dbg->arch.nr_hw_bp);

... while this would continue silently, and corrupt memory.

> +       if (IS_ERR(bp_data))
> +               return PTR_ERR(bp_data);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating code

[SF Markus Elfring]

>> -       if (!bp_data) {
>> -               ret = -ENOMEM;
>> -               goto error;
>> -       }
>> -
>> -       if (copy_from_user(bp_data,
>> -                          dbg->arch.hw_bp,
>> -                          sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
>> -               ret = -EFAULT;
>> -               goto error;
>> -       }
>> +       bp_data = memdup_user(dbg->arch.hw_bp,
>> +                             sizeof(*bp_data) * dbg->arch.nr_hw_bp);
> 
> ... while this would continue silently,

How do you think about to explain this information a bit more?


> and corrupt memory.

I wonder about this conclusion at the moment.

Did you notice the check "IS_ERR(bp_data)" and the corresponding reaction
in this update suggestion?

How does your feedback fit to the tag "Acked-by: Cornelia Huck"
from 2016-08-25?

Regards,
Markus

Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating code

[Geert Uytterhoeven]

Hi Markus,

On Mon, Oct 3, 2016 at 2:28 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> -       if (!bp_data) {
>>> -               ret = -ENOMEM;
>>> -               goto error;
>>> -       }
>>> -
>>> -       if (copy_from_user(bp_data,
>>> -                          dbg->arch.hw_bp,
>>> -                          sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
>>> -               ret = -EFAULT;
>>> -               goto error;
>>> -       }
>>> +       bp_data = memdup_user(dbg->arch.hw_bp,
>>> +                             sizeof(*bp_data) * dbg->arch.nr_hw_bp);
>>
>> ... while this would continue silently,
>
> How do you think about to explain this information a bit more?

kmalloc_array() has a builtin check for overflow while calculating the size.
This is the real reason why it's better to use kmalloc_array() than
kzalloc(n * size). If "n * size" overflow, kzalloc(n * size) would allocate a
memory block with a bogus size.

>> and corrupt memory.
>
> I wonder about this conclusion at the moment.
>
> Did you notice the check "IS_ERR(bp_data)" and the corresponding reaction
> in this update suggestion?

Yes, but bp_data may still be a valid (as in "not an error") value.

Your commit a1708a2eaded836b ("KVM: s390: Improve determination of sizes in
kvm_s390_import_bp_data()") made the code more robust, as kmalloc_array() ha
 a builtin overflow check, and will return NULL if overflow is detected.
However, commit 0624a8eb82efd58e ("KVM: s390: Use memdup_user() rather than
duplicating code") dropped that safety net again.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating code

[SF Markus Elfring]

>> Did you notice the check "IS_ERR(bp_data)" and the corresponding reaction
>> in this update suggestion?
> 
> Yes, but bp_data may still be a valid (as in "not an error") value.

Thanks for your constructive feedback.


> Your commit a1708a2eaded836b ("KVM: s390: Improve determination of sizes in
> kvm_s390_import_bp_data()") made the code more robust, as kmalloc_array() ha
>  a builtin overflow check, and will return NULL if overflow is detected.
> However, commit 0624a8eb82efd58e ("KVM: s390: Use memdup_user() rather than
> duplicating code") dropped that safety net again.

* How much are you concerned about the shown software evolution around
  multiplications for memory allocations?

* Does there really a probability remain that an inappropriate product
  would be calculated here (as the situation was before my two update steps
  for this software module)?

* Can it be that you are looking for a variant of a function like "memdup_user"
  where values can be passed as separate parameters "count" and "size" so that
  the needed multiplication and corresponding overflow check would be performed
  together as desired?

Regards,
Markus

Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating code

[Geert Uytterhoeven]

Hi Markus,

On Mon, Oct 3, 2016 at 3:47 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> Did you notice the check "IS_ERR(bp_data)" and the corresponding reaction
>>> in this update suggestion?
>>
>> Yes, but bp_data may still be a valid (as in "not an error") value.
>
> Thanks for your constructive feedback.
>
>> Your commit a1708a2eaded836b ("KVM: s390: Improve determination of sizes in
>> kvm_s390_import_bp_data()") made the code more robust, as kmalloc_array() ha
>>  a builtin overflow check, and will return NULL if overflow is detected.
>> However, commit 0624a8eb82efd58e ("KVM: s390: Use memdup_user() rather than
>> duplicating code") dropped that safety net again.
>
> * How much are you concerned about the shown software evolution around
>   multiplications for memory allocations?

Enough to write my reply ;-)

> * Does there really a probability remain that an inappropriate product
>   would be calculated here (as the situation was before my two update steps
>   for this software module)?

Perhaps not. Hence my "Probably not an issue here, ...".

> * Can it be that you are looking for a variant of a function like "memdup_user"
>   where values can be passed as separate parameters "count" and "size" so that
>   the needed multiplication and corresponding overflow check would be performed
>   together as desired?

If there are enough uses, and people like it, adding memdup_user_array()
may be a good idea...

P.S. Why do your questions make me think of a scientific paper? ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: KVM: s390: Use memdup_user() rather than duplicating code

[SF Markus Elfring]

>> * Does there really a probability remain that an inappropriate product
>>   would be calculated here (as the situation was before my two update steps
>>   for this software module)?
> 
> Perhaps not. Hence my "Probably not an issue here, ...".

Thanks for your clarification.


>> * Can it be that you are looking for a variant of a function like "memdup_user"
>>   where values can be passed as separate parameters "count" and "size" so that
>>   the needed multiplication and corresponding overflow check would be performed
>>   together as desired?
> 
> If there are enough uses, and people like it, adding memdup_user_array()
> may be a good idea...

How are the chances of such an addition for the Linux programming interface?


> P.S. Why do your questions make me think of a scientific paper? ;-)

Would you like to recommend any for further reading?   ;-)

Regards,
Markus

Re: [PATCH v2 0/2] KVM: s390: Fine-tuning for kvm_s390_import_bp_data()

[Christian Borntraeger]

On 08/24/2016 02:30 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 24 Aug 2016 20:20:02 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (2):
>   Improve determination of sizes
>   Use memdup_user() rather than duplicating code
> 
>  arch/s390/kvm/guestdbg.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 

Thanks applied.

[PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 19:25:50 +0200

The kfree() function was called in a few cases by the
kvm_s390_import_bp_data() function during error handling
even if a passed variable contained a null pointer.

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kvm/guestdbg.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index 8f886ee..f2514af 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -239,7 +239,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 		wp_info = kmalloc(size, GFP_KERNEL);
 		if (!wp_info) {
 			ret = -ENOMEM;
-			goto error;
+			goto free_bp_data;
 		}
 	}
 	size = nr_bp * sizeof(*bp_info);
@@ -247,7 +247,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 		bp_info = kmalloc(size, GFP_KERNEL);
 		if (!bp_info) {
 			ret = -ENOMEM;
-			goto error;
+			goto free_wp_info;
 		}
 	}
 
@@ -257,7 +257,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 			ret = __import_wp_info(vcpu, &bp_data[i],
 					       &wp_info[nr_wp]);
 			if (ret)
-				goto error;
+				goto free_bp_info;
 			nr_wp++;
 			break;
 		case KVM_HW_BP:
@@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
 	vcpu->arch.guestdbg.hw_wp_info = wp_info;
 	return 0;
-error:
-	kfree(bp_data);
-	kfree(wp_info);
+free_bp_info:
 	kfree(bp_info);
+free_wp_info:
+	kfree(wp_info);
+free_bp_data:
+	kfree(bp_data);
 	return ret;
 }
 
-- 
2.9.3

Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[David Hildenbrand]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:25:50 +0200
> 
> The kfree() function was called in a few cases by the
> kvm_s390_import_bp_data() function during error handling
> even if a passed variable contained a null pointer.
> 
> Adjust jump targets according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 

Nack, we don't need micro optimization for error handling code. Adding more
jump labels is never a good idea, it just increases complexity.

David

Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[Cornelia Huck]

On Wed, 17 Aug 2016 20:10:37 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:25:50 +0200
> 
> The kfree() function was called in a few cases by the
> kvm_s390_import_bp_data() function during error handling
> even if a passed variable contained a null pointer.
> 
> Adjust jump targets according to the Linux coding style convention.

NACK.

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 

> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
>  	return 0;
> -error:
> -	kfree(bp_data);
> -	kfree(wp_info);
> +free_bp_info:
>  	kfree(bp_info);
> +free_wp_info:
> +	kfree(wp_info);
> +free_bp_data:
> +	kfree(bp_data);
>  	return ret;
>  }
> 

This replaces a perfectly fine fallthrough with some horrible labels.
Please don't do that.

Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[SF Markus Elfring]

>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
>>  	return 0;
>> -error:
>> -	kfree(bp_data);
>> -	kfree(wp_info);
>> +free_bp_info:
>>  	kfree(bp_info);
>> +free_wp_info:
>> +	kfree(wp_info);
>> +free_bp_data:
>> +	kfree(bp_data);
>>  	return ret;
>>  }
>>
> 
> This replaces a perfectly fine fallthrough

The usage of a single goto label like "error" seems to be convenient.
But how do these habits fit to the current Linux coding style convention?


> with some horrible labels.

Do they explain better which processing steps should be performed
for an efficient exception handling in this function implementation?

Regards,
Markus

Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[Cornelia Huck]

On Mon, 22 Aug 2016 18:56:47 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
> >>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
> >>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
> >>  	return 0;
> >> -error:
> >> -	kfree(bp_data);
> >> -	kfree(wp_info);
> >> +free_bp_info:
> >>  	kfree(bp_info);
> >> +free_wp_info:
> >> +	kfree(wp_info);
> >> +free_bp_data:
> >> +	kfree(bp_data);
> >>  	return ret;
> >>  }
> >>
> > 
> > This replaces a perfectly fine fallthrough
> 
> The usage of a single goto label like "error" seems to be convenient.
> But how do these habits fit to the current Linux coding style convention?
> 
> 
> > with some horrible labels.
> 
> Do they explain better which processing steps should be performed
> for an efficient exception handling in this function implementation?

*sigh*

It's _exception handling_. It does not need to be "efficient", it needs
to be easily parsable by humans. If in doubt, the compiler will be
_much_ better at optimizing that kind of stuff anyway.

So still NACK.

Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[SF Markus Elfring]

>>>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>>>>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
>>>>  	return 0;
>>>> -error:
>>>> -	kfree(bp_data);
>>>> -	kfree(wp_info);
>>>> +free_bp_info:
>>>>  	kfree(bp_info);
>>>> +free_wp_info:
>>>> +	kfree(wp_info);
>>>> +free_bp_data:
>>>> +	kfree(bp_data);
>>>>  	return ret;
>>>>  }
>>>>
>>>
>>> This replaces a perfectly fine fallthrough
>>
>> The usage of a single goto label like "error" seems to be convenient.
>> But how do these habits fit to the current Linux coding style convention?
>>
>>
>>> with some horrible labels.
>>
>> Do they explain better which processing steps should be performed
>> for an efficient exception handling in this function implementation?
> 
> *sigh*
> 
> It's _exception handling_. It does not need to be "efficient",

I imagine that run time situations could evolve where software efficiency
will also matter for this purpose.


> it needs to be easily parsable by humans.

I guess that we have got different preferences for this detail.


> If in doubt, the compiler will be _much_ better at optimizing
> that kind of stuff anyway.

Which compiler (or optimizer) implementation is capable to restructure
the jump targets for you automatically in the way I propose here?

Regards,
Markus

Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[Cornelia Huck]

On Mon, 22 Aug 2016 23:17:26 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >>>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
> >>>>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
> >>>>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
> >>>>  	return 0;
> >>>> -error:
> >>>> -	kfree(bp_data);
> >>>> -	kfree(wp_info);
> >>>> +free_bp_info:
> >>>>  	kfree(bp_info);
> >>>> +free_wp_info:
> >>>> +	kfree(wp_info);
> >>>> +free_bp_data:
> >>>> +	kfree(bp_data);
> >>>>  	return ret;
> >>>>  }
> >>>>
> >>>
> >>> This replaces a perfectly fine fallthrough
> >>
> >> The usage of a single goto label like "error" seems to be convenient.
> >> But how do these habits fit to the current Linux coding style convention?
> >>
> >>
> >>> with some horrible labels.
> >>
> >> Do they explain better which processing steps should be performed
> >> for an efficient exception handling in this function implementation?
> > 
> > *sigh*
> > 
> > It's _exception handling_. It does not need to be "efficient",
> 
> I imagine that run time situations could evolve where software efficiency
> will also matter for this purpose.

*major sigh*

We can start to optimize error handling that should never run after we
fixed every other performance problem that we have. Not earlier.

> 
> 
> > it needs to be easily parsable by humans.
> 
> I guess that we have got different preferences for this detail.

And I'm maintainer for this code.

> 
> 
> > If in doubt, the compiler will be _much_ better at optimizing
> > that kind of stuff anyway.
> 
> Which compiler (or optimizer) implementation is capable to restructure
> the jump targets for you automatically in the way I propose here?

No, please stop right here. NACK. EOD.

Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[Paolo Bonzini]

On 22/08/2016 23:17, SF Markus Elfring wrote:
>> If in doubt, the compiler will be _much_ better at optimizing
>> that kind of stuff anyway.
> 
> Which compiler (or optimizer) implementation is capable to restructure
> the jump targets for you automatically in the way I propose here?

If kfree were implemented as

	if (p)
		really_kfree(p);

then the compiler would be able to jump over the NULL test.  In
principle one could also add a "does nothing if NULL" attribute to GCC
and annotate kfree with it.

Paolo

Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[Christian Borntraeger]

On 08/17/2016 02:10 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:25:50 +0200
> 
> The kfree() function was called in a few cases by the
> kvm_s390_import_bp_data() function during error handling
> even if a passed variable contained a null pointer.
> 
> Adjust jump targets according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index 8f886ee..f2514af 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -239,7 +239,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  		wp_info = kmalloc(size, GFP_KERNEL);
>  		if (!wp_info) {
>  			ret = -ENOMEM;
> -			goto error;
> +			goto free_bp_data;
>  		}
>  	}
>  	size = nr_bp * sizeof(*bp_info);
> @@ -247,7 +247,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  		bp_info = kmalloc(size, GFP_KERNEL);
>  		if (!bp_info) {
>  			ret = -ENOMEM;
> -			goto error;
> +			goto free_wp_info;
>  		}
>  	}
> 
> @@ -257,7 +257,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  			ret = __import_wp_info(vcpu, &bp_data[i],
>  					       &wp_info[nr_wp]);
>  			if (ret)
> -				goto error;
> +				goto free_bp_info;
>  			nr_wp++;
>  			break;
>  		case KVM_HW_BP:
> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
>  	return 0;
> -error:
> -	kfree(bp_data);
> -	kfree(wp_info);
> +free_bp_info:
>  	kfree(bp_info);
> +free_wp_info:
> +	kfree(wp_info);
> +free_bp_data:
> +	kfree(bp_data);
>  	return ret;
>  }

I agree with Cornelia, while it seems correct from a technical point of view, it will
make the code harder to maintain. For example if we ever add a new malloc and remove 
another one over time we would need to reshuffle the labels and this did went wrong
several times in the past.

Christian

Re: KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

[SF Markus Elfring]

>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
>>  	return 0;
>> -error:
>> -	kfree(bp_data);
>> -	kfree(wp_info);
>> +free_bp_info:
>>  	kfree(bp_info);
>> +free_wp_info:
>> +	kfree(wp_info);
>> +free_bp_data:
>> +	kfree(bp_data);
>>  	return ret;
>>  }
> 
> I agree with Cornelia,

This is generally fine.


> while it seems correct from a technical point of view,

Thanks for another bit of acknowledgement.


> it will make the code harder to maintain.

I agree that there some efforts and challenges involved.


> For example if we ever add a new malloc and remove another one

Do you see any changes coming from this direction?


> over time we would need to reshuffle the labels

This can occasionally happen, can't it?


> and this did went wrong several times in the past.

Would you like to add any corresponding software development experiences
to discussions around a topic like "CodingStyle: add some more error
handling guidelines"?

https://www.spinics.net/lists/linux-doc/msg39307.html
http://marc.info/?l=linux-doc&m=147187538413914

Regards,
Markus

[PATCH 4/4] KVM-S390: Delete an unnecessary initialisation for a buffer variable

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 19:28:15 +0200

The variable "bp_data" will eventually be set to an appropriate pointer
from a call of the memdup_user() function.
Thus omit the explicit initialisation which became unnecessary with
a previous update step.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kvm/guestdbg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index f2514af..ad04609 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -207,7 +207,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 			    struct kvm_guest_debug *dbg)
 {
 	int ret = 0, nr_wp = 0, nr_bp = 0, i, size;
-	struct kvm_hw_breakpoint *bp_data = NULL;
+	struct kvm_hw_breakpoint *bp_data;
 	struct kvm_hw_wp_info_arch *wp_info = NULL;
 	struct kvm_hw_bp_info_arch *bp_info = NULL;
 
-- 
2.9.3

Re: [PATCH 4/4] KVM-S390: Delete an unnecessary initialisation for a buffer variable

[David Hildenbrand]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:28:15 +0200
> 
> The variable "bp_data" will eventually be set to an appropriate pointer
> from a call of the memdup_user() function.
> Thus omit the explicit initialisation which became unnecessary with
> a previous update step.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Looks sane to me.

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

Thanks!

David

Re: [PATCH 4/4] KVM-S390: Delete an unnecessary initialisation for a buffer variable

[Cornelia Huck]

On Wed, 17 Aug 2016 20:12:15 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:28:15 +0200
> 
> The variable "bp_data" will eventually be set to an appropriate pointer
> from a call of the memdup_user() function.
> Thus omit the explicit initialisation which became unnecessary with
> a previous update step.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index f2514af..ad04609 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -207,7 +207,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  			    struct kvm_guest_debug *dbg)
>  {
>  	int ret = 0, nr_wp = 0, nr_bp = 0, i, size;
> -	struct kvm_hw_breakpoint *bp_data = NULL;
> +	struct kvm_hw_breakpoint *bp_data;
>  	struct kvm_hw_wp_info_arch *wp_info = NULL;
>  	struct kvm_hw_bp_info_arch *bp_info = NULL;
> 

NACK.