vfs: use kstrdup()

This piece of code can be replaced by a kstrdup().

---
 fs/namespace.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

index 4f6f763..08c3b2d 100644
@@ -127,14 +127,7 @@ struct vfsmount *alloc_vfsmnt(const char *name)
 		INIT_LIST_HEAD(&mnt->mnt_slave_list);
 		INIT_LIST_HEAD(&mnt->mnt_slave);
 		atomic_set(&mnt->__mnt_writers, 0);
-		if (name) {
-			int size = strlen(name) + 1;
-			char *newname = kmalloc(size, GFP_KERNEL);
-			if (newname) {
-				memcpy(newname, name, size);
-				mnt->mnt_devname = newname;
-			}
-		}
+		mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
 	}
 	return mnt;
 }

1.5.4.rc3

Looks good to me.

[Li Zefan - Sat, Jul 19, 2008 at 06:16:20PM +0800]

| This piece of code can be replaced by a kstrdup().
| 
| Signed-off-by: Li Zefan <[email protected]>
| ---
|  fs/namespace.c |    9 +--------
|  1 files changed, 1 insertions(+), 8 deletions(-)
| 
| diff --git a/fs/namespace.c b/fs/namespace.c
| index 4f6f763..08c3b2d 100644
| --- a/fs/namespace.c
| +++ b/fs/namespace.c
| @@ -127,14 +127,7 @@ struct vfsmount *alloc_vfsmnt(const char *name)
|  		INIT_LIST_HEAD(&mnt->mnt_slave_list);
|  		INIT_LIST_HEAD(&mnt->mnt_slave);
|  		atomic_set(&mnt->__mnt_writers, 0);
| -		if (name) {
| -			int size = strlen(name) + 1;
| -			char *newname = kmalloc(size, GFP_KERNEL);
| -			if (newname) {
| -				memcpy(newname, name, size);
| -				mnt->mnt_devname = newname;
| -			}
| -		}
| +		mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
|  	}
|  	return mnt;
|  }
| -- 
| 1.5.4.rc3
| 
| 

but kstrdup may return NULL - is it safe there?
Sorry if that "not smart" question.

- Cyrill -

[Cyrill Gorcunov - Sat, Jul 19, 2008 at 05:13:17PM +0400] [...]

| | -			}
| | -		}
| | +		mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
| |  	}
| |  	return mnt;
| |  }
| | -- 
| | 1.5.4.rc3
| | 
| | 
| 
| but kstrdup may return NULL - is it safe there?
| Sorry if that "not smart" question.
| 
| 		- Cyrill -

ah, I see it is safe - sorry for noise

- Cyrill -

FWIW, it _is_ a good question.

* is all code treating ->mnt_devname as optional? AFAICS, there's
at least one place in NFS that doesn't. We could treat failing allocation the same way we treat failing allocation of vfsmount itself - callers can cope with that already.
* AFAICS, it should be const char *.
* ... or perhaps we shouldn't copy it at all. How about something
like

	struct {
		int count;
		char name[];
	}

with cloning sharing the reference and bumping the count, protecting it with e.g. vfsmount_lock? For setups where we have a lot of bindings/namespaces it might be noticable.

Any takers?

I just did a cleanup, and the original code didn't check for NULL.

I just looked into the git history, and I found out since fs/namespace.c was created in v2.4.10.4, the code has never changed to check for failing allocation of ->mnt_devname.

Agreed.

I'm not sure whether this is a good idea, as I have limited knowledge about the vfs internal.

I know.

It used to have no users beyond fs/namespace.c itself and for _those_ the thing had been optional, so leaving NULL had been OK. Unfortunately, it still had been a bad idea - new users had appeared and those predictably didn't notice that fun detail.

The right thing here is to consider failing allocation of ->mnt_devname as failure of the entire alloc.

[Al Viro - Mon, Jul 21, 2008 at 08:03:46AM +0100]

| On Mon, Jul 21, 2008 at 02:29:47PM +0800, Li Zefan wrote:
| > > FWIW, it _is_ a good question.
| > > 
| > > 	* is all code treating ->mnt_devname as optional?  AFAICS, there's
| > > at least one place in NFS that doesn't.  We could treat failing allocation
| > > the same way we treat failing allocation of vfsmount itself - callers can
| > > cope with that already.
| > 
| > I just did a cleanup, and the original code didn't check for NULL.
| 
| I know.
| 
| > I just looked into the git history, and I found out since fs/namespace.c was
| > created in v2.4.10.4, the code has never changed to check for failing
| > allocation of ->mnt_devname.
| 
| It used to have no users beyond fs/namespace.c itself and for _those_ the
| thing had been optional, so leaving NULL had been OK.  Unfortunately, it
| still had been a bad idea - new users had appeared and those predictably
| didn't notice that fun detail.
| 
| The right thing here is to consider failing allocation of ->mnt_devname
| as failure of the entire alloc.
|

Hi Al,

