[PATCH] s390/pkey: Use memdup_user() rather than duplicating its implementation

[PATCH] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 6 Nov 2019 11:11:42 +0100

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

Generated by: scripts/coccinelle/api/memdup_user.cocci

Delete local variables which became unnecessary with this refactoring
in two function implementations.

Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/s390/crypto/pkey_api.c | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
index 9de3d46b3253..8c0f12686cba 100644
--- a/drivers/s390/crypto/pkey_api.c
+++ b/drivers/s390/crypto/pkey_api.c
@@ -715,36 +715,16 @@ static int pkey_apqns4keytype(enum pkey_key_type ktype,

 static void *_copy_key_from_user(void __user *ukey, size_t keylen)
 {
-	void *kkey;
-
-	if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE)
-		return ERR_PTR(-EINVAL);
-	kkey = kmalloc(keylen, GFP_KERNEL);
-	if (!kkey)
-		return ERR_PTR(-ENOMEM);
-	if (copy_from_user(kkey, ukey, keylen)) {
-		kfree(kkey);
-		return ERR_PTR(-EFAULT);
-	}
-
-	return kkey;
+	return !ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE
+	       ? ERR_PTR(-EINVAL)
+	       : memdup_user(ukey, keylen);
 }

 static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns)
 {
-	void *kapqns = NULL;
-	size_t nbytes;
-
-	if (uapqns && nr_apqns > 0) {
-		nbytes = nr_apqns * sizeof(struct pkey_apqn);
-		kapqns = kmalloc(nbytes, GFP_KERNEL);
-		if (!kapqns)
-			return ERR_PTR(-ENOMEM);
-		if (copy_from_user(kapqns, uapqns, nbytes))
-			return ERR_PTR(-EFAULT);
-	}
-
-	return kapqns;
+	return uapqns && nr_apqns > 0
+	       ? memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn))
+	       : NULL;
 }

 static long pkey_unlocked_ioctl(struct file *filp, unsigned int cmd,
--
2.24.0

Re: [PATCH] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Joe Perches]

On Wed, 2019-11-06 at 11:22 +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 6 Nov 2019 11:11:42 +0100
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> Generated by: scripts/coccinelle/api/memdup_user.cocci
> 
> Delete local variables which became unnecessary with this refactoring
> in two function implementations.
> 
> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")

This doesn't fix anything and the Fixes: line is not appropriate.

> diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
[]
> @@ -715,36 +715,16 @@ static int pkey_apqns4keytype(enum pkey_key_type ktype,
> 
>  static void *_copy_key_from_user(void __user *ukey, size_t keylen)
>  {
> -	void *kkey;
> -
> -	if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE)
> -		return ERR_PTR(-EINVAL);
> -	kkey = kmalloc(keylen, GFP_KERNEL);
> -	if (!kkey)
> -		return ERR_PTR(-ENOMEM);
> -	if (copy_from_user(kkey, ukey, keylen)) {
> -		kfree(kkey);
> -		return ERR_PTR(-EFAULT);
> -	}
> -
> -	return kkey;
> +	return !ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE
> +	       ? ERR_PTR(-EINVAL)
> +	       : memdup_user(ukey, keylen);

This is a very poor use of ternary ?: code.
This is much more readable for humans.

	if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KBLOBBUFSIZE)
		return ERR_PTR(-EINVAL);

	return memdup_user(ukey, keylen);

The compiler will produce the same code.

>  static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns)
>  {
> -	void *kapqns = NULL;
> -	size_t nbytes;
> -
> -	if (uapqns && nr_apqns > 0) {
> -		nbytes = nr_apqns * sizeof(struct pkey_apqn);
> -		kapqns = kmalloc(nbytes, GFP_KERNEL);
> -		if (!kapqns)
> -			return ERR_PTR(-ENOMEM);
> -		if (copy_from_user(kapqns, uapqns, nbytes))
> -			return ERR_PTR(-EFAULT);
> -	}
> -
> -	return kapqns;
> +	return uapqns && nr_apqns > 0
> +	       ? memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn))
> +	       : NULL;

