linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] s390/pkey: Use memdup_user() rather than duplicating its implementation
@ 2019-11-06 10:22 Markus Elfring
  2019-11-06 10:38 ` Joe Perches
  0 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-06 10:22 UTC (permalink / raw)
  To: linux-s390, Christian Bornträger, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik
  Cc: LKML, kernel-janitors

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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 10:22 [PATCH] s390/pkey: Use memdup_user() rather than duplicating its implementation Markus Elfring
@ 2019-11-06 10:38 ` Joe Perches
  2019-11-06 13:00   ` Markus Elfring
                     ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Joe Perches @ 2019-11-06 10:38 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Christian Bornträger,
	Harald Freudenberger, Heiko Carstens, Ingo Franzki,
	Vasily Gorbik
  Cc: LKML, kernel-janitors

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



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 10:38 ` Joe Perches
  2019-11-06 13:00   ` Markus Elfring
@ 2019-11-06 13:00   ` Markus Elfring
  2019-11-06 18:30     ` Christian Borntraeger
  2019-11-07 10:06   ` [PATCH v2] " Markus Elfring
  2 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-06 13:00 UTC (permalink / raw)
  To: Joe Perches, linux-s390, kernel-janitors
  Cc: Christian Bornträger, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, LKML

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 10:38 ` Joe Perches
@ 2019-11-06 13:00   ` Markus Elfring
  2019-11-06 17:29     ` Joe Perches
  2019-11-06 13:00   ` Markus Elfring
  2019-11-07 10:06   ` [PATCH v2] " Markus Elfring
  2 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-06 13:00 UTC (permalink / raw)
  To: Joe Perches, linux-s390, kernel-janitors
  Cc: Christian Bornträger, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, LKML

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 13:00   ` Markus Elfring
@ 2019-11-06 17:29     ` Joe Perches
  2019-11-06 18:55       ` Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Joe Perches @ 2019-11-06 17:29 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, kernel-janitors
  Cc: Christian Bornträger, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, LKML

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.



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 13:00   ` Markus Elfring
@ 2019-11-06 18:30     ` Christian Borntraeger
  2019-11-07  6:48       ` Dan Carpenter
  0 siblings, 1 reply; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-06 18:30 UTC (permalink / raw)
  To: Markus Elfring, Joe Perches, linux-s390, kernel-janitors
  Cc: Harald Freudenberger, Heiko Carstens, Ingo Franzki, Vasily Gorbik, LKML



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. 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 17:29     ` Joe Perches
@ 2019-11-06 18:55       ` Markus Elfring
  2019-11-06 19:01         ` Joe Perches
  0 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-06 18:55 UTC (permalink / raw)
  To: Joe Perches, linux-s390, kernel-janitors
  Cc: Christian Bornträger, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, LKML

> There is no bug here.

Do you find duplicated source code questionable?

Is this also an error item?

Regards,
Markus

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 18:55       ` Markus Elfring
@ 2019-11-06 19:01         ` Joe Perches
  2019-11-06 19:18           ` Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Joe Perches @ 2019-11-06 19:01 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, kernel-janitors
  Cc: Christian Bornträger, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, LKML

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.



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 19:01         ` Joe Perches
@ 2019-11-06 19:18           ` Markus Elfring
  0 siblings, 0 replies; 31+ messages in thread
From: Markus Elfring @ 2019-11-06 19:18 UTC (permalink / raw)
  To: Joe Perches, linux-s390, kernel-janitors
  Cc: Christian Bornträger, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, LKML

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 18:30     ` Christian Borntraeger
@ 2019-11-07  6:48       ` Dan Carpenter
  2019-11-07  8:07         ` Christian Borntraeger
  0 siblings, 1 reply; 31+ messages in thread
