I notice there are many places doing copy_from_user() which follows
kmalloc():
dst = kmalloc(len, GFP_KERNEL);
if (!dst)
return -ENOMEM;
if (copy_from_user(dst, src, len)) {
kfree(dst);
return -EFAULT
}
kmemdup_from_user() is a wrapper of the above code. With this new
function, we don't have to write 'len' twice, which can lead to
typos/mistakes. It also produces smaller code.
A qucik grep shows 250+ places where kmemdup_from_user() *may* be
used. I'll prepare a patchset to do this conversion.
Signed-off-by: Li Zefan <[email protected]>
---
include/linux/string.h | 1 +
mm/util.c | 24 ++++++++++++++++++++++++
2 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index 76ec218..397e622 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -105,6 +105,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
extern char *kstrdup(const char *s, gfp_t gfp);
extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
extern void *kmemdup(const void *src, size_t len, gfp_t gfp);
+extern void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp);
extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
extern void argv_free(char **argv);
diff --git a/mm/util.c b/mm/util.c
index 37eaccd..a608ebb 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -70,6 +70,30 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
EXPORT_SYMBOL(kmemdup);
/**
+ * kmemdup_from_user - duplicate memory region from user space
+ *
+ * @src: source address in user space
+ * @len: number of bytes to copy
+ * @gfp: GFP mask to use
+ */
+void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp)
+{
+ void *p;
+
+ p = kmalloc_track_caller(len, gfp);
+ if (!p)
+ return ERR_PTR(-ENOMEM);
+
+ if (copy_from_user(p, src, len)) {
+ kfree(p);
+ return ERR_PTR(-EFAULT);
+ }
+
+ return p;
+}
+EXPORT_SYMBOL(kmemdup_from_user);
+
+/**
* __krealloc - like krealloc() but don't free @p.
* @p: object to reallocate memory for.
* @new_size: how many bytes of memory are required.
--
1.5.4.rc3
On Fri, Mar 06, 2009 at 03:04:12PM +0800, Li Zefan wrote:
>I notice there are many places doing copy_from_user() which follows
>kmalloc():
>
> dst = kmalloc(len, GFP_KERNEL);
> if (!dst)
> return -ENOMEM;
> if (copy_from_user(dst, src, len)) {
> kfree(dst);
> return -EFAULT
> }
>
>kmemdup_from_user() is a wrapper of the above code. With this new
>function, we don't have to write 'len' twice, which can lead to
>typos/mistakes. It also produces smaller code.
>
>A qucik grep shows 250+ places where kmemdup_from_user() *may* be
>used. I'll prepare a patchset to do this conversion.
>
>Signed-off-by: Li Zefan <[email protected]>
>---
> include/linux/string.h | 1 +
> mm/util.c | 24 ++++++++++++++++++++++++
> 2 files changed, 25 insertions(+), 0 deletions(-)
>
>diff --git a/include/linux/string.h b/include/linux/string.h
>index 76ec218..397e622 100644
>--- a/include/linux/string.h
>+++ b/include/linux/string.h
>@@ -105,6 +105,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
> extern char *kstrdup(const char *s, gfp_t gfp);
> extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
> extern void *kmemdup(const void *src, size_t len, gfp_t gfp);
>+extern void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp);
>
> extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
> extern void argv_free(char **argv);
>diff --git a/mm/util.c b/mm/util.c
>index 37eaccd..a608ebb 100644
>--- a/mm/util.c
>+++ b/mm/util.c
>@@ -70,6 +70,30 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
> EXPORT_SYMBOL(kmemdup);
>
> /**
>+ * kmemdup_from_user - duplicate memory region from user space
>+ *
>+ * @src: source address in user space
>+ * @len: number of bytes to copy
>+ * @gfp: GFP mask to use
>+ */
>+void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp)
>+{
>+ void *p;
>+
>+ p = kmalloc_track_caller(len, gfp);
Well, you use kmalloc_track_caller, instead of kmalloc as you showed
above. :) Why don't you mention this?
>+ if (!p)
>+ return ERR_PTR(-ENOMEM);
>+
>+ if (copy_from_user(p, src, len)) {
>+ kfree(p);
>+ return ERR_PTR(-EFAULT);
>+ }
>+
>+ return p;
>+}
>+EXPORT_SYMBOL(kmemdup_from_user);
>+
>+/**
> * __krealloc - like krealloc() but don't free @p.
> * @p: object to reallocate memory for.
> * @new_size: how many bytes of memory are required.
>--
>1.5.4.rc3
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at http://www.tux.org/lkml/
--
Do what you love, f**k the rest! F**k the regulations!
> > /**
> >+ * kmemdup_from_user - duplicate memory region from user space
> >+ *
> >+ * @src: source address in user space
> >+ * @len: number of bytes to copy
> >+ * @gfp: GFP mask to use
> >+ */
> >+void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp)
> >+{
> >+ void *p;
> >+
> >+ p = kmalloc_track_caller(len, gfp);
>
>
> Well, you use kmalloc_track_caller, instead of kmalloc as you showed
> above. :) Why don't you mention this?
kmalloc() wrapper function must use kmalloc_track_caller().
his code is right.
if not, kmalloc tracking feature is breaked.
>> +void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp)
>> +{
>> + void *p;
>> +
>> + p = kmalloc_track_caller(len, gfp);
>
> Well, you use kmalloc_track_caller, instead of kmalloc as you showed
> above. :) Why don't you mention this?
>
Because this changelog is not going to explain what kmalloc_track_caller()
is used for, which has been explained in linux/slab.h ..
(cc to Davi Arnaut)
> > > /**
> > >+ * kmemdup_from_user - duplicate memory region from user space
> > >+ *
> > >+ * @src: source address in user space
> > >+ * @len: number of bytes to copy
> > >+ * @gfp: GFP mask to use
> > >+ */
> > >+void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp)
> > >+{
> > >+ void *p;
> > >+
> > >+ p = kmalloc_track_caller(len, gfp);
> >
> >
> > Well, you use kmalloc_track_caller, instead of kmalloc as you showed
> > above. :) Why don't you mention this?
>
> kmalloc() wrapper function must use kmalloc_track_caller().
I find another kmalloc() usage in the same file.
Davi, Can you agree following patch?
==
Subject: [PATCH] Don't use kmalloc() in strndup_user(). instead, use kmalloc_track_caller().
kmalloc() wrapper function should use kmalloc_track_caller() instead
kmalloc().
slab.h talk about the reason.
/*
* kmalloc_track_caller is a special version of kmalloc that records the
* calling function of the routine calling it for slab leak tracking instead
* of just the calling function (confusing, eh?).
* It's useful when the call to kmalloc comes from a widely-used standard
* allocator where we care about the real place the memory allocation
* request comes from.
*/
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/util.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/mm/util.c b/mm/util.c
index 37eaccd..202da19 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -167,7 +167,7 @@ char *strndup_user(const char __user *s, long n)
if (length > n)
return ERR_PTR(-EINVAL);
- p = kmalloc(length, GFP_KERNEL);
+ p = kmalloc_track_caller(length, GFP_KERNEL);
if (!p)
return ERR_PTR(-ENOMEM);
--
1.6.1.2
On Fri, Mar 06, 2009 at 03:04:12PM +0800, Li Zefan wrote:
> I notice there are many places doing copy_from_user() which follows
> kmalloc():
>
> dst = kmalloc(len, GFP_KERNEL);
> if (!dst)
> return -ENOMEM;
> if (copy_from_user(dst, src, len)) {
> kfree(dst);
> return -EFAULT
> }
>
> kmemdup_from_user() is a wrapper of the above code. With this new
> function, we don't have to write 'len' twice, which can lead to
> typos/mistakes. It also produces smaller code.
Name totally sucks, it mixes kernel idiom of allocation with purely
userspace function.
> A qucik grep shows 250+ places where kmemdup_from_user() *may* be
> used. I'll prepare a patchset to do this conversion.
250?
Let's not add wrapper for every two lines that happen to be used
together.
BTW, can we drop strstarts() and kzfree() on the same reasoning?
Alexey Dobriyan wrote:
> On Fri, Mar 06, 2009 at 03:04:12PM +0800, Li Zefan wrote:
>> I notice there are many places doing copy_from_user() which follows
>> kmalloc():
>>
>> dst = kmalloc(len, GFP_KERNEL);
>> if (!dst)
>> return -ENOMEM;
>> if (copy_from_user(dst, src, len)) {
>> kfree(dst);
>> return -EFAULT
>> }
>>
>> kmemdup_from_user() is a wrapper of the above code. With this new
>> function, we don't have to write 'len' twice, which can lead to
>> typos/mistakes. It also produces smaller code.
>
> Name totally sucks, it mixes kernel idiom of allocation with purely
> userspace function.
>
I'm not good at English, and I don't know why "kernel memory duplicated
from user space" is so bad...
or memdup_user() ?
>> A qucik grep shows 250+ places where kmemdup_from_user() *may* be
>> used. I'll prepare a patchset to do this conversion.
>
> 250?
>
I just found out how many copy_from_user() following km/zalloc(), so
not all of them are replace-able.
> Let's not add wrapper for every two lines that happen to be used
> together.
>
Why not if we have good reasons? And I don't think we can call this
"happen to" if there are 250+ of them?
> BTW, can we drop strstarts() and kzfree() on the same reasoning?
>
On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <[email protected]> wrote:
> > Let's not add wrapper for every two lines that happen to be used
> > together.
> >
>
> Why not if we have good reasons? And I don't think we can call this
> "happen to" if there are 250+ of them?
The change is a good one. If a reviewer (me) sees it then you know the
code's all right and the review effort becomes less - all you need to check
is that the call site is using IS_ERR/PTR_ERR and isn't testing for
NULL. Less code, less chance for bugs.
Plus it makes kernel text smaller.
Yes, the name is a bit cumbersome.
On Fri, Mar 06, 2009 at 12:39:00AM -0800, Andrew Morton wrote:
> On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <[email protected]> wrote:
>
> > > Let's not add wrapper for every two lines that happen to be used
> > > together.
> > >
> >
> > Why not if we have good reasons? And I don't think we can call this
> > "happen to" if there are 250+ of them?
>
> The change is a good one. If a reviewer (me) sees it then you know the
> code's all right and the review effort becomes less - all you need to check
> is that the call site is using IS_ERR/PTR_ERR and isn't testing for
> NULL. Less code, less chance for bugs.
>
> Plus it makes kernel text smaller.
>
> Yes, the name is a bit cumbersome.
Some do NUL-termination afterwards and allocate "len + 1", but copy "len".
Some don't care.
On Fri, Mar 06, 2009 at 04:27:53PM +0800, Li Zefan wrote:
> Alexey Dobriyan wrote:
> > On Fri, Mar 06, 2009 at 03:04:12PM +0800, Li Zefan wrote:
> >> I notice there are many places doing copy_from_user() which follows
> >> kmalloc():
> >>
> >> dst = kmalloc(len, GFP_KERNEL);
> >> if (!dst)
> >> return -ENOMEM;
> >> if (copy_from_user(dst, src, len)) {
> >> kfree(dst);
> >> return -EFAULT
> >> }
> >>
> >> kmemdup_from_user() is a wrapper of the above code. With this new
> >> function, we don't have to write 'len' twice, which can lead to
> >> typos/mistakes. It also produces smaller code.
> >
> > Name totally sucks, it mixes kernel idiom of allocation with purely
> > userspace function.
> >
>
> I'm not good at English, and I don't know why "kernel memory duplicated
> from user space" is so bad...
>
> or memdup_user() ?
>
> >> A qucik grep shows 250+ places where kmemdup_from_user() *may* be
> >> used. I'll prepare a patchset to do this conversion.
> >
> > 250?
> >
>
> I just found out how many copy_from_user() following km/zalloc(), so
> not all of them are replace-able.
>
> > Let's not add wrapper for every two lines that happen to be used
> > together.
> >
>
> Why not if we have good reasons? And I don't think we can call this
> "happen to" if there are 250+ of them?
Please, read through them. This "250+" number suddenly will become
like 20, because wrapper is not good enough.
Andrew Morton wrote:
> On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <[email protected]> wrote:
>
>>> Let's not add wrapper for every two lines that happen to be used
>>> together.
>>>
>> Why not if we have good reasons? And I don't think we can call this
>> "happen to" if there are 250+ of them?
>
> The change is a good one. If a reviewer (me) sees it then you know the
> code's all right and the review effort becomes less - all you need to check
> is that the call site is using IS_ERR/PTR_ERR and isn't testing for
> NULL. Less code, less chance for bugs.
>
> Plus it makes kernel text smaller.
>
> Yes, the name is a bit cumbersome.
>
How about memdup_user()? like kstrndup() vs strndup_user().
Here is the statistics when using 5 kmemdup_from_user() in btrfs:
$ diffstat
ioctl.c | 49 ++++++++++++-------------------------------------
super.c | 13 ++++---------
2 files changed, 16 insertions(+), 46 deletions(-)
the kernel size on i386:
text data bss dec hex filename
288339 1924 508 290771 46fd3 fs/btrfs/btrfs.o.orig
text data bss dec hex filename
288255 1924 508 290687 46f7f fs/btrfs/btrfs.o
so saves 84 bytes.
the kernel size on IA64:
text data bss dec hex filename
898752 3736 109 902597 dc5c5 fs/btrfs/btrfs.o.orig
text data bss dec hex filename
898176 3712 109 901997 dc36d fs/btrfs/btrfs.o
so saves 576 bytes.
>> Why not if we have good reasons? And I don't think we can call this
>> "happen to" if there are 250+ of them?
>
> Please, read through them. This "250+" number suddenly will become
> like 20, because wrapper is not good enough.
>
I did read most of them roughly, and it will be at least 50.
> On Fri, Mar 06, 2009 at 12:39:00AM -0800, Andrew Morton wrote:
> > On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <[email protected]> wrote:
> >
> > > > Let's not add wrapper for every two lines that happen to be used
> > > > together.
> > > >
> > >
> > > Why not if we have good reasons? And I don't think we can call this
> > > "happen to" if there are 250+ of them?
> >
> > The change is a good one. If a reviewer (me) sees it then you know the
> > code's all right and the review effort becomes less - all you need to check
> > is that the call site is using IS_ERR/PTR_ERR and isn't testing for
> > NULL. Less code, less chance for bugs.
> >
> > Plus it makes kernel text smaller.
> >
> > Yes, the name is a bit cumbersome.
>
> Some do NUL-termination afterwards and allocate "len + 1", but copy "len".
> Some don't care.
if subsystem want string data, it should use strndup_user().
memdump don't need to care NUL-termination
In addition, also I often review various mm code and patch, I also like
this change.
On Fri, 06 Mar 2009 17:01:48 +0800 Li Zefan <[email protected]> wrote:
> How about memdup_user()? like kstrndup() vs strndup_user().
Sounds OK to me.
I notice there are many places doing copy_from_user() which follows
kmalloc():
dst = kmalloc(len, GFP_KERNEL);
if (!dst)
return -ENOMEM;
if (copy_from_user(dst, src, len)) {
kfree(dst);
return -EFAULT
}
memdup_user() is a wrapper of the above code. With this new function,
we don't have to write 'len' twice, which can lead to typos/mistakes.
It also produces smaller code and kernel text.
A quick grep shows 250+ places where memdup_user() *may* be used. I'll
prepare a patchset to do this conversion.
v1 -> v2: change the name from kmemdup_from_user to memdup_user.
Signed-off-by: Li Zefan <[email protected]>
---
Can this get into 2.6.29, so I can prepare patches based on linux-next?
And this won't cause regression, since no one uses it yet. :)
---
include/linux/string.h | 1 +
mm/util.c | 26 ++++++++++++++++++++++++++
2 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index 76ec218..79f30f3 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -12,6 +12,7 @@
#include <linux/stddef.h> /* for NULL */
extern char *strndup_user(const char __user *, long);
+extern void *memdup_user(const void __user *, size_t, gfp_t);
/*
* Include machine specific inline routines
diff --git a/mm/util.c b/mm/util.c
index 37eaccd..3d21c21 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -70,6 +70,32 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
EXPORT_SYMBOL(kmemdup);
/**
+ * memdup_user - duplicate memory region from user space
+ *
+ * @src: source address in user space
+ * @len: number of bytes to copy
+ * @gfp: GFP mask to use
+ *
+ * Returns an ERR_PTR() on failure.
+ */
+void *memdup_user(const void __user *src, size_t len, gfp_t gfp)
+{
+ void *p;
+
+ p = kmalloc_track_caller(len, gfp);
+ if (!p)
+ return ERR_PTR(-ENOMEM);
+
+ if (copy_from_user(p, src, len)) {
+ kfree(p);
+ return ERR_PTR(-EFAULT);
+ }
+
+ return p;
+}
+EXPORT_SYMBOL(memdup_user);
+
+/**
* __krealloc - like krealloc() but don't free @p.
* @p: object to reallocate memory for.
* @new_size: how many bytes of memory are required.
--
1.5.4.rc3
On Fri, 06 Mar 2009 17:49:45 +0800
Li Zefan <[email protected]> wrote:
> I notice there are many places doing copy_from_user() which follows
> kmalloc():
>
> dst = kmalloc(len, GFP_KERNEL);
> if (!dst)
> return -ENOMEM;
> if (copy_from_user(dst, src, len)) {
> kfree(dst);
> return -EFAULT
> }
>
> memdup_user() is a wrapper of the above code. With this new function,
> we don't have to write 'len' twice, which can lead to typos/mistakes.
> It also produces smaller code and kernel text.
>
> A quick grep shows 250+ places where memdup_user() *may* be used. I'll
> prepare a patchset to do this conversion.
>
> v1 -> v2: change the name from kmemdup_from_user to memdup_user.
>
> Signed-off-by: Li Zefan <[email protected]>
> ---
>
> Can this get into 2.6.29, so I can prepare patches based on linux-next?
> And this won't cause regression, since no one uses it yet. :)
I'd be OK with doing that from a patch logistics point of view, but I'd
prefer to leave a patch like this for a few days to gather more
feedback from other developers, which might push this into 2.6.30.
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 76ec218..79f30f3 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -12,6 +12,7 @@
> #include <linux/stddef.h> /* for NULL */
>
> extern char *strndup_user(const char __user *, long);
> +extern void *memdup_user(const void __user *, size_t, gfp_t);
>
> /*
> * Include machine specific inline routines
> diff --git a/mm/util.c b/mm/util.c
> index 37eaccd..3d21c21 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -70,6 +70,32 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
> EXPORT_SYMBOL(kmemdup);
>
> /**
> + * memdup_user - duplicate memory region from user space
> + *
> + * @src: source address in user space
> + * @len: number of bytes to copy
> + * @gfp: GFP mask to use
> + *
> + * Returns an ERR_PTR() on failure.
> + */
> +void *memdup_user(const void __user *src, size_t len, gfp_t gfp)
> +{
> + void *p;
> +
> + p = kmalloc_track_caller(len, gfp);
> + if (!p)
> + return ERR_PTR(-ENOMEM);
> +
> + if (copy_from_user(p, src, len)) {
> + kfree(p);
> + return ERR_PTR(-EFAULT);
> + }
> +
> + return p;
> +}
> +EXPORT_SYMBOL(memdup_user);
> +
> +/**
> * __krealloc - like krealloc() but don't free @p.
> * @p: object to reallocate memory for.
> * @new_size: how many bytes of memory are required.
> --
> 1.5.4.rc3
On Fri, 6 Mar 2009 15:03:35 -0800
Andrew Morton <[email protected]> wrote:
> >
> > /**
> > + * memdup_user - duplicate memory region from user space
> > + *
> > + * @src: source address in user space
> > + * @len: number of bytes to copy
> > + * @gfp: GFP mask to use
> > + *
> > + * Returns an ERR_PTR() on failure.
> > + */
> > +void *memdup_user(const void __user *src, size_t len, gfp_t gfp)
> > +{
> > + void *p;
> > +
> > + p = kmalloc_track_caller(len, gfp);
> > + if (!p)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + if (copy_from_user(p, src, len)) {
> > + kfree(p);
> > + return ERR_PTR(-EFAULT);
> > + }
> > +
> > + return p;
> > +}
> > +EXPORT_SYMBOL(memdup_user);
Hi,
I like the general idea of this a lot; it will make things much less
error prone (and we can add some sanity checks on "len" to catch the
standard security holes around copy_from_user usage). I'd even also
want a memdup_array() like thing in the style of calloc().
However, I have two questions/suggestions for improvement:
I would like to question the use of the gfp argument here;
copy_from_user sleeps, so you can't use GFP_ATOMIC anyway.
You can't use GFP_NOFS etc, because the pagefault path will happily do
things that are equivalent, if not identical, to GFP_KERNEL.
So the only value you can pass in correctly, as far as I can see, is
GFP_KERNEL. Am I wrong?
A second thing.. I'd like to have this function return NULL on failure;
error checking a pointer for NULL is so much easier than testing for
anything else; the only distinction is -ENOMEM versus -EFAULT, and I'm
not sure that that is worth the complexity on all callers.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
> I would like to question the use of the gfp argument here;
> copy_from_user sleeps, so you can't use GFP_ATOMIC anyway.
> You can't use GFP_NOFS etc, because the pagefault path will happily do
> things that are equivalent, if not identical, to GFP_KERNEL.
That's a convincing argument, and furthermore, strndup_user() does not
take a gfp parameter, so interface consistency also argues that the
function prototype should just be
void *memdup_user(const void __user *src, size_t len);
(By the way, the len parameter of strndup_user() is declared as long,
which seems strange, since it matches neither the userspace strndup()
nor the kernel kstrndup(), which both use size_t. So using size_t for
memdup_user() and possibly fixing strndup_user() to use size_t as well
seems like the sanest thing)
- R.
On Sat, 7 Mar 2009 08:48:05 -0800 Arjan van de Ven <[email protected]> wrote:
> On Fri, 6 Mar 2009 15:03:35 -0800
> Andrew Morton <[email protected]> wrote:
>
> > >
> > > /**
> > > + * memdup_user - duplicate memory region from user space
> > > + *
> > > + * @src: source address in user space
> > > + * @len: number of bytes to copy
> > > + * @gfp: GFP mask to use
> > > + *
> > > + * Returns an ERR_PTR() on failure.
> > > + */
> > > +void *memdup_user(const void __user *src, size_t len, gfp_t gfp)
> > > +{
> > > + void *p;
> > > +
> > > + p = kmalloc_track_caller(len, gfp);
> > > + if (!p)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + if (copy_from_user(p, src, len)) {
> > > + kfree(p);
> > > + return ERR_PTR(-EFAULT);
> > > + }
> > > +
> > > + return p;
> > > +}
> > > +EXPORT_SYMBOL(memdup_user);
>
> Hi,
>
> I like the general idea of this a lot; it will make things much less
> error prone (and we can add some sanity checks on "len" to catch the
> standard security holes around copy_from_user usage). I'd even also
> want a memdup_array() like thing in the style of calloc().
>
> However, I have two questions/suggestions for improvement:
>
> I would like to question the use of the gfp argument here;
> copy_from_user sleeps, so you can't use GFP_ATOMIC anyway.
> You can't use GFP_NOFS etc, because the pagefault path will happily do
> things that are equivalent, if not identical, to GFP_KERNEL.
>
> So the only value you can pass in correctly, as far as I can see, is
> GFP_KERNEL. Am I wrong?
True.
> A second thing.. I'd like to have this function return NULL on failure;
> error checking a pointer for NULL is so much easier than testing for
> anything else; the only distinction is -ENOMEM versus -EFAULT, and I'm
> not sure that that is worth the complexity on all callers.
This errno will be returned to userspace. If the caller guesses wrong,
that's a kernel bug, of the regression variety.
>>> +EXPORT_SYMBOL(memdup_user);
>
> Hi,
>
> I like the general idea of this a lot; it will make things much less
> error prone (and we can add some sanity checks on "len" to catch the
> standard security holes around copy_from_user usage). I'd even also
> want a memdup_array() like thing in the style of calloc().
>
> However, I have two questions/suggestions for improvement:
>
> I would like to question the use of the gfp argument here;
> copy_from_user sleeps, so you can't use GFP_ATOMIC anyway.
> You can't use GFP_NOFS etc, because the pagefault path will happily do
> things that are equivalent, if not identical, to GFP_KERNEL.
>
> So the only value you can pass in correctly, as far as I can see, is
> GFP_KERNEL. Am I wrong?
>
Right! I just dug and found a few kmalloc(GFP_ATOMIC/GFP_NOFS)+copy_from_user(),
so we have one more reason to use this memdup_user().
On Mon, 09 Mar 2009 10:22:08 +0800 Li Zefan <[email protected]> wrote:
> >>> +EXPORT_SYMBOL(memdup_user);
> >
> > Hi,
> >
> > I like the general idea of this a lot; it will make things much less
> > error prone (and we can add some sanity checks on "len" to catch the
> > standard security holes around copy_from_user usage). I'd even also
> > want a memdup_array() like thing in the style of calloc().
> >
> > However, I have two questions/suggestions for improvement:
> >
> > I would like to question the use of the gfp argument here;
> > copy_from_user sleeps, so you can't use GFP_ATOMIC anyway.
> > You can't use GFP_NOFS etc, because the pagefault path will happily do
> > things that are equivalent, if not identical, to GFP_KERNEL.
> >
> > So the only value you can pass in correctly, as far as I can see, is
> > GFP_KERNEL. Am I wrong?
> >
>
> Right! I just dug and found a few kmalloc(GFP_ATOMIC/GFP_NOFS)+copy_from_user(),
> so we have one more reason to use this memdup_user().
gack, those callsites are probably buggy. Where are they?
Andrew Morton wrote:
> On Mon, 09 Mar 2009 10:22:08 +0800 Li Zefan <[email protected]> wrote:
>
>>>>> +EXPORT_SYMBOL(memdup_user);
>>> Hi,
>>>
>>> I like the general idea of this a lot; it will make things much less
>>> error prone (and we can add some sanity checks on "len" to catch the
>>> standard security holes around copy_from_user usage). I'd even also
>>> want a memdup_array() like thing in the style of calloc().
>>>
>>> However, I have two questions/suggestions for improvement:
>>>
>>> I would like to question the use of the gfp argument here;
>>> copy_from_user sleeps, so you can't use GFP_ATOMIC anyway.
>>> You can't use GFP_NOFS etc, because the pagefault path will happily do
>>> things that are equivalent, if not identical, to GFP_KERNEL.
>>>
>>> So the only value you can pass in correctly, as far as I can see, is
>>> GFP_KERNEL. Am I wrong?
>>>
>> Right! I just dug and found a few kmalloc(GFP_ATOMIC/GFP_NOFS)+copy_from_user(),
>> so we have one more reason to use this memdup_user().
>
> gack, those callsites are probably buggy. Where are they?
>
Yes, either buggy or should use GFP_KERNEL.
All are in -mm only, except the first one:
drivers/isdn/i4l/isdn_common.c:
struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC);
...
if (copy_from_user(skb_put(skb, len), buf, len)) {
net/irda/af_irda.c:
ias_opt = kmalloc(sizeof(struct irda_ias_set), GFP_ATOMIC);
...
if (copy_from_user(ias_opt, optval, optlen)) {
fs/btrfs/ioctl.c:
vol_args = kmalloc(sizeof(*vol_args), GFP_NOFS);
...
if (copy_from_user(vol_args, arg, sizeof(*vol_args))) {
fs/ocfs2/dlm/dlmfs.c:
lvb_buf = kmalloc(writelen, GFP_NOFS);
...
bytes_left = copy_from_user(lvb_buf, buf, writelen);
net/sunrpc/auth_gss/auth_gss.c:
buf = kmalloc(mlen, GFP_NOFS);
...
if (copy_from_user(buf, src, mlen))
On Mon, 09 Mar 2009 11:30:18 +0800 Li Zefan <[email protected]> wrote:
> Andrew Morton wrote:
> > On Mon, 09 Mar 2009 10:22:08 +0800 Li Zefan <[email protected]> wrote:
> >
> >>>>> +EXPORT_SYMBOL(memdup_user);
> >>> Hi,
> >>>
> >>> I like the general idea of this a lot; it will make things much less
> >>> error prone (and we can add some sanity checks on "len" to catch the
> >>> standard security holes around copy_from_user usage). I'd even also
> >>> want a memdup_array() like thing in the style of calloc().
> >>>
> >>> However, I have two questions/suggestions for improvement:
> >>>
> >>> I would like to question the use of the gfp argument here;
> >>> copy_from_user sleeps, so you can't use GFP_ATOMIC anyway.
> >>> You can't use GFP_NOFS etc, because the pagefault path will happily do
> >>> things that are equivalent, if not identical, to GFP_KERNEL.
> >>>
> >>> So the only value you can pass in correctly, as far as I can see, is
> >>> GFP_KERNEL. Am I wrong?
> >>>
> >> Right! I just dug and found a few kmalloc(GFP_ATOMIC/GFP_NOFS)+copy_from_user(),
> >> so we have one more reason to use this memdup_user().
> >
> > gack, those callsites are probably buggy. Where are they?
> >
>
> Yes, either buggy or should use GFP_KERNEL.
>
> All are in -mm only, except the first one:
>
> drivers/isdn/i4l/isdn_common.c:
> struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC);
> ...
> if (copy_from_user(skb_put(skb, len), buf, len)) {
Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.
>
> net/irda/af_irda.c:
> ias_opt = kmalloc(sizeof(struct irda_ias_set), GFP_ATOMIC);
> ...
> if (copy_from_user(ias_opt, optval, optlen)) {
Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.
>
> fs/btrfs/ioctl.c:
> vol_args = kmalloc(sizeof(*vol_args), GFP_NOFS);
> ...
> if (copy_from_user(vol_args, arg, sizeof(*vol_args))) {
Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.
> > fs/ocfs2/dlm/dlmfs.c:
> lvb_buf = kmalloc(writelen, GFP_NOFS);
> ...
> bytes_left = copy_from_user(lvb_buf, buf, writelen);
Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.
>
> net/sunrpc/auth_gss/auth_gss.c:
> buf = kmalloc(mlen, GFP_NOFS);
> ...
> if (copy_from_user(buf, src, mlen))
Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.