And here you reverse the form of the earlier block.
Please be consistent and use this style:

	if (!uapqns || nr_apqns <= 0)
		return NULL;

	return memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn));

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

>> Reuse existing functionality from memdup_user() instead of keeping
>> duplicate source code.
>>
>> Generated by: scripts/coccinelle/api/memdup_user.cocci
…
>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>
> This doesn't fix anything

How would you categorise the proposed source code reduction and software reuse?


> and the Fixes: line is not appropriate.

Will the development opinions vary between contributors?


>> +	return !ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE
>> +	       ? ERR_PTR(-EINVAL)
>> +	       : memdup_user(ukey, keylen);
>
> This is a very poor use of ternary ?: code.

The conditional operator is applied once more in the intended way,
isn't it?


> This is much more readable for humans.

Readability preferences can vary also for this code structure.


>> +	return uapqns && nr_apqns > 0
>> +	       ? memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn))
>> +	       : NULL;
>
> And here you reverse the form of the earlier block.

I kept the previous condition specification.


> Please be consistent and use this style:

Would further developers like to get a more verbose variant for this
software transformation?

Regards,
Markus

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Joe Perches]

On Wed, 2019-11-06 at 14:00 +0100, Markus Elfring wrote:
> > > Reuse existing functionality from memdup_user() instead of keeping
> > > duplicate source code.
> > > 
> > > Generated by: scripts/coccinelle/api/memdup_user.cocci
> …
> > > Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
> > 
> > This doesn't fix anything
> 
> How would you categorise the proposed source code reduction and software reuse?

As inappropriate for a fixes tag.

The fixes tag is "used to make it easy to determine where a bug
originated, which can help review a bug fix"

There is no bug here.

> Will the development opinions vary between contributors?

Ever had a bikeshed?

> > > +	return !ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE
> > > +	       ? ERR_PTR(-EINVAL)
> > > +	       : memdup_user(ukey, keylen);
> > 
> > This is a very poor use of ternary ?: code.
> 
> The conditional operator is applied once more in the intended way,
> isn't it?

Please take your development efforts to obfuscated c contests.

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

> There is no bug here.

Do you find duplicated source code questionable?

Is this also an error item?

Regards,
Markus

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Joe Perches]

On Wed, 2019-11-06 at 19:55 +0100, Markus Elfring wrote:
> > There is no bug here.
> 
> Do you find duplicated source code questionable?

No.  It is something that can be improved through
code consolidation though.

> Is this also an error item?

Definitely not.  It is _only_ an error if there is
some logic defect.  There is no logic defect here.

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

>>> There is no bug here.
>>
>> Do you find duplicated source code questionable?
>
> No.

Do you disagree to the following information which can be provided by
a coccicheck run?

./drivers/s390/crypto/pkey_api.c:722:8-15: WARNING opportunity for memdup_user


> It is something that can be improved through
> code consolidation though.
>
>> Is this also an error item?
>
> Definitely not.

Your view seems to be very limited at the moment.


> It is _only_ an error if there is some logic defect.
> There is no logic defect here.

I suggest to consider additional software weaknesses besides logic errors.
Do we occasionally care any more for the development principle
“Don't repeat yourself”?

Regards,
Markus

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

>> Reuse existing functionality from memdup_user() instead of keeping
>> duplicate source code.
>>
>> Generated by: scripts/coccinelle/api/memdup_user.cocci
…
>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>
> This doesn't fix anything

How would you categorise the proposed source code reduction and software reuse?


> and the Fixes: line is not appropriate.

Will the development opinions vary between contributors?


>> +	return !ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE
>> +	       ? ERR_PTR(-EINVAL)
>> +	       : memdup_user(ukey, keylen);
>
> This is a very poor use of ternary ?: code.

The conditional operator is applied once more in the intended way,
isn't it?


> This is much more readable for humans.

Readability preferences can vary also for this code structure.


>> +	return uapqns && nr_apqns > 0
>> +	       ? memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn))
>> +	       : NULL;
>
> And here you reverse the form of the earlier block.

I kept the previous condition specification.


> Please be consistent and use this style:

Would further developers like to get a more verbose variant for this
software transformation?

