diff mbox series

[v3,3/3] KEYS: Use kvmalloc() to better handle large buffer allocation

Message ID 20200313152102.1707-4-longman@redhat.com (mailing list archive)
State New
Headers show
Series KEYS: Read keys to internal buffer & then copy to userspace | expand

Commit Message

Waiman Long March 13, 2020, 3:21 p.m. UTC
For large multi-page temporary buffer allocation, the security/keys
subsystem don't need contiguous physical pages. It will work perfectly
fine with virtually mapped pages.

Replace the kmalloc() call by kvmalloc() and provide a __kvzfree()
helper function to clear and free the kvmalloc'ed buffer. This will
reduce the chance of memory allocation failure just because of highly
fragmented pages.

Suggested-by: David Howells <dhowells@redhat.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 security/keys/internal.h | 14 ++++++++++++++
 security/keys/keyctl.c   | 10 +++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

Comments

Eric Biggers March 13, 2020, 4:43 p.m. UTC | #1
On Fri, Mar 13, 2020 at 11:21:02AM -0400, Waiman Long wrote:
> For large multi-page temporary buffer allocation, the security/keys
> subsystem don't need contiguous physical pages. It will work perfectly
> fine with virtually mapped pages.
> 
> Replace the kmalloc() call by kvmalloc() and provide a __kvzfree()
> helper function to clear and free the kvmalloc'ed buffer. This will
> reduce the chance of memory allocation failure just because of highly
> fragmented pages.
> 
> Suggested-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  security/keys/internal.h | 14 ++++++++++++++
>  security/keys/keyctl.c   | 10 +++++-----
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index ba3e2da14cef..855b11eb73ee 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -16,6 +16,8 @@
>  #include <linux/keyctl.h>
>  #include <linux/refcount.h>
>  #include <linux/compat.h>
> +#include <linux/mm.h>
> +#include <linux/vmalloc.h>
>  
>  struct iovec;
>  
> @@ -349,4 +351,16 @@ static inline void key_check(const struct key *key)
>  
>  #endif
>  
> +/*
> + * Helper function to clear and free a kvmalloc'ed memory object.
> + */
> +static inline void __kvzfree(const void *addr, size_t len)
> +{
> +	if (is_vmalloc_addr(addr)) {
> +		memset((void *)addr, 0, len);
> +		vfree(addr);
> +	} else {
> +		kzfree(addr);
> +	}
> +}

Since this takes the length as a parameter, it can be simplified to:

static inline void __kvzfree(const void *addr, size_t len)
{
	if (addr) {
		memset((void *)addr, 0, len);
		kvfree(addr);
	}
}

>  			if (!tmpbuf || unlikely(ret > tmpbuflen)) {
>  				if (unlikely(tmpbuf))
> -					kzfree(tmpbuf);
> +					__kvzfree(tmpbuf, tmpbuflen);

Both kzfree() and __kvzfree() handle a NULL pointer, so there's no need for the
NULL check first.

> @@ -920,7 +920,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  				ret = -EFAULT;
>  		}
>  		if (tmpbuf)
> -			kzfree(tmpbuf);
> +			__kvzfree(tmpbuf, tmpbuflen);

Likewise here.  No need for the NULL check.

- Eric
Waiman Long March 13, 2020, 5:49 p.m. UTC | #2
On 3/13/20 12:43 PM, Eric Biggers wrote:
> On Fri, Mar 13, 2020 at 11:21:02AM -0400, Waiman Long wrote:
>> For large multi-page temporary buffer allocation, the security/keys
>> subsystem don't need contiguous physical pages. It will work perfectly
>> fine with virtually mapped pages.
>>
>> Replace the kmalloc() call by kvmalloc() and provide a __kvzfree()
>> helper function to clear and free the kvmalloc'ed buffer. This will
>> reduce the chance of memory allocation failure just because of highly
>> fragmented pages.
>>
>> Suggested-by: David Howells <dhowells@redhat.com>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>  security/keys/internal.h | 14 ++++++++++++++
>>  security/keys/keyctl.c   | 10 +++++-----
>>  2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/security/keys/internal.h b/security/keys/internal.h
>> index ba3e2da14cef..855b11eb73ee 100644
>> --- a/security/keys/internal.h
>> +++ b/security/keys/internal.h
>> @@ -16,6 +16,8 @@
>>  #include <linux/keyctl.h>
>>  #include <linux/refcount.h>
>>  #include <linux/compat.h>
>> +#include <linux/mm.h>
>> +#include <linux/vmalloc.h>
>>  
>>  struct iovec;
>>  
>> @@ -349,4 +351,16 @@ static inline void key_check(const struct key *key)
>>  
>>  #endif
>>  
>> +/*
>> + * Helper function to clear and free a kvmalloc'ed memory object.
>> + */
>> +static inline void __kvzfree(const void *addr, size_t len)
>> +{
>> +	if (is_vmalloc_addr(addr)) {
>> +		memset((void *)addr, 0, len);
>> +		vfree(addr);
>> +	} else {
>> +		kzfree(addr);
>> +	}
>> +}
> Since this takes the length as a parameter, it can be simplified to:
>
> static inline void __kvzfree(const void *addr, size_t len)
> {
> 	if (addr) {
> 		memset((void *)addr, 0, len);
> 		kvfree(addr);
> 	}
> }
Yes, that will work too.
>>  			if (!tmpbuf || unlikely(ret > tmpbuflen)) {
>>  				if (unlikely(tmpbuf))
>> -					kzfree(tmpbuf);
>> +					__kvzfree(tmpbuf, tmpbuflen);
> Both kzfree() and __kvzfree() handle a NULL pointer, so there's no need for the
> NULL check first.
>
I would like to keep this one because of the unlikely annotation.


>> @@ -920,7 +920,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>>  				ret = -EFAULT;
>>  		}
>>  		if (tmpbuf)
>> -			kzfree(tmpbuf);
>> +			__kvzfree(tmpbuf, tmpbuflen);
> Likewise here.  No need for the NULL check.

