The renaming of a kobject will fail if there is another kobject

with the same name belonging to another namespace.
This patch makes the kobject lookup in kobject_rename to check if

the object exists _and_ belongs to the same namespace.
Signed-off-by: Daniel Lezcano <dlezcano@fr.ibm.com>

Acked-by: Benjamin Thery <benjamin.thery@bull.net>

---

 fs/sysfs/dir.c        |   10 ++++++++++

 include/linux/sysfs.h |    7 +++++++

 lib/kobject.c         |    2 +-

 3 files changed, 18 insertions(+), 1 deletion(-)
Index: linux-vanilla/fs/sysfs/dir.c

===================================================================

--- linux-vanilla.orig/fs/sysfs/dir.c

+++ linux-vanilla/fs/sysfs/dir.c

@@ -902,6 +902,16 @@ err_out:

 	return error;

 }
+int sysfs_tag_cmp(struct kobject *kobj1, struct kobject *kobj2)

+{

+	struct sysfs_dirent *sd1 = kobj1->sd;

+	struct sysfs_dirent *sd2 = kobj2->sd;

+	const void *tag1 = sysfs_dirent_tag(sd1);

+	const void *tag2 = sysfs_dirent_tag(sd2);

+

+	return tag1 != tag2;

+}

+

 int sysfs_rename_dir(struct kobject * kobj, const char *new_name)

 {

 	struct sysfs_dirent *sd = kobj->sd;

Index: linux-vanilla/include/linux/sysfs.h

===================================================================

--- linux-vanilla.orig/include/linux/sysfs.h

+++ linux-vanilla/include/linux/sysfs.h

@@ -95,6 +95,8 @@ int sysfs_schedule_callback(struct kobje
 int __must_check sysfs_create_dir(struct kobject *kobj);

 void sysfs_remove_dir(struct kobject *kobj);

+int sysfs_tag_cmp(struct kobject *kobj1, struct kobject *kobj2);

+

 int __must_check sysfs_rename_dir(struct kobject *kobj, const char *new_name);

 int __must_check sysfs_move_dir(struct kobject *kobj,

 				struct kobject *new_parent_kobj);

@@ -154,6 +156,11 @@ static inline void sysfs_remove_dir(stru

 {

 }
+static inline int sysfs_tag_cmp(struct kobject *kobj1, struct kobject *kobj2)

+{

+	return 0;

+}

+

 static inline int sysfs_rename_dir(struct kobject *kobj, const char *new_name)

 {

 	ret...
[ message continues ]

Ok so we are dealing with fallout from:
commit 34358c26a2c96b2a068dc44e0ac602106a466bce

Author: Greg Kroah-Hartman <gregkh@suse.de>

Date:   Wed Oct 24 16:52:31 2007 -0700
    kobject: check for duplicate names in kobject_rename
    This should catch any duplicate names before we try to tell sysfs to

    rename the object.  This happens a lot with older versions of udev and

    the network rename scripts.
    Cc: David Miller <davem@davemloft.net>

    Cc: Kay Sievers <kay.sievers@vrfy.org>

    Cc: Rafael J. Wysocki <rjw@sisk.pl>

    Cc: Tejun Heo <htejun@gmail.com>

    Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
Which added the check in kobject_rename to prevent problems, and

seems to be causing a few.
I believe this earlier? patch addresses the problem:
commit c8d90dca3211966ba5189e0f3d4bccd558d9ae08

Author: Stephen Hemminger <shemminger@linux-foundation.org>

Date:   Fri Oct 26 03:53:42 2007 -0700
    [NET] dev_change_name: ignore changes to same name
    Prevent error/backtrace from dev_rename() when changing

    name of network device to the same name. This is a common

    situation with udev and other scripts that bind addr to device.
    Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

    Signed-off-by: David S. Miller <davem@davemloft.net>
And the challenge is that we are getting false positives in the check

to see if renames will fail.
	/* see if this name is already in use */

	if (kobj->kset) {

		struct kobject *temp_kobj;

		temp_kobj = kset_find_obj(kobj->kset, new_name);

		if (temp_kobj) {

			printk(KERN_WARNING "kobject '%s' can not be renamed

				"to '%s' as '%s' is already in existance.\n",

				kobject_name(kobj), new_name, new_name);

			kobject_put(temp_kobj);

			return -EINVAL;

		}

	}
If the kobject layer wants to perform the test above how do we support

it by giving it enough information to perform the test without false

positi...
[ message continues ]

Wireless uses it also for some things, and it requires that it fail if a

duplicate is found.  I thought that s390 also used it, but I don't see

that usage in the tree anymore, perhaps they switched to something else.
good luck,
greg k-h

--

When looking at kobject_rename I found two bugs with

that exist when sysfs support is disabled in the kernel.
kobject_rename does not change the name on the kobject when

sysfs support is not compiled in.
kobject_rename without locking attempts to check the

validity of a rename operation, which the kobject layer

simply does not have the infrastructure to do so.
This patch documents the previously unstated requirement of

kobject_rename that is the responsibility of the caller to

provide mutual exclusion and to be certain that the new_name

for the kobject is valid.
This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case

to call kobject_set_name to actually change the kobject_name.
This patch removes the bogus and misleading check in kobject_rename

that attempts to see if a rename is valid.  The check is bogus

because we do not have the proper locking.  The check is misleading

because it looks like we can and do perform useful checking at the

kobject level that we don't.
The users in net/core/dev.c already have all of the necessary checks

in place and don't care.  The other use in net/core/wireless.c

as originally implemented is incorrect because it performs no locking

and a simple patch has been sent that adds the necessary locking

and sanity checks there.  Ensuring this patch will not have an

effect on users of kobject_rename or device_rename.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

---

 Documentation/kobject.txt |    4 ++++

 drivers/base/core.c       |    5 +++++

 include/linux/sysfs.h     |    2 +-

 lib/kobject.c             |   18 +++++-------------

 4 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/Documentation/kobject.txt b/Documentation/kobject.txt

index bf3256e..79184b4 100644

--- a/Documentation/kobject.txt

+++ b/Documentation/kobject.txt

@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
     int kobject_rename(struct kobject *kobj, const char *new_name);
+Note kobject_rename does perfor...
[ message continues ]

Eric, Randy Dunlap has found that this patch breaks the build when

CONFIG_SYSFS is not enabled.  Can you please fix it up before I send it

to Linus?
The exact error is:

In file included from /local/linsrc/next-20080509/include/linux/kobject.h:21,

                 from /local/linsrc/next-20080509/include/linux/module.h:16,

                 from /local/linsrc/next-20080509/include/linux/crypto.h:21,

                 from /local/linsrc/next-20080509/arch/x86/kernel/asm-offsets_64.c:7,

                 from /local/linsrc/next-20080509/arch/x86/kernel/asm-offsets.c:4:

/local/linsrc/next-20080509/include/linux/sysfs.h: In function 'sysfs_rename_dir':

/local/linsrc/next-20080509/include/linux/sysfs.h:142: error: implicit declaration of function 'kobject_set_name'
thanks,
greg k-h

--

I will take a look in the morning and see if I can see what is wrong.
Which tree was this error against?  I thought I tested this case,

and I'm wondering if there might be another patch that is hiding

kobject_set_name.
Eric

--

Argh, headers "cross-dependencies":
* linux/kobject.h includes linux/sysfs.h before defining

   kobject_set_name()
* linux/sysfs.h needs to include linux/kobject.h to find

   kobject_set_name() definition (for inlined sysfs_rename_dir() when

   CONFIG_SYSFS=n)
sysfs_rename_dir() is only called by kobject.c, kobject_rename().

I guess this kind of patch is not acceptable to fix the depency?
Index: linux-2.6/lib/kobject.c

===================================================================

--- linux-2.6.orig/lib/kobject.c        2008-05-13 15:14:38.000000000 +0200

+++ linux-2.6/lib/kobject.c     2008-05-13 15:58:37.000000000 +0200

@@ -416,8 +416,11 @@ int kobject_rename(struct kobject *kobj,

         envp[0] = devpath_string;

         envp[1] = NULL;
+#ifdef CONFIG_SYSFS

         error = sysfs_rename_dir(kobj, new_name);

-

+#else

+       error = kobject_set_name(kobj, "%s", new_name);

+#endif

         /* This function is mostly/only used for network interface.

          * Some hotplug package track interfaces by their name and

          * therefore want to know when the name is changed by the user. */
--

B e n j a m i n   T h e r y  - BULL/DT/Open Software R&D
    http://www.bull.com

--

Ick, no.  I'd rather add a kobject_set_name() function prototype to

sysfs.h in this case, that should remove the error, right?
thanks,
greg k-h

--

That's what I did first. But I thought it was worse than the above

solution.

Anyway, adding kobject_set_name() to sysfs.h did fix the error.
----------------------------------------------------------------

This message was sent using IMP, the Internet Messaging Program.
--

I figure sorting out the headers is worth while, but that is

another project.
Because kobject.h includes sysfs.h we can't get the declarations

out of sync which is the really important part.
Eric

--

When looking at kobject_rename I found two bugs with

that exist when sysfs support is disabled in the kernel.
kobject_rename does not change the name on the kobject when

sysfs support is not compiled in.
kobject_rename without locking attempts to check the

validity of a rename operation, which the kobject layer

simply does not have the infrastructure to do.
This patch documents the previously unstated requirement of

kobject_rename that is the responsibility of the caller to

provide mutual exclusion and to be certain that the new_name

for the kobject is valid.
This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case

to call kobject_set_name to actually change the kobject_name.
This patch removes the bogus and misleading check in kobject_rename

that attempts to see if a rename is valid.  The check is bogus

because we do not have the proper locking.  The check is misleading

because it looks like we can and do perform checking at the kobject

level that we don't.
Changelog:

v2:	Added a declaration of kboject_set_name to sysfs.h

	so the code actually compiles with !CONFIG_SYSFS.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

---

 Documentation/kobject.txt |    4 ++++

 drivers/base/core.c       |    5 +++++

 include/linux/sysfs.h     |    4 +++-

 lib/kobject.c             |   18 +++++-------------

 4 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/Documentation/kobject.txt b/Documentation/kobject.txt

index bf3256e..79184b4 100644

--- a/Documentation/kobject.txt

+++ b/Documentation/kobject.txt

@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
     int kobject_rename(struct kobject *kobj, const char *new_name);
+Note kobject_rename does perform any locking or have a solid notion of

+what names are valid so the provide must provide their own sanity checking

+and serialization.

+

 There is a function called kobject_set_name() but that is legacy cruft and

 is being removed.  If your code needs to call this functi...
[ message continues ]

<snip>
Better, but can you address Randy's objections?
thanks,
greg k-h

--

When looking at kobject_rename I found two bugs with

that exist when sysfs support is disabled in the kernel.
kobject_rename does not change the name on the kobject when

sysfs support is not compiled in.
kobject_rename without locking attempts to check the

validity of a rename operation, which the kobject layer

simply does not have the infrastructure to do.
This patch documents the previously unstated requirement of

kobject_rename that is the responsibility of the caller to

provide mutual exclusion and to be certain that the new_name

for the kobject is valid.
This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case

to call kobject_set_name to actually change the kobject_name.
This patch removes the bogus and misleading check in kobject_rename

that attempts to see if a rename is valid.  The check is bogus

because we do not have the proper locking.  The check is misleading

because it looks like we can and do perform checking at the kobject

level that we don't.
Changelog:

v3:	Documentation typo fixes
v2:	Added a declaration of kboject_set_name to sysfs.h

	so the code actually compiles with !CONFIG_SYSFS.
	Unscrambling the header dependencies so everything looks

	beautiful is a project for another day.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

---

 Documentation/kobject.txt |    4 ++++

 drivers/base/core.c       |    5 +++++

 include/linux/sysfs.h     |    4 +++-

 lib/kobject.c             |   18 +++++-------------

 4 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/Documentation/kobject.txt b/Documentation/kobject.txt

index bf3256e..79184b4 100644

--- a/Documentation/kobject.txt

+++ b/Documentation/kobject.txt

@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
     int kobject_rename(struct kobject *kobj, const char *new_name);
+Note kobject_rename does perform any locking or have a solid notion of

+what names are valid so the provide must provide their own sanity checking

+and serialization.

+

...
[ message continues ]

Inserted/attached wrong patch file?
--

When looking at kobject_rename I found two bugs with

that exist when sysfs support is disabled in the kernel.
kobject_rename does not change the name on the kobject when

sysfs support is not compiled in.
kobject_rename without locking attempts to check the

validity of a rename operation, which the kobject layer

simply does not have the infrastructure to do.
This patch documents the previously unstated requirement of

kobject_rename that is the responsibility of the caller to

provide mutual exclusion and to be certain that the new_name

for the kobject is valid.
This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case

to call kobject_set_name to actually change the kobject_name.
This patch removes the bogus and misleading check in kobject_rename

that attempts to see if a rename is valid.  The check is bogus

because we do not have the proper locking.  The check is misleading

because it looks like we can and do perform checking at the kobject

level that we don't.
Changelog:

v4:	Documentation typo fixes
v2:	Added a declaration of kboject_set_name to sysfs.h

	so the code actually compiles with !CONFIG_SYSFS.
	Unscrambling the header dependencies so everything looks

	beautiful is a project for another day.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

---

 Documentation/kobject.txt |    4 ++++

 drivers/base/core.c       |    5 +++++

 include/linux/sysfs.h     |    4 +++-

 lib/kobject.c             |   18 +++++-------------

 4 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/Documentation/kobject.txt b/Documentation/kobject.txt

index bf3256e..ac80d82 100644

--- a/Documentation/kobject.txt

+++ b/Documentation/kobject.txt

@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
     int kobject_rename(struct kobject *kobj, const char *new_name);
+Note kobject_rename does not perform any locking or have a solid notion of

+what names are valid so the caller must provide their own sanity checking

+and serialization.

...
[ message continues ]

Duplicating the kobject_set_name() declaration in sysfs.h is rather a hack.
It'd be better to move it into a new header file, included by both

sysfs.h and kobject.h.  Perhaps there are other declarations which can

be moved with it.
--

My gut feel says that sysfs.h should include kobject.h instead of the

other way around.
However it gets reorganized, it is an entirely separate problem

from the one this patch sets out to solve and so should go in

a different patch.
Eric
--

umm, well, actually, it's a problem which your patch introduces, by adding a

new dependency.
uninlining sysfs_rename_dir() would be a sensible solution.

--

This is also my feeling.

To do this, we need to move "struct attributes" related definitions to

kboject.h, and fix some .c files that don't include sysfs.h as they

should (sysfs.h was included indirectly via kojbect.h in these files: eg 
It is inlined only when CONFIG_SYSFS=n. When sysfs is enabled

sysfs_rename_dir() is compiled from fs/sysfs/dir.c
Uninlining it will require us to find an appropriate .c file to put it

in: we can't put it in fs/sysfs/dir.c. It is not built if CONFIG_SYSFS

is disabled.
-Benjamin
--

B e n j a m i n   T h e r y  - BULL/DT/Open Software R&D
    http://www.bull.com

--

Well yes, that's the problem.  I stuck it in lib/kobject.c but ugh.
I guess we could make it a macro and quickly run away.

--

~~~~~~~
--

~Randy

--

Good catch.
Thanks Randy.
Eric

--

device_rename only performs useful and race free validity

checking at the optional sysfs level so depending on it

for all of the validity checking in cfg80211_dev_rename

is racy.
Instead implement all of the needed validity checking

and locking in cfg80211_dev_rename.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

---

 net/wireless/core.c |   33 ++++++++++++++++++++++++++++-----

 1 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/net/wireless/core.c b/net/wireless/core.c

index 80afacd..f1da0b9 100644

--- a/net/wireless/core.c

+++ b/net/wireless/core.c

@@ -143,8 +143,11 @@ void cfg80211_put_dev(struct cfg80211_registered_device *drv)

 int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,

 			char *newname)

 {

+	struct cfg80211_registered_device *drv;

 	int idx, taken = -1, result, digits;
+	mutex_lock(&cfg80211_drv_mutex);

+

 	/* prohibit calling the thing phy%d when %d is not its number */

 	sscanf(newname, PHY_NAME "%d%n", &idx, &taken);

 	if (taken == strlen(newname) && idx != rdev->idx) {

@@ -156,14 +159,30 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,

 		 * deny the name if it is phy<idx> where <idx> is printed

 		 * without leading zeroes. taken == strlen(newname) here

 		 */

+		result = -EINVAL;

 		if (taken == strlen(PHY_NAME) + digits)

-			return -EINVAL;

+			goto out_unlock;

+	}

+

+

+	/* Ignore nop renames */

+	result = 0;

+	if (strcmp(newname, dev_name(&rdev->wiphy.dev)) == 0)

+		goto out_unlock;

+

+	/* Ensure another device does not already have this name. */

+	list_for_each_entry(drv, &cfg80211_drv_list, list) {

+		result = -EINVAL;

+		if (strcmp(newname, dev_name(&drv->wiphy.dev)) == 0)

+			goto out_unlock;

 	}
-	/* this will check for collisions */

+	/* this will only check for collisions in sysfs

+	 * which is not even always compiled in.

+	 */

 	result = device_rename(&rdev->wiphy.dev, newname);

 	if (result)

-		...
[ message continues ]

Makes sense, thanks, I didn't really think about it not being compiled

I'm about as far as you can get from either a sysfs/kobject or network

expert, but both of these patches as well as your general concept that

renames should be handled at sysfs seem to me to make perfect sense.
thanks,

--

Looking at this a little further kobject_rename is a complete noop

when sysfs support is not compiled in.  That is the kobject does not

get renamed, even if the higher level objects do.
This makes the wireless dependency on an error code from kobject_rename

completely bogus as the kobject layer is not prepared to give any kind of

reliable result, and it makes kobject_rename completely bogus if sysfs

support is not compiled in.
Further there is no locking to guarantee renames are atomic

or mutually exclusive at the kobject level.
With no locking and code that does nothing in the absence of sysfs

attempting to check renames for validity at the kobject level (when

renames don't happen at the kobject level) is totally bogus.
Since renames don't happen at the kobject level checking them for

sanity at the kobject level makes no sense.
Eric
--

Well yes we are dealing with a pile of seemingly unnecessary layers,

that are attempting to add uniformity where no uniformity existed.
Looks like it, that use is brand new, and at first glance I thought

it was another instance of the device_rename case that kobject uses.

Apparently not.
It looks like getting the wireless devices into the network namespace

is going to be interesting.  Since this phy name is user controllable

and shows up in rtnetlink messages it definitely appears to belong in

the network namespace. 
Virtual/logical devices are such a pain.
....
Given that kobject_rename is growing users we definitely need to

fix it so a noop rename does not return -EINVAL.
i.e. if (temp_kobj && (temp_kobj != kobj)) return -EINVAL.
That is the device_move -> kobject_move case.  Very similar (and nice
Thanks.  It looks like we are just about there.
Hopefully we can get this merged soon so there won't be many more sets

of shifting requirements to chase.
Eric

--

On Wed, 07 May 2008 13:54:27 -0700,
Sounds like a good idea. I can test the _move() stuff (after I've

managed to find some time to try this patchset...)

--

Do you remember if we have ever sorted out the race between _move

and module unload/directory teardown at the sysfs level?
Eric

--

On Thu, 08 May 2008 12:28:19 -0700,
Unfortunately I can't remember anything about this, do you have a

pointer to the original discussion?

--

Ok the conversation was:

"[Bluez-devel] Oops involving RFCOMM and sysfs"

http://lkml.org/lkml/2007/12/28/87
Following the thread through it appears the bug was fixed

and it wasn't the race Al Viro was concerned about.

http://lkml.org/lkml/2008/1/9/59
And it looks like all of the symptoms have been corrected.

Yeah!
Which implies to me that kobject_move has the same locking

restrictions that kobject_rename has.  I.e. the upper level

better do what it takes to make the operation safe.
Which says that at least for now not worrying about the locking

between sysfs_move_dir/sysfs_rename_dir and sysfs_remove_dir is ok.

As we require the upper levels not to do that.
Which says that we may be able to figure out how to merge

sysfs_move_dir and sysfs_rename_dir.  It is still a nasty business

though.
Eric
--