Regards,
Markus

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 06.11.19 14:00, Markus Elfring wrote:
>>> Reuse existing functionality from memdup_user() instead of keeping
>>> duplicate source code.
>>>
>>> Generated by: scripts/coccinelle/api/memdup_user.cocci
> …
>>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>>
>> This doesn't fix anything
> 
> How would you categorise the proposed source code reduction and software reuse?

Cleanup.

Can you please stop arguing about review feedback that is clearly right? This is not fixing 
anything. The Fixes tag is used to decide if something needs a backport. And every backport has 
a chance for a regression. So adding a Fixes tag to something that is not fixing a bug is actually
hurting. 

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Dan Carpenter]

On Wed, Nov 06, 2019 at 07:30:19PM +0100, Christian Borntraeger wrote:
> 
> 
> On 06.11.19 14:00, Markus Elfring wrote:
> >>> Reuse existing functionality from memdup_user() instead of keeping
> >>> duplicate source code.
> >>>
> >>> Generated by: scripts/coccinelle/api/memdup_user.cocci
> > …
> >>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
> >>
> >> This doesn't fix anything
> > 
> > How would you categorise the proposed source code reduction and software reuse?
> 
> Cleanup.
> 
> Can you please stop arguing about review feedback that is clearly right? This is not fixing 
> anything. The Fixes tag is used to decide if something needs a backport.

Fixes tags are independent from backports.  If you want a backport you
should CC stable.

Fixes tags are useful for a bunch of things like when you're reviewing a
patch you can look at the original commit to see what was intended.
Also we can do automated analysis to see what sort of commits introduce
bugs (did they spend time in linux-next etc).

regards,
dan carpenter

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 07.11.19 07:48, Dan Carpenter wrote:
> On Wed, Nov 06, 2019 at 07:30:19PM +0100, Christian Borntraeger wrote:
>>
>>
>> On 06.11.19 14:00, Markus Elfring wrote:
>>>>> Reuse existing functionality from memdup_user() instead of keeping
>>>>> duplicate source code.
>>>>>
>>>>> Generated by: scripts/coccinelle/api/memdup_user.cocci
>>> …
>>>>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>>>>
>>>> This doesn't fix anything
>>>
>>> How would you categorise the proposed source code reduction and software reuse?
>>
>> Cleanup.
>>
>> Can you please stop arguing about review feedback that is clearly right? This is not fixing 
>> anything. The Fixes tag is used to decide if something needs a backport.
> 
> Fixes tags are independent from backports.  If you want a backport you
> should CC stable.

I agree, but a Fixes tag increases the likelyhood of automatic backports anyway.
(e.g. some distro kernel maintainers check for this tag).

[PATCH v2] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 7 Nov 2019 10:40:18 +0100

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

Generated by: scripts/coccinelle/api/memdup_user.cocci

Delete local variables which became unnecessary with this refactoring
in two function implementations.

Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
Further changes were requested by Joe Perches.
https://lore.kernel.org/r/6137855bb4170c438c7436cbdb7dfd21639a8855.camel@perches.com/

* The proposed usage of two conditional operators was replaced by
  an other code structure.

* A sanity check was adjusted for the function “_copy_apqns_from_user”.


 drivers/s390/crypto/pkey_api.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