From: Dan Carpenter @ 2019-11-07  6:48 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Markus Elfring, Joe Perches, linux-s390, kernel-janitors,
	Harald Freudenberger, Heiko Carstens, Ingo Franzki,
	Vasily Gorbik, LKML

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-07  6:48       ` Dan Carpenter
@ 2019-11-07  8:07         ` Christian Borntraeger
  0 siblings, 0 replies; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-07  8:07 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Markus Elfring, Joe Perches, linux-s390, kernel-janitors,
	Harald Freudenberger, Heiko Carstens, Ingo Franzki,
	Vasily Gorbik, LKML



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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-06 10:38 ` Joe Perches
  2019-11-06 13:00   ` Markus Elfring
  2019-11-06 13:00   ` Markus Elfring
@ 2019-11-07 10:06   ` Markus Elfring
  2019-11-07 12:44     ` Christian Borntraeger
  2 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-07 10:06 UTC (permalink / raw)
  To: linux-s390, Joe Perches, Christian Bornträger,
	Harald Freudenberger, Heiko Carstens, Ingo Franzki,
	Vasily Gorbik
  Cc: LKML, kernel-janitors

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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v2] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-07 10:06   ` [PATCH v2] " Markus Elfring
@ 2019-11-07 12:44     ` Christian Borntraeger
  2019-11-07 13:45       ` Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-07 12:44 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Joe Perches, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik
  Cc: LKML, kernel-janitors



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
> 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-07 12:44     ` Christian Borntraeger
@ 2019-11-07 13:45       ` Markus Elfring
  2019-11-07 13:54         ` Christian Borntraeger
  0 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-07 13:45 UTC (permalink / raw)
  To: Christian Bornträger, linux-s390, Joe Perches,
	Harald Freudenberger, Heiko Carstens, Ingo Franzki,
	Vasily Gorbik
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-07 13:45       ` Markus Elfring
@ 2019-11-07 13:54         ` Christian Borntraeger
  2019-11-07 14:27           ` Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-07 13:54 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Joe Perches, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant



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. 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-07 13:54         ` Christian Borntraeger
@ 2019-11-07 14:27           ` Markus Elfring
  2019-11-08 11:32             ` Christian Borntraeger
  0 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-07 14:27 UTC (permalink / raw)
  To: Christian Bornträger, linux-s390, Joe Perches,
	Harald Freudenberger, Heiko Carstens, Ingo Franzki,
	Vasily Gorbik
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-07 14:27           ` Markus Elfring
@ 2019-11-08 11:32             ` Christian Borntraeger
  2019-11-08 17:14               ` [PATCH v3] " Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-08 11:32 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Joe Perches, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant



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,


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-08 11:32             ` Christian Borntraeger
@ 2019-11-08 17:14               ` Markus Elfring
  2019-11-11  7:54                 ` Christian Borntraeger
  0 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-08 17:14 UTC (permalink / raw)
  To: linux-s390, Christian Bornträger, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-08 17:14               ` [PATCH v3] " Markus Elfring
@ 2019-11-11  7:54                 ` Christian Borntraeger
  2019-11-11  8:11                   ` [v3] " Markus Elfring
  2019-11-11 14:45                   ` [PATCH v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding Markus Elfring
  0 siblings, 2 replies; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-11  7:54 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-11  7:54                 ` Christian Borntraeger
@ 2019-11-11  8:11                   ` Markus Elfring
  2019-11-11  8:27                     ` Christian Borntraeger
  2019-11-11 14:45                   ` [PATCH v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding Markus Elfring
  1 sibling, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-11  8:11 UTC (permalink / raw)
  To: Christian Bornträger, linux-s390, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-11  8:11                   ` [v3] " Markus Elfring
@ 2019-11-11  8:27                     ` Christian Borntraeger
  2019-11-11  8:42                       ` Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-11  8:27 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant



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.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-11  8:27                     ` Christian Borntraeger
@ 2019-11-11  8:42                       ` Markus Elfring
  2019-11-11  8:56                         ` Christian Borntraeger
  0 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-11  8:42 UTC (permalink / raw)
  To: Christian Bornträger, linux-s390, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

> 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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-11  8:42                       ` Markus Elfring
@ 2019-11-11  8:56                         ` Christian Borntraeger
  2019-11-11  9:06                           ` Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-11  8:56 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant



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. 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-11  8:56                         ` Christian Borntraeger
@ 2019-11-11  9:06                           ` Markus Elfring
  2019-11-11  9:08                             ` Christian Borntraeger
  0 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-11  9:06 UTC (permalink / raw)
  To: Christian Bornträger, linux-s390, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-11  9:06                           ` Markus Elfring
@ 2019-11-11  9:08                             ` Christian Borntraeger
  2019-11-11  9:17                               ` Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-11  9:08 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant



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.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-11  9:08                             ` Christian Borntraeger
@ 2019-11-11  9:17                               ` Markus Elfring
  2019-11-11  9:18                                 ` Christian Borntraeger
  0 siblings, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-11  9:17 UTC (permalink / raw)
  To: Christian Bornträger, linux-s390, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-11  9:17                               ` Markus Elfring