thanks a lot for comments! I think it is more then enough for now (i'm about failing allocation in whole). If that happens and we are not able to duplicate string - it's quite probable we will be in serious troubles soon anyway ('cause of further kmalloc calls). So it's better to get mount allocation fails then NULL deref.

- Cyrill -

[Al Viro - Mon, Jul 21, 2008 at 08:03:46AM +0100]
|
[...]

| The right thing here is to consider failing allocation of ->mnt_devname
| as failure of the entire alloc.
| 

Al, what about the patch below? I'm not sure if Li's version already in someone tree so it's from-the-scratch. If this ok, i think Li could update his version and resend.

- Cyrill -
---

Index: linux-2.6.git/fs/namespace.c
===================================================================
@@ -112,9 +112,13 @@ struct vfsmount *alloc_vfsmnt(const char
 		int err;
 
 		err = mnt_alloc_id(mnt);
-		if (err) {
-			kmem_cache_free(mnt_cache, mnt);
-			return NULL;
+		if (err)
+			goto err;
+
+		if (name) {
+			mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
+			if (!mnt->mnt_devname)
+				goto err;
 		}
 
 		atomic_set(&mnt->mnt_count, 1);

@@ -127,16 +131,12 @@ struct vfsmount *alloc_vfsmnt(const char
 		INIT_LIST_HEAD(&mnt->mnt_slave_list);
 		INIT_LIST_HEAD(&mnt->mnt_slave);
 		atomic_set(&mnt->__mnt_writers, 0);
-		if (name) {
-			int size = strlen(name) + 1;
-			char *newname = kmalloc(size, GFP_KERNEL);
-			if (newname) {
-				memcpy(newname, name, size);
-				mnt->mnt_devname = newname;
-			}
-		}
 	}
 	return mnt;
+
+err:
+	kmem_cache_free(mnt_cache, mnt);
+	return NULL;
 }
 
 /*

[Al Viro - Mon, Jul 21, 2008 at 09:28:57AM +0100]

| On Mon, Jul 21, 2008 at 12:04:27PM +0400, Cyrill Gorcunov wrote:
| >  		err = mnt_alloc_id(mnt);
| > -		if (err) {
| > -			kmem_cache_free(mnt_cache, mnt);
| > -			return NULL;
| > +		if (err)
| > +			goto err;
| 
| Ugh...  Labels are in a separate namespace, but really...
| 
| > +		if (name) {
| > +			mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
| > +			if (!mnt->mnt_devname)
| > +				goto err;
| 
| > +err:
| > +	kmem_cache_free(mnt_cache, mnt);
| > +	return NULL;
| 
| Leak; note the mnt_alloc_id() above.  Either do that kstrdup() first and
| kfree the result on mnt_alloc_id() failure or do mnt_free_id() on kstrdup()
| one...
| 

oh my, that stupid from my side - will update shortly

- Cyrill -

[Al Viro - Mon, Jul 21, 2008 at 09:28:57AM +0100]

| On Mon, Jul 21, 2008 at 12:04:27PM +0400, Cyrill Gorcunov wrote:
| >  		err = mnt_alloc_id(mnt);
| > -		if (err) {
| > -			kmem_cache_free(mnt_cache, mnt);
| > -			return NULL;
| > +		if (err)
| > +			goto err;
| 
| Ugh...  Labels are in a separate namespace, but really...
| 
| > +		if (name) {
| > +			mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
| > +			if (!mnt->mnt_devname)
| > +				goto err;
| 
| > +err:
| > +	kmem_cache_free(mnt_cache, mnt);
| > +	return NULL;
| 
| Leak; note the mnt_alloc_id() above.  Either do that kstrdup() first and
| kfree the result on mnt_alloc_id() failure or do mnt_free_id() on kstrdup()
| one...
| 

ok, here is an update version. labels names could be not that good choosen (check please).

- Cyrill -
---

Index: linux-2.6.git/fs/namespace.c
===================================================================
@@ -112,9 +112,13 @@ struct vfsmount *alloc_vfsmnt(const char
 		int err;
 
 		err = mnt_alloc_id(mnt);
-		if (err) {
-			kmem_cache_free(mnt_cache, mnt);
-			return NULL;
+		if (err)
+			goto out_free_cache;
+
+		if (name) {
+			mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
+			if (!mnt->mnt_devname)
+				goto out_free_id;
 		}
 
 		atomic_set(&mnt->mnt_count, 1);

@@ -127,16 +131,14 @@ struct vfsmount *alloc_vfsmnt(const char
 		INIT_LIST_HEAD(&mnt->mnt_slave_list);
 		INIT_LIST_HEAD(&mnt->mnt_slave);
 		atomic_set(&mnt->__mnt_writers, 0);
-		if (name) {
-			int size = strlen(name) + 1;
-			char *newname = kmalloc(size, GFP_KERNEL);
-			if (newname) {
-				memcpy(newname, name, size);
-				mnt->mnt_devname = newname;
-			}
-		}
 	}
 	return mnt;
+
+out_free_id:
+	mnt_free_id(mnt);
+out_free_cache:
+	kmem_cache_free(mnt_cache, mnt);
+	return NULL;
 }
 
 /*

It's already in -mm tree, but Andrew can drop it and queue the new one.

[Li Zefan - Mon, Jul 21, 2008 at 04:28:17PM +0800]

| Cyrill Gorcunov wrote:
| > [Al Viro - Mon, Jul 21, 2008 at 08:03:46AM +0100]
| > | 
| > [...]
| > | The right thing here is to consider failing allocation of ->mnt_devname
| > | as failure of the entire alloc.
| > | 
| > 
| > Al, what about the patch below? I'm not sure if Li's version already
| > in someone tree so it's from-the-scratch. If this ok, i think Li could
| > update his version and resend.
| > 
| 
| It's already in -mm tree, but Andrew can drop it and queue the new one.
| 
| > 		- Cyrill -
| > ---
| > 
| > Index: linux-2.6.git/fs/namespace.c
| > ===================================================================
| > --- linux-2.6.git.orig/fs/namespace.c	2008-07-21 11:34:37.000000000 +0400
| > +++ linux-2.6.git/fs/namespace.c	2008-07-21 12:00:01.000000000 +0400
| > @@ -112,9 +112,13 @@ struct vfsmount *alloc_vfsmnt(const char
| >  		int err;
| >  
| >  		err = mnt_alloc_id(mnt);
| > -		if (err) {
| > -			kmem_cache_free(mnt_cache, mnt);
| > -			return NULL;
| > +		if (err)
| > +			goto err;
| > +
| > +		if (name) {
| > +			mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
| > +			if (!mnt->mnt_devname)
| 
| should call mnt_free_id() here.
| 
| > +				goto err;
| >  		}
| >  
| >  		atomic_set(&mnt->mnt_count, 1);
| > @@ -127,16 +131,12 @@ struct vfsmount *alloc_vfsmnt(const char
| >  		INIT_LIST_HEAD(&mnt->mnt_slave_list);
| >  		INIT_LIST_HEAD(&mnt->mnt_slave);
| >  		atomic_set(&mnt->__mnt_writers, 0);
| > -		if (name) {
| > -			int size = strlen(name) + 1;
| > -			char *newname = kmalloc(size, GFP_KERNEL);
| > -			if (newname) {
| > -				memcpy(newname, name, size);
| > -				mnt->mnt_devname = newname;
| > -			}
| > -		}
| >  	}
| >  	return mnt;
| > +
| > +err:
| > +	kmem_cache_free(mnt_cache, mnt);
| > +	return NULL;
| >  }
| >  
| >  /*
| > 
| > 
|

well, as only Al find new version more or less correct we could update yours.

- Cyrill -

Sorry, I don't quite catch what you mean for my poor English. :(

Do you expect me to update and resend my version? or do you mean don't drop the patch in -mm but you will resend a patch based against it?

Hi,

Andrew already merged your patch so I think Cyrill should just send a new patch against -mm.

Pekka

[Pekka Enberg - Mon, Jul 21, 2008 at 12:06:55PM +0300]

| Hi,
| 
| On Mon, Jul 21, 2008 at 11:59 AM, Li Zefan <[email protected]> wrote:
| >> well, as only Al find new version more or less correct we could
| >> update yours.
| >
| > Sorry, I don't quite catch what you mean for my poor English. :(
| >
| > Do you expect me to update and resend my version? or do you mean don't
| > drop the patch in -mm but you will resend a patch based against it?
| 
| Andrew already merged your patch so I think Cyrill should just send a
| new patch against -mm.
| 
|                           Pekka
|

Thanks guys, will send patch against -mm soon (a bit busy now)

- Cyrill -

Oh, I just sent out a mix-up patch because I'm going to get off work but I didn't get your respond, you can just ignore it.

[Li Zefan - Mon, Jul 21, 2008 at 06:10:19PM +0800]

| Cyrill Gorcunov wrote:
| > [Pekka Enberg - Mon, Jul 21, 2008 at 12:06:55PM +0300]
| > | Hi,
| > | 
| > | On Mon, Jul 21, 2008 at 11:59 AM, Li Zefan <[email protected]> wrote:
| > | >> well, as only Al find new version more or less correct we could
| > | >> update yours.
| > | >
| > | > Sorry, I don't quite catch what you mean for my poor English. :(
| > | >
| > | > Do you expect me to update and resend my version? or do you mean don't
| > | > drop the patch in -mm but you will resend a patch based against it?
| > | 
| > | Andrew already merged your patch so I think Cyrill should just send a
| > | new patch against -mm.
| > | 
| > |                           Pekka
| > |
| > 
| > Thanks guys, will send patch against -mm soon (a bit busy now)
| >  
| 
| Oh, I just sent out a mix-up patch because I'm going to get off work but I didn't
| get your respond, you can just ignore it.
|

it's ok - since you did my work for me :) Thanks.

- Cyrill -

<shrug>

FWIW, the latest posted variant looks sane. Whether you want it in one piece or two... up to you, folks. Whichever way you do it, throw the char * -> const char * transition for mnt_devname into the mix and send the prefered splitup; I'll apply it after I get some sleep...