index 9de3d46b3253..ac99fd97569d 100644
--- a/drivers/s390/crypto/pkey_api.c
+++ b/drivers/s390/crypto/pkey_api.c
@@ -715,36 +715,18 @@ static int pkey_apqns4keytype(enum pkey_key_type ktype,

 static void *_copy_key_from_user(void __user *ukey, size_t keylen)
 {
-	void *kkey;
-
 	if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE)
 		return ERR_PTR(-EINVAL);
-	kkey = kmalloc(keylen, GFP_KERNEL);
-	if (!kkey)
-		return ERR_PTR(-ENOMEM);
-	if (copy_from_user(kkey, ukey, keylen)) {
-		kfree(kkey);
-		return ERR_PTR(-EFAULT);
-	}

-	return kkey;
+	return memdup_user(ukey, keylen);
 }

 static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns)
 {
-	void *kapqns = NULL;
-	size_t nbytes;
-
-	if (uapqns && nr_apqns > 0) {
-		nbytes = nr_apqns * sizeof(struct pkey_apqn);
-		kapqns = kmalloc(nbytes, GFP_KERNEL);
-		if (!kapqns)
-			return ERR_PTR(-ENOMEM);
-		if (copy_from_user(kapqns, uapqns, nbytes))
-			return ERR_PTR(-EFAULT);
-	}
+	if (!uapqns || nr_apqns <= 0)
+		return NULL;

-	return kapqns;
+	return memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn));
 }

 static long pkey_unlocked_ioctl(struct file *filp, unsigned int cmd,
--
2.24.0

Re: [PATCH v2] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 07.11.19 11:06, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 7 Nov 2019 10:40:18 +0100
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> Generated by: scripts/coccinelle/api/memdup_user.cocci
> 
> Delete local variables which became unnecessary with this refactoring
> in two function implementations.
> 
> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")

With that patch description, the Fixes tag is wrong...but (see below)

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> 
> v2:
> Further changes were requested by Joe Perches.
> https://lore.kernel.org/r/6137855bb4170c438c7436cbdb7dfd21639a8855.camel@perches.com/
> 
> * The proposed usage of two conditional operators was replaced by
>   an other code structure.
> 
> * A sanity check was adjusted for the function “_copy_apqns_from_user”.
> 
> 
>  drivers/s390/crypto/pkey_api.c | 26 ++++----------------------
>  1 file changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
> index 9de3d46b3253..ac99fd97569d 100644
> --- a/drivers/s390/crypto/pkey_api.c
> +++ b/drivers/s390/crypto/pkey_api.c
> @@ -715,36 +715,18 @@ static int pkey_apqns4keytype(enum pkey_key_type ktype,
> 
>  static void *_copy_key_from_user(void __user *ukey, size_t keylen)
>  {
> -	void *kkey;
> -
>  	if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE)
>  		return ERR_PTR(-EINVAL);
> -	kkey = kmalloc(keylen, GFP_KERNEL);
> -	if (!kkey)
> -		return ERR_PTR(-ENOMEM);
> -	if (copy_from_user(kkey, ukey, keylen)) {
> -		kfree(kkey);
> -		return ERR_PTR(-EFAULT);
> -	}
> 
> -	return kkey;
> +	return memdup_user(ukey, keylen);

This part looks good

>  }
> 
>  static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns)
>  {

This part below is not an equivalent replacement. In fact you are fixing a bug here...

> -	void *kapqns = NULL;
> -	size_t nbytes;
> -
> -	if (uapqns && nr_apqns > 0) {
> -		nbytes = nr_apqns * sizeof(struct pkey_apqn);
> -		kapqns = kmalloc(nbytes, GFP_KERNEL);
> -		if (!kapqns)
> -			return ERR_PTR(-ENOMEM);
> -		if (copy_from_user(kapqns, uapqns, nbytes))

	.... here we would need to kfree kapqns, but we do not. So this is
a memory leak. Isnt it?

So indeed this is fixing something. But please rework your the patch 
description accordingly.


> -			return ERR_PTR(-EFAULT);
> -	}


> +	if (!uapqns || nr_apqns <= 0)
> +		return NULL;
> 
> -	return kapqns;
> +	return memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn));
>  }
> 
>  static long pkey_unlocked_ioctl(struct file *filp, unsigned int cmd,
> --
> 2.24.0
> 

Re: [PATCH v2] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

>> Reuse existing functionality from memdup_user() instead of keeping
>> duplicate source code.
>>
>> Generated by: scripts/coccinelle/api/memdup_user.cocci
>>
>> Delete local variables which became unnecessary with this refactoring
>> in two function implementations.
>>
>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>
> With that patch description, the Fixes tag is wrong...but (see below)

I wonder about such a conclusion together with your subsequent feedback.


>>  static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns)
>>  {
>
> This part below is not an equivalent replacement.

The shown refactoring provides also different run time characteristics,
doesn't it?


> In fact you are fixing a bug here...

Thanks for your acknowledgement.


>> -	void *kapqns = NULL;
>> -	size_t nbytes;
>> -
>> -	if (uapqns && nr_apqns > 0) {
>> -		nbytes = nr_apqns * sizeof(struct pkey_apqn);
>> -		kapqns = kmalloc(nbytes, GFP_KERNEL);
>> -		if (!kapqns)
>> -			return ERR_PTR(-ENOMEM);
>> -		if (copy_from_user(kapqns, uapqns, nbytes))
>
> 	.... here we would need to kfree kapqns, but we do not. So this is
> a memory leak. Isnt it?

This is another undesirable software weakness because of incomplete
exception handling in the previous copy approach.


> So indeed this is fixing something. But please rework your the patch
> description accordingly.

Can the final committer pick the opportunity up to extend the change
description another bit?


>> +	if (!uapqns || nr_apqns <= 0)
>> +		return NULL;
>>
>> -	return kapqns;
>> +	return memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn));
>>  }


