linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 00/10] net: fix sysfs permssions when device changes network
@ 2020-02-12 10:43 Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 01/10] sysfs: add sysfs_file_change_owner() Christian Brauner
                   ` (10 more replies)
  0 siblings, 11 replies; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Hey everyone,

(I've tagged this with net-next since it's triggered by a bug for
 network device files but it also touches driver core aspects so it's
 not clear-cut. I can of course split this series into separate
 patchsets.) 
We have been struggling with a bug surrounding the ownership of network
device sysfs files when moving network devices between network
namespaces owned by different user namespaces reported by multiple
users.

Currently, when moving network devices between network namespaces the
ownership of the corresponding sysfs entries is not changed. This leads
to problems when tools try to operate on the corresponding sysfs files.

I also causes a bug when creating a network device in a network
namespaces owned by a user namespace and moving that network device back
to the host network namespaces. Because when a network device is created
in a network namespaces it will be owned by the root user of the user
namespace and all its associated sysfs files will also be owned by the
root user of the corresponding user namespace.
If such a network device has to be moved back to the host network
namespace the permissions will still be set to the root user of the
owning user namespaces of the originating network namespace. This means
unprivileged users can e.g. re-trigger uevents for such incorrectly
owned devices on the host or in other network namespaces. They can also
modify the settings of the device itself through sysfs when they
wouldn't be able to do the same through netlink. Both of these things
are unwanted.

For example, quite a few workloads will create network devices in the
host network namespace. Other tools will then proceed to move such
devices between network namespaces owner by other user namespaces. While
the ownership of the device itself is updated in
net/core/net-sysfs.c:dev_change_net_namespace() the corresponding sysfs
entry for the device is not. Below you'll find that moving a network
device (here a veth device) from a network namespace into another
network namespaces owned by a different user namespace with a different
id mapping. As you can see the permissions are wrong even though it is
owned by the userns root user after it has been moved and can be
interacted with through netlink: 

drwxr-xr-x 5 nobody nobody    0 Jan 25 18:08 .
drwxr-xr-x 9 nobody nobody    0 Jan 25 18:08 ..
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 addr_assign_type
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 addr_len
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 address
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 broadcast
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 carrier
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 carrier_changes
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 carrier_down_count
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 carrier_up_count
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 dev_id
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 dev_port
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 dormant
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 duplex
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 flags
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 gro_flush_timeout
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 ifalias
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 ifindex
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 iflink
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 link_mode
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 mtu
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 name_assign_type
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 netdev_group
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 operstate
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 phys_port_id
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 phys_port_name
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 phys_switch_id
drwxr-xr-x 2 nobody nobody    0 Jan 25 18:09 power
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 proto_down
drwxr-xr-x 4 nobody nobody    0 Jan 25 18:09 queues
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 speed
drwxr-xr-x 2 nobody nobody    0 Jan 25 18:09 statistics
lrwxrwxrwx 1 nobody nobody    0 Jan 25 18:08 subsystem -> ../../../../class/net
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 tx_queue_len
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 type
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:08 uevent

Constrast this with creating a device of the same type in the network
namespace directly. In this case the device's sysfs permissions will be
correctly updated.
(Please also note, that in a lot of workloads this strategy of creating
 the network device directly in the network device to workaround this
 issue can not be used. Either because the network device is dedicated
 after it has been created or because it used by a process that is
 heavily sandboxed and couldn't create network devices itself.):

drwxr-xr-x 5 root   root      0 Jan 25 18:12 .
drwxr-xr-x 9 nobody nobody    0 Jan 25 18:08 ..
-r--r--r-- 1 root   root   4096 Jan 25 18:12 addr_assign_type
-r--r--r-- 1 root   root   4096 Jan 25 18:12 addr_len
-r--r--r-- 1 root   root   4096 Jan 25 18:12 address
-r--r--r-- 1 root   root   4096 Jan 25 18:12 broadcast
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 carrier
-r--r--r-- 1 root   root   4096 Jan 25 18:12 carrier_changes
-r--r--r-- 1 root   root   4096 Jan 25 18:12 carrier_down_count
-r--r--r-- 1 root   root   4096 Jan 25 18:12 carrier_up_count
-r--r--r-- 1 root   root   4096 Jan 25 18:12 dev_id
-r--r--r-- 1 root   root   4096 Jan 25 18:12 dev_port
-r--r--r-- 1 root   root   4096 Jan 25 18:12 dormant
-r--r--r-- 1 root   root   4096 Jan 25 18:12 duplex
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 flags
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 gro_flush_timeout
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 ifalias
-r--r--r-- 1 root   root   4096 Jan 25 18:12 ifindex
-r--r--r-- 1 root   root   4096 Jan 25 18:12 iflink
-r--r--r-- 1 root   root   4096 Jan 25 18:12 link_mode
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 mtu
-r--r--r-- 1 root   root   4096 Jan 25 18:12 name_assign_type
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 netdev_group
-r--r--r-- 1 root   root   4096 Jan 25 18:12 operstate
-r--r--r-- 1 root   root   4096 Jan 25 18:12 phys_port_id
-r--r--r-- 1 root   root   4096 Jan 25 18:12 phys_port_name
-r--r--r-- 1 root   root   4096 Jan 25 18:12 phys_switch_id
drwxr-xr-x 2 root   root      0 Jan 25 18:12 power
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 proto_down
drwxr-xr-x 4 root   root      0 Jan 25 18:12 queues
-r--r--r-- 1 root   root   4096 Jan 25 18:12 speed
drwxr-xr-x 2 root   root      0 Jan 25 18:12 statistics
lrwxrwxrwx 1 nobody nobody    0 Jan 25 18:12 subsystem -> ../../../../class/net
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 tx_queue_len
-r--r--r-- 1 root   root   4096 Jan 25 18:12 type
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 uevent

Now, when creating a network device in a network namespace owned by a
user namespace and moving it to the host the permissions will be set to
the id that the user namespace root user has been mapped to on the host
leading to all sorts of permission issues mentioned above:

458752
drwxr-xr-x 5 458752 458752      0 Jan 25 18:12 .
drwxr-xr-x 9 root   root        0 Jan 25 18:08 ..
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 addr_assign_type
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 addr_len
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 address
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 broadcast
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 carrier
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 carrier_changes
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 carrier_down_count
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 carrier_up_count
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 dev_id
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 dev_port
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 dormant
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 duplex
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 flags
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 gro_flush_timeout
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 ifalias
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 ifindex
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 iflink
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 link_mode
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 mtu
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 name_assign_type
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 netdev_group
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 operstate
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 phys_port_id
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 phys_port_name
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 phys_switch_id
drwxr-xr-x 2 458752 458752      0 Jan 25 18:12 power
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 proto_down
drwxr-xr-x 4 458752 458752      0 Jan 25 18:12 queues
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 speed
drwxr-xr-x 2 458752 458752      0 Jan 25 18:12 statistics
lrwxrwxrwx 1 root   root        0 Jan 25 18:12 subsystem -> ../../../../class/net
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 tx_queue_len
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 type
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 uevent

Fix this by changing the basic sysfs files associated with network
devices when moving them between network namespaces. To this end we add
some infrastructure to sysfs.

The patchset takes care to only do this when the owning user namespaces
changes and the kids differ. So there's only a performance overhead,
when the owning user namespace of the network namespace is different
__and__ the kid mappings for the root user are different for the two
user namespaces:
Assume we have a netdev eth0 which we create in netns1 owned by userns1.
userns1 has an id mapping of 0 100000 100000. Now we move eth0 into
netns2 which is owned by userns2 which also defines an id mapping of 0
100000 100000. In this case sysfs doesn't need updating. The patch will
handle this case and not do any needless work. Now assume eth0 is moved
into netns3 which is owned by userns3 which defines an id mapping of 0
123456 65536. In this case the root user in each namespace corresponds
to different kid and sysfs needs updating.

Thanks!
Christian

Christian Brauner (10):
  sysfs: add sysfs_file_change_owner()
  sysfs: add sysfs_link_change_owner()
  sysfs: add sysfs_group_change_owner()
  sysfs: add sysfs_groups_change_owner()
  sysfs: add sysfs_change_owner()
  device: add device_change_owner()
  drivers/base/power: add dpm_sysfs_change_owner()
  net-sysfs: add netdev_change_owner()
  net-sysfs: add queue_change_owner()
  net: fix sysfs permssions when device changes network namespace

 drivers/base/core.c        |  77 ++++++++++++++++++++++
 drivers/base/power/power.h |   2 +
 drivers/base/power/sysfs.c |  37 +++++++++++
 fs/sysfs/file.c            | 119 ++++++++++++++++++++++++++++++++++
 fs/sysfs/group.c           | 113 ++++++++++++++++++++++++++++++++
 include/linux/device.h     |   1 +
 include/linux/sysfs.h      |  38 +++++++++++
 net/core/dev.c             |   9 ++-
 net/core/net-sysfs.c       | 128 +++++++++++++++++++++++++++++++++++++
 net/core/net-sysfs.h       |   2 +
 10 files changed, 525 insertions(+), 1 deletion(-)


base-commit: bb6d3fb354c5ee8d6bde2d576eb7220ea09862b9
-- 
2.25.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH net-next 01/10] sysfs: add sysfs_file_change_owner()
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 13:19   ` Greg Kroah-Hartman
  2020-02-12 10:43 ` [PATCH net-next 02/10] sysfs: add sysfs_link_change_owner() Christian Brauner
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Add a helper to change the owner of a sysfs file.
The ownership of a sysfs object is determined based on the ownership of
the corresponding kobject, i.e. only if the ownership of a kobject is
changed will this function change the ownership of the corresponding
sysfs entry.
This function will be used to correctly account for kobject ownership
changes, e.g. when moving network devices between network namespaces.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/sysfs/file.c       | 46 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  7 +++++++
 2 files changed, 53 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 130fc6fbcc03..007b97ca8165 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -558,3 +558,49 @@ void sysfs_remove_bin_file(struct kobject *kobj,
 	kernfs_remove_by_name(kobj->sd, attr->attr.name);
 }
 EXPORT_SYMBOL_GPL(sysfs_remove_bin_file);
+
+static int internal_change_owner(struct kernfs_node *kn, struct kobject *kobj)
+{
+	kuid_t uid;
+	kgid_t gid;
+	struct iattr newattrs = {
+		.ia_valid = ATTR_UID | ATTR_GID,
+	};
+
+	kobject_get_ownership(kobj, &uid, &gid);
+	newattrs.ia_uid = uid;
+	newattrs.ia_gid = gid;
+
+	return kernfs_setattr(kn, &newattrs);
+}
+
+/**
+ *	sysfs_file_change_owner - change owner of a file.
+ *	@kobj:	object.
+ *	@name:	name of the file to change.
+ *	        can be NULL to change current file.
+ */
+int sysfs_file_change_owner(struct kobject *kobj, const char *name)
+{
+	struct kernfs_node *kn;
+	int error;
+
+	if (!kobj->state_in_sysfs)
+		return -EINVAL;
+
+	if (name) {
+		kn = kernfs_find_and_get(kobj->sd, name);
+	} else {
+		kernfs_get(kobj->sd);
+		kn = kobj->sd;
+	}
+	if (!kn)
+		return -ENOENT;
+
+	error = internal_change_owner(kn, kobj);
+
+	kernfs_put(kn);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index fa7ee503fb76..58fa71d47c7f 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -310,6 +310,8 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn)
 	return kernfs_enable_ns(kn);
 }
 
+int sysfs_file_change_owner(struct kobject *kobj, const char *name);
+
 #else /* CONFIG_SYSFS */
 
 static inline int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
@@ -522,6 +524,11 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn)
 {
 }
 
+static inline int sysfs_file_change_owner(struct kobject *kobj, const char *name)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next 02/10] sysfs: add sysfs_link_change_owner()
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 01/10] sysfs: add sysfs_file_change_owner() Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 03/10] sysfs: add sysfs_group_change_owner() Christian Brauner
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Add a helper to change the owner of a sysfs link.
The ownership of a sysfs object is determined based on the ownership of
the corresponding kobject, i.e. only if the ownership of a kobject is
changed will this function change the ownership of the corresponding
sysfs entry.
This function will be used to correctly account for kobject ownership
changes, e.g. when moving network devices between network namespaces.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/sysfs/file.c       | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  9 +++++++++
 2 files changed, 47 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 007b97ca8165..6239d9584f0b 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -574,6 +574,44 @@ static int internal_change_owner(struct kernfs_node *kn, struct kobject *kobj)
 	return kernfs_setattr(kn, &newattrs);
 }
 
+/**
+ *	sysfs_link_change_owner - change owner of a link.
+ *	@kobj:	object of the kernfs_node the symlink is located in.
+ *	@targ:	object of the kernfs_node the symlink points to.
+ *	@name:	name of the link.
+ */
+int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
+			    const char *name)
+{
+	struct kernfs_node *parent, *kn = NULL;
+	int error;
+
+	if (!kobj)
+		parent = sysfs_root_kn;
+	else
+		parent = kobj->sd;
+
+	if (!targ->state_in_sysfs)
+		return -EINVAL;
+
+	error = -ENOENT;
+	kn = kernfs_find_and_get_ns(parent, name, targ->sd->ns);
+	if (!kn)
+		goto out;
+
+	error = -EINVAL;
+	if (kernfs_type(kn) != KERNFS_LINK)
+		goto out;
+	if (kn->symlink.target_kn->priv != targ)
+		goto out;
+
+	error = internal_change_owner(kn, targ);
+
+out:
+	kernfs_put(kn);
+	return error;
+}
+
 /**
  *	sysfs_file_change_owner - change owner of a file.
  *	@kobj:	object.
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 58fa71d47c7f..4e10085739b7 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -311,6 +311,8 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn)
 }
 
 int sysfs_file_change_owner(struct kobject *kobj, const char *name);
+int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
+			    const char *name);
 
 #else /* CONFIG_SYSFS */
 
@@ -529,6 +531,13 @@ static inline int sysfs_file_change_owner(struct kobject *kobj, const char *name
 	return 0;
 }
 
+static inline int sysfs_link_change_owner(struct kobject *kobj,
+					  struct kobject *targ,
+					  const char *name)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next 03/10] sysfs: add sysfs_group_change_owner()
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 01/10] sysfs: add sysfs_file_change_owner() Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 02/10] sysfs: add sysfs_link_change_owner() Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 04/10] sysfs: add sysfs_groups_change_owner() Christian Brauner
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Add a helper to change the owner of a sysfs group.
The ownership of a sysfs object is determined based on the ownership of
the corresponding kobject, i.e. only if the ownership of a kobject is
changed will this function change the ownership of the corresponding
sysfs entry.
This function will be used to correctly account for kobject ownership
changes, e.g. when moving network devices between network namespaces.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/sysfs/group.c      | 85 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  8 ++++
 2 files changed, 93 insertions(+)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index c4ab045926b7..34b702ec9782 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -13,6 +13,7 @@
 #include <linux/dcache.h>
 #include <linux/namei.h>
 #include <linux/err.h>
+#include <linux/fs.h>
 #include "sysfs.h"
 
 
@@ -457,3 +458,87 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
 	return PTR_ERR_OR_ZERO(link);
 }
 EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
+
+static int sysfs_group_attrs_change_owner(struct kernfs_node *grp_kn,
+					  const struct attribute_group *grp,
+					  struct iattr *newattrs)
+{
+	struct kernfs_node *kn;
+	int error;
+
+	if (grp->attrs) {
+		struct attribute *const *attr;
+
+		for (attr = grp->attrs; *attr; attr++) {
+			kn = kernfs_find_and_get(grp_kn, (*attr)->name);
+			if (!kn)
+				return -ENOENT;
+
+			error = kernfs_setattr(kn, newattrs);
+			kernfs_put(kn);
+			if (error)
+				return error;
+		}
+	}
+
+	if (grp->bin_attrs) {
+		struct bin_attribute *const *bin_attr;
+
+		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
+			kn = kernfs_find_and_get(grp_kn, (*bin_attr)->attr.name);
+			if (!kn)
+				return -ENOENT;
+
+			error = kernfs_setattr(kn, newattrs);
+			kernfs_put(kn);
+			if (error)
+				return error;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * sysfs_group_change_owner - change owner of an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @grp:	The attribute group.
+ *
+ * Returns 0 on success or error code on failure.
+ */
+int sysfs_group_change_owner(struct kobject *kobj,
+			     const struct attribute_group *grp)
+{
+	struct kernfs_node *grp_kn;
+	kuid_t uid;
+	kgid_t gid;
+	int error;
+	struct iattr newattrs = {
+		.ia_valid = ATTR_UID | ATTR_GID,
+	};
+
+	if (!kobj->state_in_sysfs)
+		return -EINVAL;
+
+	if (grp->name) {
+		grp_kn = kernfs_find_and_get(kobj->sd, grp->name);
+	} else {
+		kernfs_get(kobj->sd);
+		grp_kn = kobj->sd;
+	}
+	if (!grp_kn)
+		return -ENOENT;
+
+	kobject_get_ownership(kobj, &uid, &gid);
+	newattrs.ia_uid = uid;
+	newattrs.ia_gid = gid;
+
+	error = kernfs_setattr(grp_kn, &newattrs);
+	if (!error)
+		error = sysfs_group_attrs_change_owner(grp_kn, grp, &newattrs);
+
+	kernfs_put(grp_kn);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(sysfs_group_change_owner);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 4e10085739b7..9363c61b9349 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -313,6 +313,8 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn)
 int sysfs_file_change_owner(struct kobject *kobj, const char *name);
 int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
 			    const char *name);
+int sysfs_group_change_owner(struct kobject *kobj,
+			     const struct attribute_group *groups);
 
 #else /* CONFIG_SYSFS */
 
@@ -538,6 +540,12 @@ static inline int sysfs_link_change_owner(struct kobject *kobj,
 	return 0;
 }
 
+static inline int sysfs_group_change_owner(struct kobject *kobj,
+			 const struct attribute_group **groups)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next 04/10] sysfs: add sysfs_groups_change_owner()
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
                   ` (2 preceding siblings ...)
  2020-02-12 10:43 ` [PATCH net-next 03/10] sysfs: add sysfs_group_change_owner() Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 05/10] sysfs: add sysfs_change_owner() Christian Brauner
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Add a helper to change the owner of a set of sysfs groups file.
The ownership of a sysfs object is determined based on the ownership of
the corresponding kobject, i.e. only if the ownership of a kobject is
changed will this function change the ownership of the corresponding
sysfs entry.
This function will be used to correctly account for kobject ownership
changes, e.g. when moving network devices between network namespaces.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/sysfs/group.c      | 28 ++++++++++++++++++++++++++++
 include/linux/sysfs.h |  8 ++++++++
 2 files changed, 36 insertions(+)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 34b702ec9782..4c9de5d972bd 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -542,3 +542,31 @@ int sysfs_group_change_owner(struct kobject *kobj,
 	return error;
 }
 EXPORT_SYMBOL_GPL(sysfs_group_change_owner);
+
+/**
+ * sysfs_groups_change_owner - change owner of a set of attribute groups.
+ * @kobj:	The kobject containing the groups.
+ * @groups:	The attribute groups.
+ *
+ * Returns 0 on success or error code on failure.
+ */
+int sysfs_groups_change_owner(struct kobject *kobj,
+			      const struct attribute_group **groups)
+{
+	int error = 0, i;
+
+	if (!kobj->state_in_sysfs)
+		return -EINVAL;
+
+	if (!groups)
+		return 0;
+
+	for (i = 0; groups[i]; i++) {
+		error = sysfs_group_change_owner(kobj, groups[i]);
+		if (error)
+			break;
+	}
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(sysfs_groups_change_owner);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9363c61b9349..3b9770c5ecb7 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -313,6 +313,8 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn)
 int sysfs_file_change_owner(struct kobject *kobj, const char *name);
 int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
 			    const char *name);
+int sysfs_groups_change_owner(struct kobject *kobj,
+			      const struct attribute_group **groups);
 int sysfs_group_change_owner(struct kobject *kobj,
 			     const struct attribute_group *groups);
 
@@ -540,6 +542,12 @@ static inline int sysfs_link_change_owner(struct kobject *kobj,
 	return 0;
 }
 
+static inline int sysfs_groups_change_owner(struct kobject *kobj,
+			  const struct attribute_group **groups)
+{
+	return 0;
+}
+
 static inline int sysfs_group_change_owner(struct kobject *kobj,
 			 const struct attribute_group **groups)
 {
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next 05/10] sysfs: add sysfs_change_owner()
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
                   ` (3 preceding siblings ...)
  2020-02-12 10:43 ` [PATCH net-next 04/10] sysfs: add sysfs_groups_change_owner() Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 13:18   ` Greg Kroah-Hartman
  2020-02-12 10:43 ` [PATCH net-next 06/10] device: add device_change_owner() Christian Brauner
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Add a helper to change the owner of sysfs objects.
The ownership of a sysfs object is determined based on the ownership of
the corresponding kobject, i.e. only if the ownership of a kobject is
changed will this function change the ownership of the corresponding
sysfs entry.
This function will be used to correctly account for kobject ownership
changes, e.g. when moving network devices between network namespaces.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/sysfs/file.c       | 35 +++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  6 ++++++
 2 files changed, 41 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 6239d9584f0b..6a0fe88061fd 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -642,3 +642,38 @@ int sysfs_file_change_owner(struct kobject *kobj, const char *name)
 	return error;
 }
 EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
+
+/**
+ *	sysfs_change_owner - change owner of the given object.
+ *	@kobj:	object.
+ */
+int sysfs_change_owner(struct kobject *kobj)
+{
+	int error;
+	const struct kobj_type *ktype;
+
+	if (!kobj->state_in_sysfs)
+		return -EINVAL;
+
+	error = sysfs_file_change_owner(kobj, NULL);
+	if (error)
+		return error;
+
+	ktype = get_ktype(kobj);
+	if (ktype) {
+		struct attribute **kattr;
+
+		for (kattr = ktype->default_attrs; kattr && *kattr; kattr++) {
+			error = sysfs_file_change_owner(kobj, (*kattr)->name);
+			if (error)
+				return error;
+		}
+
+		error = sysfs_groups_change_owner(kobj, ktype->default_groups);
+		if (error)
+			return error;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(sysfs_change_owner);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 3b9770c5ecb7..b9ce60261e38 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -310,6 +310,7 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn)
 	return kernfs_enable_ns(kn);
 }
 
+int sysfs_change_owner(struct kobject *kobj);
 int sysfs_file_change_owner(struct kobject *kobj, const char *name);
 int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
 			    const char *name);
@@ -542,6 +543,11 @@ static inline int sysfs_link_change_owner(struct kobject *kobj,
 	return 0;
 }
 
+static inline int sysfs_change_owner(struct kobject *kobj)
+{
+	return 0;
+}
+
 static inline int sysfs_groups_change_owner(struct kobject *kobj,
 			  const struct attribute_group **groups)
 {
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next 06/10] device: add device_change_owner()
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
                   ` (4 preceding siblings ...)
  2020-02-12 10:43 ` [PATCH net-next 05/10] sysfs: add sysfs_change_owner() Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 07/10] drivers/base/power: add dpm_sysfs_change_owner() Christian Brauner
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Add a helper to change the owner of a device's sysfs entries. This
needs to happen when the ownership of a device is changed, e.g. when
moving network devices between network namespaces.
The ownership of a device is determined based on the ownership of
the corresponding kobject, i.e. only if the ownership of a kobject is
changed will this function change the ownership of the corresponding
device's sysfs entries.
This function will be used to correctly account for ownership changes,
e.g. when moving network devices between network namespaces.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 drivers/base/core.c    | 73 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  1 +
 2 files changed, 74 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 42a672456432..262217287a09 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3458,6 +3458,79 @@ int device_move(struct device *dev, struct device *new_parent,
 }
 EXPORT_SYMBOL_GPL(device_move);
 
+static int device_attrs_change_owner(struct device *dev)
+{
+	struct kobject *kobj = &dev->kobj;
+	struct class *class = dev->class;
+	const struct device_type *type = dev->type;
+	int error;
+
+	if (class) {
+		error = sysfs_groups_change_owner(kobj, class->dev_groups);
+		if (error)
+			return error;
+	}
+
+	if (type) {
+		error = sysfs_groups_change_owner(kobj, type->groups);
+		if (error)
+			return error;
+	}
+
+	error = sysfs_groups_change_owner(kobj, dev->groups);
+	if (error)
+		return error;
+
+	if (device_supports_offline(dev) && !dev->offline_disabled) {
+		error = sysfs_file_change_owner(kobj, dev_attr_online.attr.name);
+		if (error)
+			return error;
+	}
+
+	return 0;
+}
+
+/**
+ * device_change_owner - change the owner of an existing device.
+ * @dev: device.
+ */
+int device_change_owner(struct device *dev)
+{
+	int error;
+	struct kobject *kobj = &dev->kobj;
+
+	dev = get_device(dev);
+	if (!dev)
+		return -EINVAL;
+
+	error = sysfs_change_owner(kobj);
+	if (error)
+		goto out;
+
+	error = sysfs_file_change_owner(kobj, dev_attr_uevent.attr.name);
+	if (error)
+		goto out;
+
+	error = device_attrs_change_owner(dev);
+	if (error)
+		goto out;
+
+#ifdef CONFIG_BLOCK
+	if (sysfs_deprecated && dev->class == &block_class)
+		goto out;
+#endif
+
+	error = sysfs_link_change_owner(&dev->class->p->subsys.kobj, &dev->kobj,
+					dev_name(dev));
+	if (error)
+		goto out;
+
+out:
+	put_device(dev);
+	return error;
+}
+EXPORT_SYMBOL_GPL(device_change_owner);
+
 /**
  * device_shutdown - call ->shutdown() on each device to shutdown.
  */
diff --git a/include/linux/device.h b/include/linux/device.h
index 0cd7c647c16c..63ebd9e3f6b0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -817,6 +817,7 @@ extern struct device *device_find_child_by_name(struct device *parent,
 extern int device_rename(struct device *dev, const char *new_name);
 extern int device_move(struct device *dev, struct device *new_parent,
 		       enum dpm_order dpm_order);
+extern int device_change_owner(struct device *dev);
 extern const char *device_get_devnode(struct device *dev,
 				      umode_t *mode, kuid_t *uid, kgid_t *gid,
 				      const char **tmp);
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next 07/10] drivers/base/power: add dpm_sysfs_change_owner()
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
                   ` (5 preceding siblings ...)
  2020-02-12 10:43 ` [PATCH net-next 06/10] device: add device_change_owner() Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 10:52   ` Rafael J. Wysocki
  2020-02-12 10:43 ` [PATCH net-next 08/10] net-sysfs: add netdev_change_owner() Christian Brauner
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Add a helper to change the owner of a device's power entries. This
needs to happen when the ownership of a device is changed, e.g. when
moving network devices between network namespaces.
The ownership of a device's power entries is determined based on the
ownership of the corresponding kobject, i.e. only if the ownership of a
kobject is changed will this function change the ownership of the
corresponding sysfs entries.
This function will be used to correctly account for ownership changes,
e.g. when moving network devices between network namespaces.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 drivers/base/core.c        |  4 ++++
 drivers/base/power/power.h |  2 ++
 drivers/base/power/sysfs.c | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 262217287a09..dfaf6d3614fa 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3515,6 +3515,10 @@ int device_change_owner(struct device *dev)
 	if (error)
 		goto out;
 
+	error = dpm_sysfs_change_owner(dev);
+	if (error)
+		goto out;
+
 #ifdef CONFIG_BLOCK
 	if (sysfs_deprecated && dev->class == &block_class)
 		goto out;
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index 444f5c169a0b..f68490d0811b 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -74,6 +74,7 @@ extern int pm_qos_sysfs_add_flags(struct device *dev);
 extern void pm_qos_sysfs_remove_flags(struct device *dev);
 extern int pm_qos_sysfs_add_latency_tolerance(struct device *dev);
 extern void pm_qos_sysfs_remove_latency_tolerance(struct device *dev);
+extern int dpm_sysfs_change_owner(struct device *dev);
 
 #else /* CONFIG_PM */
 
@@ -88,6 +89,7 @@ static inline void pm_runtime_remove(struct device *dev) {}
 
 static inline int dpm_sysfs_add(struct device *dev) { return 0; }
 static inline void dpm_sysfs_remove(struct device *dev) {}
+static inline int dpm_sysfs_change_owner(struct device *dev) { return 0; }
 
 #endif
 
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d7d82db2e4bc..ce1fd346e854 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -684,6 +684,43 @@ int dpm_sysfs_add(struct device *dev)
 	return rc;
 }
 
+int dpm_sysfs_change_owner(struct device *dev)
+{
+	int rc;
+
+	if (device_pm_not_required(dev))
+		return 0;
+
+	rc = sysfs_group_change_owner(&dev->kobj, &pm_attr_group);
+	if (rc)
+		return rc;
+
+	if (pm_runtime_callbacks_present(dev)) {
+		rc = sysfs_group_change_owner(&dev->kobj,
+					      &pm_runtime_attr_group);
+		if (rc)
+			return rc;
+	}
+	if (device_can_wakeup(dev)) {
+		rc = sysfs_group_change_owner(&dev->kobj,
+					      &pm_wakeup_attr_group);
+		if (rc)
+			return rc;
+	}
+	if (dev->power.set_latency_tolerance) {
+		rc = sysfs_group_change_owner(&dev->kobj,
+				&pm_qos_latency_tolerance_attr_group);
+		if (rc)
+			return rc;
+	}
+	if (dev->power.wakeup && dev->power.wakeup->dev) {
+		rc = device_change_owner(dev->power.wakeup->dev);
+		if (rc)
+			return rc;
+	}
+	return 0;
+}
+
 int wakeup_sysfs_add(struct device *dev)
 {
 	return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next 08/10] net-sysfs: add netdev_change_owner()
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
                   ` (6 preceding siblings ...)
  2020-02-12 10:43 ` [PATCH net-next 07/10] drivers/base/power: add dpm_sysfs_change_owner() Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 09/10] net-sysfs: add queue_change_owner() Christian Brauner
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Add a function to change the owner of a network device when it is moved
between network namespaces.

Currently, when moving network devices between network namespaces the
ownership of the corresponding sysfs entries is not changed. This leads
to problems when tools try to operate on the corresponding sysfs files.
This leads to a bug whereby a network device that is created in a
network namespaces owned by a user namespace will have its corresponding
sysfs entry owned by the root user of the corresponding user namespace.
If such a network device has to be moved back to the host network
namespace the permissions will still be set to the user namespaces. This
means unprivileged users can e.g. trigger uevents for such incorrectly
owned devices. They can also modify the settings of the device itself.
Both of these things are unwanted.

For example, workloads will create network devices in the host network
namespace. Other tools will then proceed to move such devices between
network namespaces owner by other user namespaces. While the ownership
of the device itself is updated in
net/core/net-sysfs.c:dev_change_net_namespace() the corresponding sysfs
entry for the device is not:

drwxr-xr-x 5 nobody nobody    0 Jan 25 18:08 .
drwxr-xr-x 9 nobody nobody    0 Jan 25 18:08 ..
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 addr_assign_type
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 addr_len
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 address
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 broadcast
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 carrier
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 carrier_changes
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 carrier_down_count
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 carrier_up_count
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 dev_id
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 dev_port
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 dormant
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 duplex
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 flags
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 gro_flush_timeout
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 ifalias
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 ifindex
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 iflink
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 link_mode
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 mtu
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 name_assign_type
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 netdev_group
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 operstate
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 phys_port_id
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 phys_port_name
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 phys_switch_id
drwxr-xr-x 2 nobody nobody    0 Jan 25 18:09 power
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 proto_down
drwxr-xr-x 4 nobody nobody    0 Jan 25 18:09 queues
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 speed
drwxr-xr-x 2 nobody nobody    0 Jan 25 18:09 statistics
lrwxrwxrwx 1 nobody nobody    0 Jan 25 18:08 subsystem -> ../../../../class/net
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:09 tx_queue_len
-r--r--r-- 1 nobody nobody 4096 Jan 25 18:09 type
-rw-r--r-- 1 nobody nobody 4096 Jan 25 18:08 uevent

However, if a device is created directly in the network namespace then
the device's sysfs permissions will be correctly updated:

drwxr-xr-x 5 root   root      0 Jan 25 18:12 .
drwxr-xr-x 9 nobody nobody    0 Jan 25 18:08 ..
-r--r--r-- 1 root   root   4096 Jan 25 18:12 addr_assign_type
-r--r--r-- 1 root   root   4096 Jan 25 18:12 addr_len
-r--r--r-- 1 root   root   4096 Jan 25 18:12 address
-r--r--r-- 1 root   root   4096 Jan 25 18:12 broadcast
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 carrier
-r--r--r-- 1 root   root   4096 Jan 25 18:12 carrier_changes
-r--r--r-- 1 root   root   4096 Jan 25 18:12 carrier_down_count
-r--r--r-- 1 root   root   4096 Jan 25 18:12 carrier_up_count
-r--r--r-- 1 root   root   4096 Jan 25 18:12 dev_id
-r--r--r-- 1 root   root   4096 Jan 25 18:12 dev_port
-r--r--r-- 1 root   root   4096 Jan 25 18:12 dormant
-r--r--r-- 1 root   root   4096 Jan 25 18:12 duplex
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 flags
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 gro_flush_timeout
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 ifalias
-r--r--r-- 1 root   root   4096 Jan 25 18:12 ifindex
-r--r--r-- 1 root   root   4096 Jan 25 18:12 iflink
-r--r--r-- 1 root   root   4096 Jan 25 18:12 link_mode
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 mtu
-r--r--r-- 1 root   root   4096 Jan 25 18:12 name_assign_type
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 netdev_group
-r--r--r-- 1 root   root   4096 Jan 25 18:12 operstate
-r--r--r-- 1 root   root   4096 Jan 25 18:12 phys_port_id
-r--r--r-- 1 root   root   4096 Jan 25 18:12 phys_port_name
-r--r--r-- 1 root   root   4096 Jan 25 18:12 phys_switch_id
drwxr-xr-x 2 root   root      0 Jan 25 18:12 power
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 proto_down
drwxr-xr-x 4 root   root      0 Jan 25 18:12 queues
-r--r--r-- 1 root   root   4096 Jan 25 18:12 speed
drwxr-xr-x 2 root   root      0 Jan 25 18:12 statistics
lrwxrwxrwx 1 nobody nobody    0 Jan 25 18:12 subsystem -> ../../../../class/net
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 tx_queue_len
-r--r--r-- 1 root   root   4096 Jan 25 18:12 type
-rw-r--r-- 1 root   root   4096 Jan 25 18:12 uevent

Now, when creating a network device in a network namespace owned by a
user namespace and moving it to the host the permissions will be set to
the id that the user namespace root user has been mapped to on the host
leading to all sorts of permission issues:

458752
drwxr-xr-x 5 458752 458752      0 Jan 25 18:12 .
drwxr-xr-x 9 root   root        0 Jan 25 18:08 ..
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 addr_assign_type
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 addr_len
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 address
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 broadcast
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 carrier
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 carrier_changes
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 carrier_down_count
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 carrier_up_count
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 dev_id
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 dev_port
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 dormant
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 duplex
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 flags
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 gro_flush_timeout
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 ifalias
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 ifindex
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 iflink
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 link_mode
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 mtu
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 name_assign_type
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 netdev_group
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 operstate
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 phys_port_id
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 phys_port_name
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 phys_switch_id
drwxr-xr-x 2 458752 458752      0 Jan 25 18:12 power
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 proto_down
drwxr-xr-x 4 458752 458752      0 Jan 25 18:12 queues
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 speed
drwxr-xr-x 2 458752 458752      0 Jan 25 18:12 statistics
lrwxrwxrwx 1 root   root        0 Jan 25 18:12 subsystem -> ../../../../class/net
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 tx_queue_len
-r--r--r-- 1 458752 458752   4096 Jan 25 18:12 type
-rw-r--r-- 1 458752 458752   4096 Jan 25 18:12 uevent

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 net/core/net-sysfs.c | 27 +++++++++++++++++++++++++++
 net/core/net-sysfs.h |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 4c826b8bf9b1..4fda021edf6d 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1767,6 +1767,33 @@ int netdev_register_kobject(struct net_device *ndev)
 	return error;
 }
 
+/* Change owner for sysfs entries when moving network devices across network
+ * namespaces owned by different user namespaces.
+ */
+int netdev_change_owner(struct net_device *ndev, const struct net *net_old,
+			const struct net *net_new)
+{
+	struct device *dev = &ndev->dev;
+	kuid_t old_uid, new_uid;
+	kgid_t old_gid, new_gid;
+	int error;
+
+	net_ns_get_ownership(net_old, &old_uid, &old_gid);
+	net_ns_get_ownership(net_new, &new_uid, &new_gid);
+
+	/* The network namespace was changed but the owning user namespace is
+	 * identical so there's no need to change the owner of sysfs entries.
+	 */
+	if (uid_eq(old_uid, new_uid) && gid_eq(old_gid, new_gid))
+		return 0;
+
+	error = device_change_owner(dev);
+	if (error)
+		return error;
+
+	return 0;
+}
+
 int netdev_class_create_file_ns(const struct class_attribute *class_attr,
 				const void *ns)
 {
diff --git a/net/core/net-sysfs.h b/net/core/net-sysfs.h
index 006876c7b78d..8a5b04c2699a 100644
--- a/net/core/net-sysfs.h
+++ b/net/core/net-sysfs.h
@@ -8,5 +8,7 @@ void netdev_unregister_kobject(struct net_device *);
 int net_rx_queue_update_kobjects(struct net_device *, int old_num, int new_num);
 int netdev_queue_update_kobjects(struct net_device *net,
 				 int old_num, int new_num);
+int netdev_change_owner(struct net_device *, const struct net *net_old,
+			const struct net *net_new);
 
 #endif
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next 09/10] net-sysfs: add queue_change_owner()
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
                   ` (7 preceding siblings ...)
  2020-02-12 10:43 ` [PATCH net-next 08/10] net-sysfs: add netdev_change_owner() Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 10:43 ` [PATCH net-next 10/10] net: fix sysfs permssions when device changes network namespace Christian Brauner
  2020-02-12 17:53 ` [PATCH net-next 00/10] net: fix sysfs permssions when device changes network David Miller
  10 siblings, 0 replies; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Add a function to change the owner of the queue entries for a network device
when it is moved between network namespaces.

Currently, when moving network devices between network namespaces the
ownership of the corresponding queue sysfs entries are not changed. This leads
to problems when tools try to operate on the corresponding sysfs files. Fix
this.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 net/core/net-sysfs.c | 101 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 4fda021edf6d..53db98be6198 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -944,6 +944,22 @@ static int rx_queue_add_kobject(struct net_device *dev, int index)
 	kobject_put(kobj);
 	return error;
 }
+
+static int rx_queue_change_owner(struct net_device *dev, int index)
+{
+	int error;
+	struct netdev_rx_queue *queue = dev->_rx + index;
+	struct kobject *kobj = &queue->kobj;
+
+	error = sysfs_change_owner(kobj);
+	if (error)
+		return error;
+
+	if (dev->sysfs_rx_queue_group)
+		error = sysfs_group_change_owner(kobj, dev->sysfs_rx_queue_group);
+
+	return error;
+}
 #endif /* CONFIG_SYSFS */
 
 int
@@ -981,6 +997,28 @@ net_rx_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
 #endif
 }
 
+int net_rx_queue_change_owner(struct net_device *dev, int num)
+{
+#ifdef CONFIG_SYSFS
+	int i;
+	int error = 0;
+
+#ifndef CONFIG_RPS
+	if (!dev->sysfs_rx_queue_group)
+		return 0;
+#endif
+	for (i = 0; i < num; i++) {
+		error = rx_queue_change_owner(dev, i);
+		if (error)
+			break;
+	}
+
+	return error;
+#else
+	return 0;
+#endif
+}
+
 #ifdef CONFIG_SYSFS
 /*
  * netdev_queue sysfs structures and functions.
@@ -1486,6 +1524,22 @@ static int netdev_queue_add_kobject(struct net_device *dev, int index)
 	kobject_put(kobj);
 	return error;
 }
+
+static int tx_queue_change_owner(struct net_device *ndev, int index)
+{
+	int error;
+	struct netdev_queue *queue = ndev->_tx + index;
+	struct kobject *kobj = &queue->kobj;
+
+	error = sysfs_change_owner(kobj);
+	if (error)
+		return error;
+
+#ifdef CONFIG_BQL
+	error = sysfs_group_change_owner(kobj, &dql_group);
+#endif
+	return error;
+}
 #endif /* CONFIG_SYSFS */
 
 int
@@ -1520,6 +1574,24 @@ netdev_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
 #endif /* CONFIG_SYSFS */
 }
 
+static int net_tx_queue_change_owner(struct net_device *dev, int num)
+{
+#ifdef CONFIG_SYSFS
+	int i;
+	int error = 0;
+
+	for (i = 0; i < num; i++) {
+		error = tx_queue_change_owner(dev, i);
+		if (error)
+			break;
+	}
+
+	return error;
+#else
+	return 0;
+#endif /* CONFIG_SYSFS */
+}
+
 static int register_queue_kobjects(struct net_device *dev)
 {
 	int error = 0, txq = 0, rxq = 0, real_rx = 0, real_tx = 0;
@@ -1554,6 +1626,31 @@ static int register_queue_kobjects(struct net_device *dev)
 	return error;
 }
 
+static int queue_change_owner(struct net_device *ndev)
+{
+	int error = 0, real_rx = 0, real_tx = 0;
+
+#ifdef CONFIG_SYSFS
+	if (ndev->queues_kset) {
+		error = sysfs_change_owner(&ndev->queues_kset->kobj);
+		if (error)
+			return error;
+	}
+	real_rx = ndev->real_num_rx_queues;
+#endif
+	real_tx = ndev->real_num_tx_queues;
+
+	error = net_rx_queue_change_owner(ndev, real_rx);
+	if (error)
+		return error;
+
+	error = net_tx_queue_change_owner(ndev, real_tx);
+	if (error)
+		return error;
+
+	return 0;
+}
+
 static void remove_queue_kobjects(struct net_device *dev)
 {
 	int real_rx = 0, real_tx = 0;
@@ -1791,6 +1888,10 @@ int netdev_change_owner(struct net_device *ndev, const struct net *net_old,
 	if (error)
 		return error;
 
+	error = queue_change_owner(ndev);
+	if (error)
+		return error;
+
 	return 0;
 }
 
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next 10/10] net: fix sysfs permssions when device changes network namespace
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
                   ` (8 preceding siblings ...)
  2020-02-12 10:43 ` [PATCH net-next 09/10] net-sysfs: add queue_change_owner() Christian Brauner
@ 2020-02-12 10:43 ` Christian Brauner
  2020-02-12 17:53 ` [PATCH net-next 00/10] net: fix sysfs permssions when device changes network David Miller
  10 siblings, 0 replies; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 10:43 UTC (permalink / raw)
  To: David S. Miller, Greg Kroah-Hartman, linux-kernel, netdev
  Cc: Rafael J. Wysocki, Pavel Machek, Jakub Kicinski, Eric Dumazet,
	Stephen Hemminger, linux-pm, Christian Brauner

Now that we moved all the helpers in place and make use netdev_change_owner()
to fixup the permissions when moving network devices between network
namespaces.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 net/core/dev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a69e8bd7ed74..e463539f0b1d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10017,6 +10017,7 @@ EXPORT_SYMBOL(unregister_netdev);
 int dev_change_net_namespace(struct net_device *dev, struct net *net, const char *pat)
 {
 	int err, new_nsid, new_ifindex;
+	struct net *net_old = dev_net(dev);
 
 	ASSERT_RTNL();
 
@@ -10031,7 +10032,7 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 
 	/* Get out if there is nothing todo */
 	err = 0;
-	if (net_eq(dev_net(dev), net))
+	if (net_eq(net_old, net))
 		goto out;
 
 	/* Pick the destination device name, and ensure
@@ -10107,6 +10108,12 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	err = device_rename(&dev->dev, dev->name);
 	WARN_ON(err);
 
+	/* Adapt owner in case owning user namespace of target network
+	 * namespace is different from the original one.
+	 */
+	err = netdev_change_owner(dev, net_old, net);
+	WARN_ON(err);
+
 	/* Add the device back in the hashes */
 	list_netdevice(dev);
 
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next 07/10] drivers/base/power: add dpm_sysfs_change_owner()
  2020-02-12 10:43 ` [PATCH net-next 07/10] drivers/base/power: add dpm_sysfs_change_owner() Christian Brauner
