[RFC][PATCH] kmemdup_from_user(): introduce

[RFC][PATCH] kmemdup_from_user(): introduce

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Américo Wang]

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 <lizf@cn.fujitsu.com>
>---
> 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 majordomo@vger.kernel.org
>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!
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[KOSAKI Motohiro]

> > /**
> >+ * 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.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[KOSAKI Motohiro]

(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 <kosaki.motohiro@jp.fujitsu.com>
---
 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



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Li Zefan]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Alexey Dobriyan]

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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Li Zefan]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Andrew Morton]

On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <lizf@cn.fujitsu.com> 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Alexey Dobriyan]

On Fri, Mar 06, 2009 at 12:39:00AM -0800, Andrew Morton wrote:
> On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <lizf@cn.fujitsu.com> 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[KOSAKI Motohiro]

> On Fri, Mar 06, 2009 at 12:39:00AM -0800, Andrew Morton wrote:
> > On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <lizf@cn.fujitsu.com> 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.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Li Zefan]

Andrew Morton wrote:
> On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <lizf@cn.fujitsu.com> 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Andrew Morton]

On Fri, 06 Mar 2009 17:01:48 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:

> How about memdup_user()? like kstrndup() vs strndup_user().

Sounds OK to me.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH -v2] memdup_user(): introduce

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -v2] memdup_user(): introduce

[Andrew Morton]

On Fri, 06 Mar 2009 17:49:45 +0800
Li Zefan <lizf@cn.fujitsu.com> 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 <lizf@cn.fujitsu.com>
> ---
> 
> 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -v2] memdup_user(): introduce

[Arjan van de Ven]

On Fri, 6 Mar 2009 15:03:35 -0800
Andrew Morton <akpm@linux-foundation.org> 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -v2] memdup_user(): introduce

[Roland Dreier]

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


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -v2] memdup_user(): introduce

[Andrew Morton]

On Sat, 7 Mar 2009 08:48:05 -0800 Arjan van de Ven <arjan@infradead.org> wrote:

> On Fri, 6 Mar 2009 15:03:35 -0800
> Andrew Morton <akpm@linux-foundation.org> 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.  

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -v2] memdup_user(): introduce

[Li Zefan]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -v2] memdup_user(): introduce

[Andrew Morton]

On Mon, 09 Mar 2009 10:22:08 +0800 Li Zefan <lizf@cn.fujitsu.com> 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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -v2] memdup_user(): introduce

[Li Zefan]

Andrew Morton wrote:
> On Mon, 09 Mar 2009 10:22:08 +0800 Li Zefan <lizf@cn.fujitsu.com> 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))

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -v2] memdup_user(): introduce

[Andrew Morton]

On Mon, 09 Mar 2009 11:30:18 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:

> Andrew Morton wrote:
> > On Mon, 09 Mar 2009 10:22:08 +0800 Li Zefan <lizf@cn.fujitsu.com> 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Alexey Dobriyan]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] kmemdup_from_user(): introduce

[Li Zefan]

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


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>