Would you like to add any tags for the presented software improvement?

Regards,
Markus

Re: [PATCH v2] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 07.11.19 14:45, Markus Elfring wrote:
>>> Reuse existing functionality from memdup_user() instead of keeping
>>> duplicate source code.
>>>
>>> Generated by: scripts/coccinelle/api/memdup_user.cocci
>>>
>>> Delete local variables which became unnecessary with this refactoring
>>> in two function implementations.
>>>
>>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>>
>> With that patch description, the Fixes tag is wrong...but (see below)
> 
> I wonder about such a conclusion together with your subsequent feedback.

Please try to read and understand what other people write. My point was that your
patch description only talks about refactoring and avoiding code duplication.
So you do not claim to have fixed anything. You claim to have refactored things
to avoid code duplication. And no, refactoring is NOT a fix.

That fact that you fix a bug was obviously just by accident. So you have not even
noticed that your change was actually chaning the logical flow of the code. 

Now: When you change the patch description explaining what you fix, a Fixes tag is
appropriate. 

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

>>>> Reuse existing functionality from memdup_user() instead of keeping
>>>> duplicate source code.
>>>>
>>>> Generated by: scripts/coccinelle/api/memdup_user.cocci
>>>>
>>>> Delete local variables which became unnecessary with this refactoring
>>>> in two function implementations.
>>>>
>>>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>>>
>>> With that patch description, the Fixes tag is wrong...but (see below)
>>
>> I wonder about such a conclusion together with your subsequent feedback.
>
> Please try to read and understand what other people write.

I am also trying as usual.


> My point was that your patch description only talks about refactoring
> and avoiding code duplication.

These implementation details are mentioned.


> So you do not claim to have fixed anything.

We have got a different understanding for the provided wording.


> You claim to have refactored things to avoid code duplication.

The reused code can reduce the probability for programming mistakes,
can't it?


> And no, refactoring is NOT a fix.

Software development opinions vary around such a view, don't they?


> That fact that you fix a bug was obviously just by accident.

I can follow this view to some degree.


> So you have not even noticed that your change was actually chaning
> the logical flow of the code.

I suggested to improve two function implementations.


> Now: When you change the patch description explaining what you fix,
> a Fixes tag is appropriate.

Can such a disagreement be resolved by adding the information
to the change description that an incomplete exception handling
(which can trigger a memory leak) should be replaced by hopefully
better functionality?

Regards,
Markus

Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 07.11.19 15:27, Markus Elfring wrote:
>>>>> Reuse existing functionality from memdup_user() instead of keeping
>>>>> duplicate source code.
>>>>>
>>>>> Generated by: scripts/coccinelle/api/memdup_user.cocci
>>>>>
>>>>> Delete local variables which became unnecessary with this refactoring
>>>>> in two function implementations.
>>>>>
>>>>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>>>>
>>>> With that patch description, the Fixes tag is wrong...but (see below)
>>>
>>> I wonder about such a conclusion together with your subsequent feedback.
>>
>> Please try to read and understand what other people write.
> 
> I am also trying as usual.
> 
> 
>> My point was that your patch description only talks about refactoring
>> and avoiding code duplication.
> 
> These implementation details are mentioned.