@ 2020-02-12 10:52   ` Rafael J. Wysocki
  0 siblings, 0 replies; 20+ messages in thread
From: Rafael J. Wysocki @ 2020-02-12 10:52 UTC (permalink / raw)
  To: Christian Brauner
  Cc: David S. Miller, Greg Kroah-Hartman, Linux Kernel Mailing List,
	netdev, Rafael J. Wysocki, Pavel Machek, Jakub Kicinski,
	Eric Dumazet, Stephen Hemminger, Linux PM

On Wed, Feb 12, 2020 at 11:43 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> Add a helper to change the owner of a device's power entries. This
> needs to happen when the ownership of a device is changed, e.g. when
> moving network devices between network namespaces.
> The ownership of a device's power entries is determined based on the
> ownership of the corresponding kobject, i.e. only if the ownership of a
> kobject is changed will this function change the ownership of the
> corresponding sysfs entries.
> This function will be used to correctly account for ownership changes,
> e.g. when moving network devices between network namespaces.
>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  drivers/base/core.c        |  4 ++++
>  drivers/base/power/power.h |  2 ++
>  drivers/base/power/sysfs.c | 37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 43 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 262217287a09..dfaf6d3614fa 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3515,6 +3515,10 @@ int device_change_owner(struct device *dev)
>         if (error)
>                 goto out;
>
> +       error = dpm_sysfs_change_owner(dev);
> +       if (error)
> +               goto out;
> +
>  #ifdef CONFIG_BLOCK
>         if (sysfs_deprecated && dev->class == &block_class)
>                 goto out;
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index 444f5c169a0b..f68490d0811b 100644
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -74,6 +74,7 @@ extern int pm_qos_sysfs_add_flags(struct device *dev);
>  extern void pm_qos_sysfs_remove_flags(struct device *dev);
>  extern int pm_qos_sysfs_add_latency_tolerance(struct device *dev);
>  extern void pm_qos_sysfs_remove_latency_tolerance(struct device *dev);
> +extern int dpm_sysfs_change_owner(struct device *dev);
>
>  #else /* CONFIG_PM */
>
> @@ -88,6 +89,7 @@ static inline void pm_runtime_remove(struct device *dev) {}
>
>  static inline int dpm_sysfs_add(struct device *dev) { return 0; }
>  static inline void dpm_sysfs_remove(struct device *dev) {}
> +static inline int dpm_sysfs_change_owner(struct device *dev) { return 0; }
>
>  #endif
>
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index d7d82db2e4bc..ce1fd346e854 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -684,6 +684,43 @@ int dpm_sysfs_add(struct device *dev)
>         return rc;
>  }
>
> +int dpm_sysfs_change_owner(struct device *dev)
> +{
> +       int rc;
> +
> +       if (device_pm_not_required(dev))
> +               return 0;
> +
> +       rc = sysfs_group_change_owner(&dev->kobj, &pm_attr_group);
> +       if (rc)
> +               return rc;
> +
> +       if (pm_runtime_callbacks_present(dev)) {
> +               rc = sysfs_group_change_owner(&dev->kobj,
> +                                             &pm_runtime_attr_group);
> +               if (rc)
> +                       return rc;
> +       }
> +       if (device_can_wakeup(dev)) {
> +               rc = sysfs_group_change_owner(&dev->kobj,
> +                                             &pm_wakeup_attr_group);
> +               if (rc)
> +                       return rc;
> +       }
> +       if (dev->power.set_latency_tolerance) {
> +               rc = sysfs_group_change_owner(&dev->kobj,
> +                               &pm_qos_latency_tolerance_attr_group);
> +               if (rc)
> +                       return rc;
> +       }
> +       if (dev->power.wakeup && dev->power.wakeup->dev) {

This is related to the device_can_wakeup(dev) condition above (i.e. it
will never be 'true' if that one is 'false').

LGTM apart from this.

> +               rc = device_change_owner(dev->power.wakeup->dev);
> +               if (rc)
> +                       return rc;
> +       }
> +       return 0;
> +}
> +
>  int wakeup_sysfs_add(struct device *dev)
>  {
>         return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> --
> 2.25.0
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next 05/10] sysfs: add sysfs_change_owner()
  2020-02-12 10:43 ` [PATCH net-next 05/10] sysfs: add sysfs_change_owner() Christian Brauner
@ 2020-02-12 13:18   ` Greg Kroah-Hartman
  2020-02-12 15:07     ` Christian Brauner
  0 siblings, 1 reply; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-02-12 13:18 UTC (permalink / raw)
  To: Christian Brauner
  Cc: David S. Miller, linux-kernel, netdev, Rafael J. Wysocki,
	Pavel Machek, Jakub Kicinski, Eric Dumazet, Stephen Hemminger,
	linux-pm

On Wed, Feb 12, 2020 at 11:43:16AM +0100, Christian Brauner wrote:
> Add a helper to change the owner of sysfs objects.

Seems sane, but:

> The ownership of a sysfs object is determined based on the ownership of
> the corresponding kobject, i.e. only if the ownership of a kobject is
> changed will this function change the ownership of the corresponding
> sysfs entry.

A "sysfs object" is a kobject.  So I don't understand this sentance,
sorry.

> This function will be used to correctly account for kobject ownership
> changes, e.g. when moving network devices between network namespaces.
> 
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  fs/sysfs/file.c       | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  6 ++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 6239d9584f0b..6a0fe88061fd 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -642,3 +642,38 @@ int sysfs_file_change_owner(struct kobject *kobj, const char *name)
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
> +
> +/**
> + *	sysfs_change_owner - change owner of the given object.
> + *	@kobj:	object.
> + */
> +int sysfs_change_owner(struct kobject *kobj)

What does this change the owner of the given object _to_?

> +{
> +	int error;
> +	const struct kobj_type *ktype;
> +
> +	if (!kobj->state_in_sysfs)
> +		return -EINVAL;
> +
> +	error = sysfs_file_change_owner(kobj, NULL);

It passes NULL?


> +	if (error)
> +		return error;
> +
> +	ktype = get_ktype(kobj);
> +	if (ktype) {
> +		struct attribute **kattr;
> +
> +		for (kattr = ktype->default_attrs; kattr && *kattr; kattr++) {
> +			error = sysfs_file_change_owner(kobj, (*kattr)->name);
> +			if (error)
> +				return error;
> +		}
> +
> +		error = sysfs_groups_change_owner(kobj, ktype->default_groups);
> +		if (error)
> +			return error;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(sysfs_change_owner);

I can understand wanting to change owners/groups/whatever of existing
sysfs objects and their files, but I can't figure out how to call this
function to set the attribute I want to change.

With only one parameter, how does this work?  It guesses?  :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next 01/10] sysfs: add sysfs_file_change_owner()
  2020-02-12 10:43 ` [PATCH net-next 01/10] sysfs: add sysfs_file_change_owner() Christian Brauner
@ 2020-02-12 13:19   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-02-12 13:19 UTC (permalink / raw)
  To: Christian Brauner
  Cc: David S. Miller, linux-kernel, netdev, Rafael J. Wysocki,
	Pavel Machek, Jakub Kicinski, Eric Dumazet, Stephen Hemminger,
	linux-pm

On Wed, Feb 12, 2020 at 11:43:12AM +0100, Christian Brauner wrote:
> Add a helper to change the owner of a sysfs file.
> The ownership of a sysfs object is determined based on the ownership of
> the corresponding kobject, i.e. only if the ownership of a kobject is
> changed will this function change the ownership of the corresponding
> sysfs entry.
> This function will be used to correctly account for kobject ownership
> changes, e.g. when moving network devices between network namespaces.
> 
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  fs/sysfs/file.c       | 46 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  7 +++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 130fc6fbcc03..007b97ca8165 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -558,3 +558,49 @@ void sysfs_remove_bin_file(struct kobject *kobj,
>  	kernfs_remove_by_name(kobj->sd, attr->attr.name);
>  }
>  EXPORT_SYMBOL_GPL(sysfs_remove_bin_file);
> +
> +static int internal_change_owner(struct kernfs_node *kn, struct kobject *kobj)
> +{
> +	kuid_t uid;
> +	kgid_t gid;
> +	struct iattr newattrs = {
> +		.ia_valid = ATTR_UID | ATTR_GID,
> +	};
> +
> +	kobject_get_ownership(kobj, &uid, &gid);
> +	newattrs.ia_uid = uid;
> +	newattrs.ia_gid = gid;
> +
> +	return kernfs_setattr(kn, &newattrs);
> +}
> +
> +/**
> + *	sysfs_file_change_owner - change owner of a file.
> + *	@kobj:	object.
> + *	@name:	name of the file to change.
> + *	        can be NULL to change current file.
> + */
> +int sysfs_file_change_owner(struct kobject *kobj, const char *name)

Same meta-question I did for the other call, what does this set the file
owner to?  How to you specify this?

I understand your overall goal/need here, I'm just not understanding how
this actually changes anything.

lost,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next 05/10] sysfs: add sysfs_change_owner()
  2020-02-12 13:18   ` Greg Kroah-Hartman
@ 2020-02-12 15:07     ` Christian Brauner
  2020-02-12 16:04       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 15:07 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: David S. Miller, linux-kernel, netdev, Rafael J. Wysocki,
	Pavel Machek, Jakub Kicinski, Eric Dumazet, Stephen Hemminger,
	linux-pm

On Wed, Feb 12, 2020 at 05:18:08AM -0800, Greg Kroah-Hartman wrote:
> On Wed, Feb 12, 2020 at 11:43:16AM +0100, Christian Brauner wrote:
> > Add a helper to change the owner of sysfs objects.
> 
> Seems sane, but:
> 
> > The ownership of a sysfs object is determined based on the ownership of
> > the corresponding kobject, i.e. only if the ownership of a kobject is
> > changed will this function change the ownership of the corresponding
> > sysfs entry.
> 
> A "sysfs object" is a kobject.  So I don't understand this sentance,
> sorry.

I meant that only if you change the uid/gid the underlying kobject is
associated with will this function do anything, meaning that you can't
pass in uids/gids directly. I'll explain why I did this down below [1].
Sorry if that was confusing.

> 
> > This function will be used to correctly account for kobject ownership
> > changes, e.g. when moving network devices between network namespaces.
> > 
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> >  fs/sysfs/file.c       | 35 +++++++++++++++++++++++++++++++++++
> >  include/linux/sysfs.h |  6 ++++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > index 6239d9584f0b..6a0fe88061fd 100644
> > --- a/fs/sysfs/file.c
> > +++ b/fs/sysfs/file.c
> > @@ -642,3 +642,38 @@ int sysfs_file_change_owner(struct kobject *kobj, const char *name)
> >  	return error;
> >  }
> >  EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
> > +
> > +/**
> > + *	sysfs_change_owner - change owner of the given object.
> > + *	@kobj:	object.
> > + */
> > +int sysfs_change_owner(struct kobject *kobj)
> 
> What does this change the owner of the given object _to_?

[1]:
So ownership only changes if the kobject's uid/gid have been changed.
So when to stick with the networking example, when a network device is
moved into a new network namespace, the uid/gid of the kobject will be
changed to the root user of the owning user namespace of that network
namespace. So when the move of the network device has completed and
kobject_get_ownership() is called it will now return a different
uid/gid.
So my reasoning was that ownership is determined dynamically that way. I
guess what you're hinting at is that we could simply add uid_t uid,
gid_t gid arguments to these sysfs helpers. That's fine with me too. It
means that callers are responsible to either retrieve the ownership from
the kobject (in case it was changed through another call) or the call to
syfs_change_owner(kobj, uid, gid) sets the new owner of the kobject. I
don't know what the best approach is. Maybe a hybrid whereby we allow
passing in uid/gid but also allow passing in ({g,u}id_t - 1) to indicate
that we want the ownership to be taken from the kobject itself (e.g.
when a network device has been updated by dev_change_net_namespace()).

> 
> > +{
> > +	int error;
> > +	const struct kobj_type *ktype;
> > +
> > +	if (!kobj->state_in_sysfs)
> > +		return -EINVAL;
> > +
> > +	error = sysfs_file_change_owner(kobj, NULL);
> 
> It passes NULL?

Which means, change the ownership of "kobj" itself and not lookup a file
relative to "kobj".

> 
> 
> > +	if (error)
> > +		return error;
> > +
> > +	ktype = get_ktype(kobj);
> > +	if (ktype) {
> > +		struct attribute **kattr;
> > +
> > +		for (kattr = ktype->default_attrs; kattr && *kattr; kattr++) {
> > +			error = sysfs_file_change_owner(kobj, (*kattr)->name);
> > +			if (error)
> > +				return error;
> > +		}
> > +
> > +		error = sysfs_groups_change_owner(kobj, ktype->default_groups);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(sysfs_change_owner);
> 
> I can understand wanting to change owners/groups/whatever of existing
> sysfs objects and their files, but I can't figure out how to call this
> function to set the attribute I want to change.
> 
> With only one parameter, how does this work?  It guesses?  :)

See [1]. :)
And sorry if that wasn't clear!

Thanks!
Christian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next 05/10] sysfs: add sysfs_change_owner()
  2020-02-12 15:07     ` Christian Brauner
@ 2020-02-12 16:04       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-02-12 16:04 UTC (permalink / raw)
  To: Christian Brauner
  Cc: David S. Miller, linux-kernel, netdev, Rafael J. Wysocki,
	Pavel Machek, Jakub Kicinski, Eric Dumazet, Stephen Hemminger,
	linux-pm

On Wed, Feb 12, 2020 at 04:07:43PM +0100, Christian Brauner wrote:
> On Wed, Feb 12, 2020 at 05:18:08AM -0800, Greg Kroah-Hartman wrote:
> > On Wed, Feb 12, 2020 at 11:43:16AM +0100, Christian Brauner wrote:
> > > Add a helper to change the owner of sysfs objects.
> > 
> > Seems sane, but:
> > 
> > > The ownership of a sysfs object is determined based on the ownership of
> > > the corresponding kobject, i.e. only if the ownership of a kobject is
> > > changed will this function change the ownership of the corresponding
> > > sysfs entry.
> > 
> > A "sysfs object" is a kobject.  So I don't understand this sentance,
> > sorry.
> 
> I meant that only if you change the uid/gid the underlying kobject is
> associated with will this function do anything, meaning that you can't
> pass in uids/gids directly. I'll explain why I did this down below [1].
> Sorry if that was confusing.
> 
> > 
> > > This function will be used to correctly account for kobject ownership
> > > changes, e.g. when moving network devices between network namespaces.
> > > 
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > ---
> > >  fs/sysfs/file.c       | 35 +++++++++++++++++++++++++++++++++++
> > >  include/linux/sysfs.h |  6 ++++++
> > >  2 files changed, 41 insertions(+)
> > > 
> > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > index 6239d9584f0b..6a0fe88061fd 100644
> > > --- a/fs/sysfs/file.c
> > > +++ b/fs/sysfs/file.c
> > > @@ -642,3 +642,38 @@ int sysfs_file_change_owner(struct kobject *kobj, const char *name)
> > >  	return error;
> > >  }
> > >  EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
> > > +
> > > +/**
> > > + *	sysfs_change_owner - change owner of the given object.
> > > + *	@kobj:	object.
> > > + */
> > > +int sysfs_change_owner(struct kobject *kobj)
> > 
> > What does this change the owner of the given object _to_?
> 
> [1]:
> So ownership only changes if the kobject's uid/gid have been changed.
> So when to stick with the networking example, when a network device is
> moved into a new network namespace, the uid/gid of the kobject will be
> changed to the root user of the owning user namespace of that network
> namespace. So when the move of the network device has completed and
> kobject_get_ownership() is called it will now return a different
> uid/gid.

Ok, then this needs to say "change the uid/gid of the kobject to..." in
order to explain what it is now being set to.  Otherwise this is really
confusing if you only read the kerneldoc, right?

> So my reasoning was that ownership is determined dynamically that way. I
> guess what you're hinting at is that we could simply add uid_t uid,
> gid_t gid arguments to these sysfs helpers. That's fine with me too.

It's fine if you want to set it to the "root owner", just say that
somewhere :)

> It
> means that callers are responsible to either retrieve the ownership from
> the kobject (in case it was changed through another call) or the call to
> syfs_change_owner(kobj, uid, gid) sets the new owner of the kobject. I
> don't know what the best approach is. Maybe a hybrid whereby we allow
> passing in uid/gid but also allow passing in ({g,u}id_t - 1) to indicate
> that we want the ownership to be taken from the kobject itself (e.g.
> when a network device has been updated by dev_change_net_namespace()).
> 
> > 
> > > +{
> > > +	int error;
> > > +	const struct kobj_type *ktype;
> > > +
> > > +	if (!kobj->state_in_sysfs)
> > > +		return -EINVAL;
> > > +
> > > +	error = sysfs_file_change_owner(kobj, NULL);
> > 
> > It passes NULL?
> 
> Which means, change the ownership of "kobj" itself and not lookup a file
> relative to "kobj".

Ok, that's totally not obvious at all :(

Better naming please, I know it's hard, but it matters.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next 00/10] net: fix sysfs permssions when device changes network
  2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
                   ` (9 preceding siblings ...)
  2020-02-12 10:43 ` [PATCH net-next 10/10] net: fix sysfs permssions when device changes network namespace Christian Brauner
@ 2020-02-12 17:53 ` David Miller
  2020-02-12 18:00   ` Christian Brauner
  10 siblings, 1 reply; 20+ messages in thread
From: David Miller @ 2020-02-12 17:53 UTC (permalink / raw)
  To: christian.brauner
  Cc: gregkh, linux-kernel, netdev, rafael, pavel, kuba, edumazet,
	stephen, linux-pm


net-next is closed

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next 00/10] net: fix sysfs permssions when device changes network
  2020-02-12 17:53 ` [PATCH net-next 00/10] net: fix sysfs permssions when device changes network David Miller
@ 2020-02-12 18:00   ` Christian Brauner
  2020-02-12 18:16     ` David Miller
  0 siblings, 1 reply; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 18:00 UTC (permalink / raw)
  To: David Miller
  Cc: gregkh, linux-kernel, netdev, rafael, pavel, kuba, edumazet,
	stephen, linux-pm

On Wed, Feb 12, 2020 at 09:53:18AM -0800, David Miller wrote:
> 
> net-next is closed

I just assumed it was open since rc1 has been out for a few days now.
Will resend once it reopens.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next 00/10] net: fix sysfs permssions when device changes network
  2020-02-12 18:00   ` Christian Brauner
@ 2020-02-12 18:16     ` David Miller
  2020-02-12 18:20       ` Christian Brauner
  0 siblings, 1 reply; 20+ messages in thread
From: David Miller @ 2020-02-12 18:16 UTC (permalink / raw)
  To: christian.brauner
  Cc: gregkh, linux-kernel, netdev, rafael, pavel, kuba, edumazet,
	stephen, linux-pm

From: Christian Brauner <christian.brauner@ubuntu.com>
Date: Wed, 12 Feb 2020 19:00:44 +0100

> On Wed, Feb 12, 2020 at 09:53:18AM -0800, David Miller wrote:
>> 
>> net-next is closed
> 
> I just assumed it was open since rc1 has been out for a few days now.

Then why do I bother maintaining:

	http://vger.kernel.org/~davem/net-next.html

which is also mentioned in the netdev FAQ
Documentation/networking/netdev-FAQ.rst:

====================
If you aren't subscribed to netdev and/or are simply unsure if
``net-next`` has re-opened yet, simply check the ``net-next`` git
repository link above for any new networking-related commits.  You may
also check the following website for the current status:

  http://vger.kernel.org/~davem/net-next.html
====================

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next 00/10] net: fix sysfs permssions when device changes network
  2020-02-12 18:16     ` David Miller
@ 2020-02-12 18:20       ` Christian Brauner
  0 siblings, 0 replies; 20+ messages in thread
From: Christian Brauner @ 2020-02-12 18:20 UTC (permalink / raw)
  To: David Miller
  Cc: gregkh, linux-kernel, netdev, rafael, pavel, kuba, edumazet,
	stephen, linux-pm

On Wed, Feb 12, 2020 at 10:16:47AM -0800, David Miller wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> Date: Wed, 12 Feb 2020 19:00:44 +0100
> 
> > On Wed, Feb 12, 2020 at 09:53:18AM -0800, David Miller wrote:
> >> 
> >> net-next is closed
> > 
> > I just assumed it was open since rc1 has been out for a few days now.
> 
> Then why do I bother maintaining:
> 
> 	http://vger.kernel.org/~davem/net-next.html
> 
> which is also mentioned in the netdev FAQ
> Documentation/networking/netdev-FAQ.rst:

Yeah, I know. I honestly missed looking at that this time around. As I
said, I'll resend once net-next reopens. Thanks for taking the time to
respond though.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-02-12 18:20 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 10:43 [PATCH net-next 00/10] net: fix sysfs permssions when device changes network Christian Brauner
2020-02-12 10:43 ` [PATCH net-next 01/10] sysfs: add sysfs_file_change_owner() Christian Brauner
2020-02-12 13:19   ` Greg Kroah-Hartman
2020-02-12 10:43 ` [PATCH net-next 02/10] sysfs: add sysfs_link_change_owner() Christian Brauner
2020-02-12 10:43 ` [PATCH net-next 03/10] sysfs: add sysfs_group_change_owner() Christian Brauner
2020-02-12 10:43 ` [PATCH net-next 04/10] sysfs: add sysfs_groups_change_owner() Christian Brauner
2020-02-12 10:43 ` [PATCH net-next 05/10] sysfs: add sysfs_change_owner() Christian Brauner
2020-02-12 13:18   ` Greg Kroah-Hartman
2020-02-12 15:07     ` Christian Brauner
2020-02-12 16:04       ` Greg Kroah-Hartman
2020-02-12 10:43 ` [PATCH net-next 06/10] device: add device_change_owner() Christian Brauner
2020-02-12 10:43 ` [PATCH net-next 07/10] drivers/base/power: add dpm_sysfs_change_owner() Christian Brauner
2020-02-12 10:52   ` Rafael J. Wysocki
2020-02-12 10:43 ` [PATCH net-next 08/10] net-sysfs: add netdev_change_owner() Christian Brauner
2020-02-12 10:43 ` [PATCH net-next 09/10] net-sysfs: add queue_change_owner() Christian Brauner
2020-02-12 10:43 ` [PATCH net-next 10/10] net: fix sysfs permssions when device changes network namespace Christian Brauner
2020-02-12 17:53 ` [PATCH net-next 00/10] net: fix sysfs permssions when device changes network David Miller
2020-02-12 18:00   ` Christian Brauner
2020-02-12 18:16     ` David Miller
2020-02-12 18:20       ` Christian Brauner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).