@ 2019-11-11  9:18                                 ` Christian Borntraeger
  2019-11-11  9:26                                   ` Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-11  9:18 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant



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.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v3] s390/pkey: Use memdup_user() rather than duplicating its implementation
  2019-11-11  9:18                                 ` Christian Borntraeger
@ 2019-11-11  9:26                                   ` Markus Elfring
  0 siblings, 0 replies; 31+ messages in thread
From: Markus Elfring @ 2019-11-11  9:26 UTC (permalink / raw)
  To: Christian Bornträger, linux-s390, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding
  2019-11-11  7:54                 ` Christian Borntraeger
  2019-11-11  8:11                   ` [v3] " Markus Elfring
@ 2019-11-11 14:45                   ` Markus Elfring
  2019-11-11 16:40                     ` Christian Borntraeger
  1 sibling, 1 reply; 31+ messages in thread
From: Markus Elfring @ 2019-11-11 14:45 UTC (permalink / raw)
  To: linux-s390, Christian Bornträger, Harald Freudenberger,
	Heiko Carstens, Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant

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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding
  2019-11-11 14:45                   ` [PATCH v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding Markus Elfring
@ 2019-11-11 16:40                     ` Christian Borntraeger
  2019-11-13 17:09                       ` [v4] " Markus Elfring
  0 siblings, 1 reply; 31+ messages in thread
From: Christian Borntraeger @ 2019-11-11 16:40 UTC (permalink / raw)
  To: Markus Elfring, linux-s390, Harald Freudenberger, Heiko Carstens,
	Ingo Franzki, Vasily Gorbik, Joe Perches
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost, Stephen McCamant


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. 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding
  2019-11-11 16:40                     ` Christian Borntraeger
@ 2019-11-13 17:09                       ` Markus Elfring
  0 siblings, 0 replies; 31+ messages in thread
From: Markus Elfring @ 2019-11-13 17:09 UTC (permalink / raw)
  To: linux-s390, Christian Bornträger, Heiko Carstens, Vasily Gorbik
  Cc: LKML, kernel-janitors, Kangjie Lu, Navid Emamdoost,
	Stephen McCamant, Harald Freudenberger, Ingo Franzki,
	Joe Perches

> 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

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2019-11-13 17:09 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06 10:22 [PATCH] s390/pkey: Use memdup_user() rather than duplicating its implementation Markus Elfring
2019-11-06 10:38 ` Joe Perches
2019-11-06 13:00   ` Markus Elfring
2019-11-06 17:29     ` Joe Perches
2019-11-06 18:55       ` Markus Elfring
2019-11-06 19:01         ` Joe Perches
2019-11-06 19:18           ` Markus Elfring
2019-11-06 13:00   ` Markus Elfring
2019-11-06 18:30     ` Christian Borntraeger
2019-11-07  6:48       ` Dan Carpenter
2019-11-07  8:07         ` Christian Borntraeger
2019-11-07 10:06   ` [PATCH v2] " Markus Elfring
2019-11-07 12:44     ` Christian Borntraeger
2019-11-07 13:45       ` Markus Elfring
2019-11-07 13:54         ` Christian Borntraeger
2019-11-07 14:27           ` Markus Elfring
2019-11-08 11:32             ` Christian Borntraeger
2019-11-08 17:14               ` [PATCH v3] " Markus Elfring
2019-11-11  7:54                 ` Christian Borntraeger
2019-11-11  8:11                   ` [v3] " Markus Elfring
2019-11-11  8:27                     ` Christian Borntraeger
2019-11-11  8:42                       ` Markus Elfring
2019-11-11  8:56                         ` Christian Borntraeger
2019-11-11  9:06                           ` Markus Elfring
2019-11-11  9:08                             ` Christian Borntraeger
2019-11-11  9:17                               ` Markus Elfring
2019-11-11  9:18                                 ` Christian Borntraeger
2019-11-11  9:26                                   ` Markus Elfring
2019-11-11 14:45                   ` [PATCH v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding Markus Elfring
2019-11-11 16:40                     ` Christian Borntraeger
2019-11-13 17:09                       ` [v4] " Markus Elfring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).