Exactly and my point is that the main value of your patch is not the refactoring,
but the fact that your refactoring uncovered an existing memory leak. The refactoring
itself is usually not a fix.


So can you just redo the patch with a new patch description ala,

refactoring and reuse. While doing this this also uncovered a real code
bug (memory leak) that is fixed by the refactoring.

And please do that without continue this discussion,

[PATCH v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 8 Nov 2019 17:50:22 +0100

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

Generated by: scripts/coccinelle/api/memdup_user.cocci

* The function "_copy_apqns_from_user" contained a memory leak
  because of a missing function call "kfree(kapqns)" for an if branch.
  Link: https://lore.kernel.org/r/833d7d5e-6ede-6bdd-a2cc-2da7f0b03908@de.ibm.com/

  Thus complete the exception handling by this code replacement.

* Delete local variables which became unnecessary with this refactoring
  in two function implementations.

Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v3:
An adjustment was requested by Christian Bornträger for the change description.
https://lore.kernel.org/r/c0df9cc8-c41a-1e5d-811c-1ff045c13fcc@de.ibm.com/

v2:
Further changes were requested by Joe Perches.
https://lore.kernel.org/r/6137855bb4170c438c7436cbdb7dfd21639a8855.camel@perches.com/

* The proposed usage of two conditional operators was replaced by
  an other code structure.

* A sanity check was adjusted for the function “_copy_apqns_from_user”.


 drivers/s390/crypto/pkey_api.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
index 9de3d46b3253..ac99fd97569d 100644
--- a/drivers/s390/crypto/pkey_api.c
+++ b/drivers/s390/crypto/pkey_api.c
@@ -715,36 +715,18 @@ static int pkey_apqns4keytype(enum pkey_key_type ktype,

 static void *_copy_key_from_user(void __user *ukey, size_t keylen)
 {
-	void *kkey;
-
 	if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE)
 		return ERR_PTR(-EINVAL);
-	kkey = kmalloc(keylen, GFP_KERNEL);
-	if (!kkey)
-		return ERR_PTR(-ENOMEM);
-	if (copy_from_user(kkey, ukey, keylen)) {
-		kfree(kkey);
-		return ERR_PTR(-EFAULT);
-	}

-	return kkey;
+	return memdup_user(ukey, keylen);
 }

 static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns)
 {
-	void *kapqns = NULL;
-	size_t nbytes;
-
-	if (uapqns && nr_apqns > 0) {
-		nbytes = nr_apqns * sizeof(struct pkey_apqn);
-		kapqns = kmalloc(nbytes, GFP_KERNEL);
-		if (!kapqns)
-			return ERR_PTR(-ENOMEM);
-		if (copy_from_user(kapqns, uapqns, nbytes))
-			return ERR_PTR(-EFAULT);
-	}
+	if (!uapqns || nr_apqns <= 0)
+		return NULL;

-	return kapqns;
+	return memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn));
 }

 static long pkey_unlocked_ioctl(struct file *filp, unsigned int cmd,
--
2.24.0

Re: [PATCH v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 08.11.19 18:14, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 8 Nov 2019 17:50:22 +0100
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> Generated by: scripts/coccinelle/api/memdup_user.cocci
> 
> * The function "_copy_apqns_from_user" contained a memory leak
>   because of a missing function call "kfree(kapqns)" for an if branch.
>   Link: https://lore.kernel.org/r/833d7d5e-6ede-6bdd-a2cc-2da7f0b03908@de.ibm.com/
> 
>   Thus complete the exception handling by this code replacement.
> 
> * Delete local variables which became unnecessary with this refactoring
>   in two function implementations.
> 
> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

You were sending this from a different email address. Can you use the same for
the sender and the signoff?



Can you also change the subject to indicate the "fix". e.g. something like

s390/pkey: fix memory leak in error case by using memdup_user() rather than open coding

Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Fri, 8 Nov 2019 17:50:22 +0100
…
>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>
> You were sending this from a different email address.

Yes.


> Can you use the same for the sender and the signoff?

I would prefer to use the other email address (for a while).


> Can you also change the subject to indicate the "fix". e.g. something like
>
> s390/pkey: fix memory leak in error case by using memdup_user() rather than open coding

Does this change request indicate also a need to split the software update
between the discussed two function implementations?

Regards,
Markus

Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 11.11.19 09:11, Markus Elfring wrote:
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> Date: Fri, 8 Nov 2019 17:50:22 +0100
> …
>>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>
>> You were sending this from a different email address.
> 
> Yes.
> 
> 
>> Can you use the same for the sender and the signoff?
> 
> I would prefer to use the other email address (for a while).

The signoff is there to track the patch flow so I prefer that both
addresses match.

>> Can you also change the subject to indicate the "fix". e.g. something like
>>
>> s390/pkey: fix memory leak in error case by using memdup_user() rather than open coding
> 
> Does this change request indicate also a need to split the software update
> between the discussed two function implementations?

No, I just want to have the word "fix" in the subject.

If you are OK with changing the sign-off to your web.de address
I can do both changes when applying.


You can answer with 
Signed-off-by: <your web.de address>

as approval.

Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

> No, I just want to have the word "fix" in the subject.

How do you think about to use the preferred subject in your final commit directly?
(Do you insist on sending a fourth patch variant?)


> If you are OK with changing the sign-off to your web.de address

Does hinder you anything from continuing to use the previous known email address?

Regards,
Markus

Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 11.11.19 09:42, Markus Elfring wrote:
>> No, I just want to have the word "fix" in the subject.
> 
> How do you think about to use the preferred subject in your final commit directly?
> (Do you insist on sending a fourth patch variant?)
> 
> 
>> If you are OK with changing the sign-off to your web.de address
> 
> Does hinder you anything from continuing to use the previous known email address?

Can you at least send a mail from sourceforge address with the Signed-off-by?
The Sign-off is meant to keep track of flow. 

Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

>> Does hinder you anything from continuing to use the previous known email address?
>
> Can you at least send a mail from sourceforge address with the Signed-off-by?

Not any more (for a while).

Message filter systems got difficulties with email redirection services.


> The Sign-off is meant to keep track of flow.

I find the provided tag still fine.

Regards,
Markus

Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 11.11.19 10:06, Markus Elfring wrote:
>>> Does hinder you anything from continuing to use the previous known email address?
>>
>> Can you at least send a mail from sourceforge address with the Signed-off-by?
> 
> Not any more (for a while).

Then I will not apply that patch with that email and signoff.

Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

>>>> Does hinder you anything from continuing to use the previous known email address?
>>>
>>> Can you at least send a mail from sourceforge address with the Signed-off-by?
>>
>> Not any more (for a while).
>
> Then I will not apply that patch with that email and signoff.

Why do you find the information “Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>” inappropriate
(at the moment)?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n418

Regards,
Markus

Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Christian Borntraeger]

On 11.11.19 10:17, Markus Elfring wrote:
>>>>> Does hinder you anything from continuing to use the previous known email address?
>>>>
>>>> Can you at least send a mail from sourceforge address with the Signed-off-by?
>>>
>>> Not any more (for a while).
>>
>> Then I will not apply that patch with that email and signoff.
> 
> Why do you find the information “Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>” inappropriate
> (at the moment)?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n418

Because I am not going to add this email as the author of the patch when you are not able
to send emails from that address.

Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation

[Markus Elfring]

>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n418
>
> Because I am not going to add this email as the author of the patch when you are not able
> to send emails from that address.

I find it more important that messages can still be sent also to
the other known email address.
How often would you like to switch addresses at this place?

Regards,
Markus

[PATCH v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding

[Markus Elfring]

Date: Mon, 11 Nov 2019 15:20:44 +0100

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

Generated by: scripts/coccinelle/api/memdup_user.cocci

* The function "_copy_apqns_from_user" contained a memory leak
  because of a misssing function call "kfree(kapqns)" for an if branch.
  Link: https://lore.kernel.org/r/833d7d5e-6ede-6bdd-a2cc-2da7f0b03908@de.ibm.com/

  Thus complete the exception handling by this code replacement.

* Delete local variables which became unnecessary with this refactoring
  in two function implementations.

Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
Signed-off-by: Markus Elfring <Markus.Elfring@web.de>
---

v4:
Further changes were requested by Christian Bornträger.
https://lore.kernel.org/r/040f3e18-d97a-fc32-b237-20e7553e1733@de.ibm.com/

* An other patch subject was selected.

* An other email address was used for the tag “Signed-off-by” this time.

v3:
An adjustment was requested by Christian Bornträger for the change description.
https://lore.kernel.org/r/c0df9cc8-c41a-1e5d-811c-1ff045c13fcc@de.ibm.com/

v2:
Further changes were requested by Joe Perches.
https://lore.kernel.org/r/6137855bb4170c438c7436cbdb7dfd21639a8855.camel@perches.com/

* The proposed usage of two conditional operators was replaced by
  an other code structure.

* A sanity check was adjusted for the function “_copy_apqns_from_user”.


 drivers/s390/crypto/pkey_api.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
index 9de3d46b3253..ac99fd97569d 100644
--- a/drivers/s390/crypto/pkey_api.c
+++ b/drivers/s390/crypto/pkey_api.c
@@ -715,36 +715,18 @@ static int pkey_apqns4keytype(enum pkey_key_type ktype,

 static void *_copy_key_from_user(void __user *ukey, size_t keylen)
 {
-	void *kkey;
-
 	if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE)
 		return ERR_PTR(-EINVAL);
-	kkey = kmalloc(keylen, GFP_KERNEL);
-	if (!kkey)
-		return ERR_PTR(-ENOMEM);
-	if (copy_from_user(kkey, ukey, keylen)) {
-		kfree(kkey);
-		return ERR_PTR(-EFAULT);
-	}

-	return kkey;
+	return memdup_user(ukey, keylen);
 }

 static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns)
 {
-	void *kapqns = NULL;
-	size_t nbytes;
-
-	if (uapqns && nr_apqns > 0) {
-		nbytes = nr_apqns * sizeof(struct pkey_apqn);
-		kapqns = kmalloc(nbytes, GFP_KERNEL);
-		if (!kapqns)
-			return ERR_PTR(-ENOMEM);
-		if (copy_from_user(kapqns, uapqns, nbytes))
-			return ERR_PTR(-EFAULT);
-	}
+	if (!uapqns || nr_apqns <= 0)
+		return NULL;

-	return kapqns;
+	return memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn));
 }

 static long pkey_unlocked_ioctl(struct file *filp, unsigned int cmd,
--
2.24.0

Re: [PATCH v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding

[Christian Borntraeger]

On 11.11.19 15:45, Markus Elfring wrote:
> Date: Mon, 11 Nov 2019 15:20:44 +0100
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> Generated by: scripts/coccinelle/api/memdup_user.cocci
> 
> * The function "_copy_apqns_from_user" contained a memory leak
>   because of a misssing function call "kfree(kapqns)" for an if branch.
>   Link: https://lore.kernel.org/r/833d7d5e-6ede-6bdd-a2cc-2da7f0b03908@de.ibm.com/
> 
>   Thus complete the exception handling by this code replacement.
> 
> * Delete local variables which became unnecessary with this refactoring
>   in two function implementations.
> 
> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
> Signed-off-by: Markus Elfring <Markus.Elfring@web.de>
> ---
> 
> v4:
> Further changes were requested by Christian Bornträger.
> https://lore.kernel.org/r/040f3e18-d97a-fc32-b237-20e7553e1733@de.ibm.com/
> 
> * An other patch subject was selected.
> 
> * An other email address was used for the tag “Signed-off-by” this time.

applied. [...]

> +	if (!uapqns || nr_apqns <= 0)

While applying I changed that to 
	if (!uapqns || nr_apqns == 0)

as nr_apqns is size_t and thus unsigned. 

Re: [v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding

[Markus Elfring]

> applied. [...]

I find it interesting how the commit message was changed once more.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=8b57e7c852fc58a62e668a83c0fa8d9246131803
("s390/pkey: use memdup_user() to simplify code")

Regards,
Markus