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