Yes, that tmpbuf check is not really necessary, but it doesn't harm either.

My plan is to send out a mm patch to officially add the kvzfree()
function to mm/util.c. I will remove the tmpbuf check at that time if
you don't mind.

Cheers,
Longman
Jarkko Sakkinen March 15, 2020, 9:52 p.m. UTC | #3
On Fri, Mar 13, 2020 at 01:49:57PM -0400, Waiman Long wrote:
> >>  			if (!tmpbuf || unlikely(ret > tmpbuflen)) {
> >>  				if (unlikely(tmpbuf))
> >> -					kzfree(tmpbuf);
> >> +					__kvzfree(tmpbuf, tmpbuflen);
> > Both kzfree() and __kvzfree() handle a NULL pointer, so there's no need for the
> > NULL check first.
> >
> I would like to keep this one because of the unlikely annotation.

What (measurable) gain does it bring anyway?

/Jarkko
Waiman Long March 15, 2020, 10:01 p.m. UTC | #4
On 3/15/20 5:52 PM, Jarkko Sakkinen wrote:
> On Fri, Mar 13, 2020 at 01:49:57PM -0400, Waiman Long wrote:
>>>>  			if (!tmpbuf || unlikely(ret > tmpbuflen)) {
>>>>  				if (unlikely(tmpbuf))
>>>> -					kzfree(tmpbuf);
>>>> +					__kvzfree(tmpbuf, tmpbuflen);
>>> Both kzfree() and __kvzfree() handle a NULL pointer, so there's no need for the
>>> NULL check first.
>>>
>> I would like to keep this one because of the unlikely annotation.
> What (measurable) gain does it bring anyway?

It is not a performance issue. I just want to indicate that the need to
free should not happen at all. It match the unlikely tag in the if
condition above.

Cheers,
Longman
David Howells March 16, 2020, 2:24 p.m. UTC | #5
I wonder if it's worth merging this into patch 2.  I'm not sure it's really
worth its own patch.  If you want to generalise kvzfree(), then that could go
as its own patch first.

David
Waiman Long March 16, 2020, 3:21 p.m. UTC | #6
On 3/16/20 10:24 AM, David Howells wrote:
> I wonder if it's worth merging this into patch 2.  I'm not sure it's really
> worth its own patch.  If you want to generalise kvzfree(), then that could go
> as its own patch first.
>
> David

Sure, I can merge it into patch 2.

Thanks,
Longman
Jarkko Sakkinen March 16, 2020, 10:19 p.m. UTC | #7
On Mon, 2020-03-16 at 14:24 +0000, David Howells wrote:
> I wonder if it's worth merging this into patch 2.  I'm not sure it's really
> worth its own patch.  If you want to generalise kvzfree(), then that could go
> as its own patch first.
> 
> David

Also in the sense that there is no senseful situation where you'd
only wanted to pick either but not both.

/Jarkko
diff mbox series

Patch

diff --git a/security/keys/internal.h b/security/keys/internal.h
index ba3e2da14cef..855b11eb73ee 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -16,6 +16,8 @@ 
 #include <linux/keyctl.h>
 #include <linux/refcount.h>
 #include <linux/compat.h>
+#include <linux/mm.h>
+#include <linux/vmalloc.h>
 
 struct iovec;
 
@@ -349,4 +351,16 @@  static inline void key_check(const struct key *key)
 
 #endif
 
+/*
+ * Helper function to clear and free a kvmalloc'ed memory object.
+ */
+static inline void __kvzfree(const void *addr, size_t len)
+{
+	if (is_vmalloc_addr(addr)) {
+		memset((void *)addr, 0, len);
+		vfree(addr);
+	} else {
+		kzfree(addr);
+	}
+}
 #endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index a05a4dd2f9ce..878259cf35d5 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -339,7 +339,7 @@  long keyctl_update_key(key_serial_t id,
 	payload = NULL;
 	if (plen) {
 		ret = -ENOMEM;
-		payload = kmalloc(plen, GFP_KERNEL);
+		payload = kvmalloc(plen, GFP_KERNEL);
 		if (!payload)
 			goto error;
 
@@ -360,7 +360,7 @@  long keyctl_update_key(key_serial_t id,
 
 	key_ref_put(key_ref);
 error2:
-	kzfree(payload);
+	__kvzfree(payload, plen);
 error:
 	return ret;
 }
@@ -893,7 +893,7 @@  long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 		 */
 		if (buflen <= 0x400) {
 allocbuf:
-			tmpbuf = kmalloc(tmpbuflen, GFP_KERNEL);
+			tmpbuf = kvmalloc(tmpbuflen, GFP_KERNEL);
 			if (!tmpbuf) {
 				ret = -ENOMEM;
 				goto error2;
@@ -911,7 +911,7 @@  long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 			 */
 			if (!tmpbuf || unlikely(ret > tmpbuflen)) {
 				if (unlikely(tmpbuf))
-					kzfree(tmpbuf);
+					__kvzfree(tmpbuf, tmpbuflen);
 				tmpbuflen = ret;
 				goto allocbuf;
 			}
@@ -920,7 +920,7 @@  long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 				ret = -EFAULT;
 		}
 		if (tmpbuf)
-			kzfree(tmpbuf);
+			__kvzfree(tmpbuf, tmpbuflen);
 	}
 
 error2: