[PATCH net-next v3 0/9] net: fix sysfs permssions when device changes network

[PATCH net-next v3 0/9] net: fix sysfs permssions when device changes network

[Christian Brauner]

Hey everyone,

This is v3 with explicit uid and gid parameters added to functions that
change sysfs object ownership as Greg requested.

(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 (9):
  sysfs: add sysfs_file_change_owner{_by_name}()
  sysfs: add sysfs_link_change_owner()
  sysfs: add sysfs_group{s}_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        |  84 +++++++++++++++++++++
 drivers/base/power/power.h |   3 +
 drivers/base/power/sysfs.c |  42 +++++++++++
 fs/sysfs/file.c            | 146 +++++++++++++++++++++++++++++++++++++
 fs/sysfs/group.c           | 117 +++++++++++++++++++++++++++++
 include/linux/device.h     |   1 +
 include/linux/sysfs.h      |  53 ++++++++++++++
 net/core/dev.c             |   9 ++-
 net/core/net-sysfs.c       | 133 +++++++++++++++++++++++++++++++++
 net/core/net-sysfs.h       |   2 +
 10 files changed, 589 insertions(+), 1 deletion(-)


base-commit: bb6d3fb354c5ee8d6bde2d576eb7220ea09862b9
-- 
2.25.0

[PATCH net-next v3 1/9] sysfs: add sysfs_file_change_owner{_by_name}()

[Christian Brauner]

Add helpers to change the owner of a sysfs files.
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>
---
/* v2 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Better naming for sysfs_file_change_owner() to reflect the fact that it
     can be used to change the owner of the kobject itself by passing NULL as
     argument.
- Christian Brauner <christian.brauner@ubuntu.com>:
  - Split sysfs_file_change_owner() into two helpers sysfs_change_owner() and
    sysfs_change_owner_by_name(). The former changes the owner of the kobject
    itself, the latter the owner of the kobject looked up via the name
    argument.

/* v3 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add explicit uid/gid parameters.
---
 fs/sysfs/file.c       | 67 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h | 17 +++++++++++
 2 files changed, 84 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 130fc6fbcc03..32bb04b4d9d9 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -558,3 +558,70 @@ 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 kuid, kgid_t kgid)
+{
+	struct iattr newattrs = {
+		.ia_valid = ATTR_UID | ATTR_GID,
+		.ia_uid = kuid,
+		.ia_gid = kgid,
+	};
+	return kernfs_setattr(kn, &newattrs);
+}
+
+/**
+ *	sysfs_file_change_owner_by_name - change owner of a file.
+ *	@kobj:	object.
+ *	@name:	name of the file to change.
+ *	@kuid:	new owner's kuid
+ *	@kgid:	new owner's kgid
+ */
+int sysfs_file_change_owner_by_name(struct kobject *kobj, const char *name,
+				    kuid_t kuid, kgid_t kgid)
+{
+	struct kernfs_node *kn;
+	int error;
+
+	if (!name)
+		return -EINVAL;
+
+	if (!kobj->state_in_sysfs)
+		return -EINVAL;
+
+	kn = kernfs_find_and_get(kobj->sd, name);
+	if (!kn)
+		return -ENOENT;
+
+	error = internal_change_owner(kn, kobj, kuid, kgid);
+
+	kernfs_put(kn);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(sysfs_file_change_owner_by_name);
+
+/**
+ *	sysfs_file_change_owner - change owner of a file.
+ *	@kobj:	object.
+ *	@kuid: new owner's kuid
+ *	@kgid: new owner's kgid
+ */
+int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
+{
+	struct kernfs_node *kn;
+	int error;
+
+	if (!kobj->state_in_sysfs)
+		return -EINVAL;
+
+	kernfs_get(kobj->sd);
+
+	kn = kobj->sd;
+	error = internal_change_owner(kn, kobj, kuid, kgid);
+
+	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..c11d11c78713 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -310,6 +310,10 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn)
 	return kernfs_enable_ns(kn);
 }
 
+int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid);
+int sysfs_file_change_owner_by_name(struct kobject *kobj, const char *name,
+				    kuid_t kuid, kgid_t kgid);
+
 #else /* CONFIG_SYSFS */
 
 static inline int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
@@ -522,6 +526,19 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn)
 {
 }
 
+static inline int int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid,
+					      kgid_t kgid)
+{
+	return 0;
+}
+
+static inline int sysfs_file_change_owner_by_name(struct kobject *kobj,
+						  const char *name, kuid_t kuid,
+						  kgid_t kgid)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
-- 
2.25.0

[PATCH net-next v3 2/9] sysfs: add sysfs_link_change_owner()

[Christian Brauner]

Add a helper to change the owner of a sysfs link.
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>
---
/* v2 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add comment how ownership of sysfs object is changed.

/* v3 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add explicit uid/gid parameters.
---
 fs/sysfs/file.c       | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h | 10 ++++++++++
 2 files changed, 50 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 32bb04b4d9d9..df5107d7b3fd 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -570,6 +570,46 @@ 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.
+ *	@kuid:	new owner's kuid
+ *	@kgid:	new owner's kgid
+ */
+int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
+			    const char *name, kuid_t kuid, kgid_t kgid)
+{
+	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, kuid, kgid);
+
+out:
+	kernfs_put(kn);
+	return error;
+}
+
 /**
  *	sysfs_file_change_owner_by_name - change owner of a file.
  *	@kobj:	object.
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index c11d11c78713..899500950410 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, kuid_t kuid, kgid_t kgid);
 int sysfs_file_change_owner_by_name(struct kobject *kobj, const char *name,
 				    kuid_t kuid, kgid_t kgid);
+int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
+			    const char *name, kuid_t kuid, kgid_t kgid);
 
 #else /* CONFIG_SYSFS */
 
@@ -539,6 +541,14 @@ static inline int sysfs_file_change_owner_by_name(struct kobject *kobj,
 	return 0;
 }
 
+static inline int sysfs_link_change_owner(struct kobject *kobj,
+					  struct kobject *targ,
+					  const char *name, kuid_t kuid,
+					  kgid_t kgid)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
-- 
2.25.0

[PATCH net-next v3 3/9] sysfs: add sysfs_group{s}_change_owner()

[Christian Brauner]

Add helpers to change the owner of sysfs groups.
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>
---
/* v2 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add comment how ownership of sysfs object is changed.

/* v3 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add explicit uid/gid parameters.
- Christian Brauner <christian.brauner@ubuntu.com>:
  - Collapse groups ownership helper patches into a single patch.
---
 fs/sysfs/group.c      | 117 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  20 ++++++++
 2 files changed, 137 insertions(+)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index c4ab045926b7..bae562d3cba1 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,119 @@ 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.
+ * @kuid:	new owner's kuid
+ * @kgid:	new owner's kgid
+ *
+ * Returns 0 on success or error code on failure.
+ */
+int sysfs_group_change_owner(struct kobject *kobj,
+			     const struct attribute_group *grp, kuid_t kuid,
+			     kgid_t kgid)
+{
+	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;
+
+	newattrs.ia_uid = kuid;
+	newattrs.ia_gid = kgid;
+	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);
+
+/**
+ * sysfs_groups_change_owner - change owner of a set of attribute groups.
+ * @kobj:	The kobject containing the groups.
+ * @groups:	The attribute groups.
+ * @kuid:	new owner's kuid
+ * @kgid:	new owner's kgid
+ *
+ * Returns 0 on success or error code on failure.
+ */
+int sysfs_groups_change_owner(struct kobject *kobj,
+			      const struct attribute_group **groups,
+			      kuid_t kuid, kgid_t kgid)
+{
+	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], kuid, kgid);
+		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 899500950410..564a2e57b90a 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -315,6 +315,12 @@ int sysfs_file_change_owner_by_name(struct kobject *kobj, const char *name,
 				    kuid_t kuid, kgid_t kgid);
 int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
 			    const char *name, kuid_t kuid, kgid_t kgid);
+int sysfs_groups_change_owner(struct kobject *kobj,
+			      const struct attribute_group **groups,
+			      kuid_t kuid, kgid_t kgid);
+int sysfs_group_change_owner(struct kobject *kobj,
+			     const struct attribute_group *groups, kuid_t kuid,
+			     kgid_t kgid);
 
 #else /* CONFIG_SYSFS */
 
@@ -549,6 +555,20 @@ 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,
+			  kuid_t kuid, kgid_t kgid)
+{
+	return 0;
+}
+
+static inline int sysfs_group_change_owner(struct kobject *kobj,
+			 const struct attribute_group **groups,
+			 kuid_t kuid, kgid_t kgid)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
-- 
2.25.0

[PATCH net-next v3 4/9] sysfs: add sysfs_change_owner()

[Christian Brauner]

Add a helper to change the owner of sysfs objects.
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>
---
/* v2 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add comment how ownership of sysfs object is changed.

/* v3 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add explicit uid/gid parameters.
---
 fs/sysfs/file.c       | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  6 ++++++
 2 files changed, 45 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index df5107d7b3fd..02f7e852aad4 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -665,3 +665,42 @@ int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
 	return error;
 }
 EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
+
+/**
+ *	sysfs_change_owner - change owner of the given object.
+ *	@kobj:	object.
+ *	@kuid:	new owner's kuid
+ *	@kgid:	new owner's kgid
+ */
+int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
+{
+	int error;
+	const struct kobj_type *ktype;
+
+	if (!kobj->state_in_sysfs)
+		return -EINVAL;
+
+	error = sysfs_file_change_owner(kobj, kuid, kgid);
+	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_by_name(
+				kobj, (*kattr)->name, kuid, kgid);
+			if (error)
+				return error;
+		}
+
+		error = sysfs_groups_change_owner(kobj, ktype->default_groups,
+						  kuid, kgid);
+		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 564a2e57b90a..fa1a37dd0f2b 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -313,6 +313,7 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn)
 int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid);
 int sysfs_file_change_owner_by_name(struct kobject *kobj, const char *name,
 				    kuid_t kuid, kgid_t kgid);
+int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid);
 int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
 			    const char *name, kuid_t kuid, kgid_t kgid);
 int sysfs_groups_change_owner(struct kobject *kobj,
@@ -555,6 +556,11 @@ static inline int sysfs_link_change_owner(struct kobject *kobj,
 	return 0;
 }
 
+static inline int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
+{
+	return 0;
+}
+
 static inline int sysfs_groups_change_owner(struct kobject *kobj,
 			  const struct attribute_group **groups,
 			  kuid_t kuid, kgid_t kgid)
-- 
2.25.0

[PATCH net-next v3 5/9] device: add device_change_owner()

[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.
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>
---
/* v2 */
unchanged

/* v3 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add explicit uid/gid parameters.
---
 drivers/base/core.c    | 80 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  1 +
 2 files changed, 81 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 42a672456432..ec0d5e8cfd0f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3458,6 +3458,86 @@ int device_move(struct device *dev, struct device *new_parent,
 }
 EXPORT_SYMBOL_GPL(device_move);
 
+static int device_attrs_change_owner(struct device *dev, kuid_t kuid,
+				     kgid_t kgid)
+{
+	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, kuid,
+						  kgid);
+		if (error)
+			return error;
+	}
+
+	if (type) {
+		error = sysfs_groups_change_owner(kobj, type->groups, kuid,
+						  kgid);
+		if (error)
+			return error;
+	}
+
+	error = sysfs_groups_change_owner(kobj, dev->groups, kuid, kgid);
+	if (error)
+		return error;
+
+	if (device_supports_offline(dev) && !dev->offline_disabled) {
+		error = sysfs_file_change_owner_by_name(
+			kobj, dev_attr_online.attr.name, kuid, kgid);
+		if (error)
+			return error;
+	}
+
+	return 0;
+}
+
+/**
+ * device_change_owner - change the owner of an existing device.
+ * @dev: device.
+ * @kuid: new owner's kuid
+ * @kgid: new owner's kgid
+ */
+int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
+{
+	int error;
+	struct kobject *kobj = &dev->kobj;
+
+	dev = get_device(dev);
+	if (!dev)
+		return -EINVAL;
+
+	error = sysfs_change_owner(kobj, kuid, kgid);
+	if (error)
+		goto out;
+
+	error = sysfs_file_change_owner_by_name(kobj, dev_attr_uevent.attr.name,
+						kuid, kgid);
+	if (error)
+		goto out;
+
+	error = device_attrs_change_owner(dev, kuid, kgid);
+	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), kuid, kgid);
+	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..3e40533d2037 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, kuid_t kuid, kgid_t kgid);
 extern const char *device_get_devnode(struct device *dev,
 				      umode_t *mode, kuid_t *uid, kgid_t *gid,
 				      const char **tmp);
-- 
2.25.0

[PATCH net-next v3 6/9] drivers/base/power: add dpm_sysfs_change_owner()

[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.
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>
---
/* v2 */
- "Rafael J. Wysocki" <rafael@kernel.org>:
  -  Fold if (dev->power.wakeup && dev->power.wakeup->dev) check into
     if (device_can_wakeup(dev)) check since the former can never be true if
     the latter is false.

- Christian Brauner <christian.brauner@ubuntu.com>:
  - Place (dev->power.wakeup && dev->power.wakeup->dev) check under
    CONFIG_PM_SLEEP ifdefine since it will wakeup_source will only be available
    when this config option is set.

/* v3 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add explicit uid/gid parameters.
---
 drivers/base/core.c        |  4 ++++
 drivers/base/power/power.h |  3 +++
 drivers/base/power/sysfs.c | 42 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index ec0d5e8cfd0f..efec2792f5d7 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3522,6 +3522,10 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
 	if (error)
 		goto out;
 
+	error = dpm_sysfs_change_owner(dev, kuid, kgid);
+	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..54292cdd7808 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, kuid_t kuid, kgid_t kgid);
 
 #else /* CONFIG_PM */
 
@@ -88,6 +89,8 @@ 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, kuid_t kuid,
+					 kgid_t kgid) { return 0; }
 
 #endif
 
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d7d82db2e4bc..4e79afcd5ca8 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -684,6 +684,48 @@ int dpm_sysfs_add(struct device *dev)
 	return rc;
 }
 
+int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
+{
+	int rc;
+
+	if (device_pm_not_required(dev))
+		return 0;
+
+	rc = sysfs_group_change_owner(&dev->kobj, &pm_attr_group, kuid, kgid);
+	if (rc)
+		return rc;
+
+	if (pm_runtime_callbacks_present(dev)) {
+		rc = sysfs_group_change_owner(
+			&dev->kobj, &pm_runtime_attr_group, kuid, kgid);
+		if (rc)
+			return rc;
+	}
+	if (device_can_wakeup(dev)) {
+		rc = sysfs_group_change_owner(&dev->kobj, &pm_wakeup_attr_group,
+					      kuid, kgid);
+		if (rc)
+			return rc;
+
+#ifdef CONFIG_PM_SLEEP
+		if (dev->power.wakeup && dev->power.wakeup->dev) {
+			rc = device_change_owner(dev->power.wakeup->dev, kuid,
+						 kgid);
+			if (rc)
+				return rc;
+		}
+#endif
+	}
+	if (dev->power.set_latency_tolerance) {
+		rc = sysfs_group_change_owner(
+			&dev->kobj, &pm_qos_latency_tolerance_attr_group, kuid,
+			kgid);
+		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

[PATCH net-next v3 7/9] net-sysfs: add netdev_change_owner()

[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>
---
/* v2 */
unchanged

/* v3 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add explicit uid/gid parameters.
---
 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..e19967665cb0 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, new_uid, new_gid);
+	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

[PATCH net-next v3 8/9] net-sysfs: add queue_change_owner()

[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>
---
/* v2 */
- kbuild test robot <lkp@intel.com> via sparse:
  - Make net_rx_queue_change_owner() static since it's not exported.

/* v3 */
-  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
   - Add explicit uid/gid parameters.
---
 net/core/net-sysfs.c | 106 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index e19967665cb0..cf0215734ceb 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -944,6 +944,24 @@ 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, kuid_t kuid,
+				 kgid_t kgid)
+{
+	struct netdev_rx_queue *queue = dev->_rx + index;
+	struct kobject *kobj = &queue->kobj;
+	int error;
+
+	error = sysfs_change_owner(kobj, kuid, kgid);
+	if (error)
+		return error;
+
+	if (dev->sysfs_rx_queue_group)
+		error = sysfs_group_change_owner(
+			kobj, dev->sysfs_rx_queue_group, kuid, kgid);
+
+	return error;
+}
 #endif /* CONFIG_SYSFS */
 
 int
@@ -981,6 +999,29 @@ net_rx_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
 #endif
 }
 
+static int net_rx_queue_change_owner(struct net_device *dev, int num,
+				     kuid_t kuid, kgid_t kgid)
+{
+#ifdef CONFIG_SYSFS
+	int error = 0;
+	int i;
+
+#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, kuid, kgid);
+		if (error)
+			break;
+	}
+
+	return error;
+#else
+	return 0;
+#endif
+}
+
 #ifdef CONFIG_SYSFS
 /*
  * netdev_queue sysfs structures and functions.
@@ -1486,6 +1527,23 @@ 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,
+				 kuid_t kuid, kgid_t kgid)
+{
+	struct netdev_queue *queue = ndev->_tx + index;
+	struct kobject *kobj = &queue->kobj;
+	int error;
+
+	error = sysfs_change_owner(kobj, kuid, kgid);
+	if (error)
+		return error;
+
+#ifdef CONFIG_BQL
+	error = sysfs_group_change_owner(kobj, &dql_group, kuid, kgid);
+#endif
+	return error;
+}
 #endif /* CONFIG_SYSFS */
 
 int
@@ -1520,6 +1578,25 @@ 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,
+				     kuid_t kuid, kgid_t kgid)
+{
+#ifdef CONFIG_SYSFS
+	int error = 0;
+	int i;
+
+	for (i = 0; i < num; i++) {
+		error = tx_queue_change_owner(dev, i, kuid, kgid);
+		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 +1631,31 @@ static int register_queue_kobjects(struct net_device *dev)
 	return error;
 }
 
+static int queue_change_owner(struct net_device *ndev, kuid_t kuid, kgid_t kgid)
+{
+	int error = 0, real_rx = 0, real_tx = 0;
+
+#ifdef CONFIG_SYSFS
+	if (ndev->queues_kset) {
+		error = sysfs_change_owner(&ndev->queues_kset->kobj, kuid, kgid);
+		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, kuid, kgid);
+	if (error)
+		return error;
+
+	error = net_tx_queue_change_owner(ndev, real_tx, kuid, kgid);
+	if (error)
+		return error;
+
+	return 0;
+}
+
 static void remove_queue_kobjects(struct net_device *dev)
 {
 	int real_rx = 0, real_tx = 0;
@@ -1791,6 +1893,10 @@ int netdev_change_owner(struct net_device *ndev, const struct net *net_old,
 	if (error)
 		return error;
 
+	error = queue_change_owner(ndev, new_uid, new_gid);
+	if (error)
+		return error;
+
 	return 0;
 }
 
-- 
2.25.0

[PATCH net-next v3 9/9] net: fix sysfs permssions when device changes network namespace

[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>
---
/* v2 */
unchanged

/* v3 */
unchanged
---
 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..0f9c4684fcbd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10016,6 +10016,7 @@ EXPORT_SYMBOL(unregister_netdev);
 
 int dev_change_net_namespace(struct net_device *dev, struct net *net, const char *pat)
 {
+	struct net *net_old = dev_net(dev);
 	int err, new_nsid, new_ifindex;
 
 	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

Re: [PATCH net-next v3 0/9] net: fix sysfs permssions when device changes network

[David Miller]

From: Christian Brauner <christian.brauner@ubuntu.com>
Date: Tue, 18 Feb 2020 17:29:34 +0100

> This is v3 with explicit uid and gid parameters added to functions that
> change sysfs object ownership as Greg requested.

Greg, please review.

Thank you.

Re: [PATCH net-next v3 6/9] drivers/base/power: add dpm_sysfs_change_owner()

[Rafael J. Wysocki]

On Tue, Feb 18, 2020 at 5:30 PM 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.
> 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>
> ---
> /* v2 */
> - "Rafael J. Wysocki" <rafael@kernel.org>:
>   -  Fold if (dev->power.wakeup && dev->power.wakeup->dev) check into
>      if (device_can_wakeup(dev)) check since the former can never be true if
>      the latter is false.
>
> - Christian Brauner <christian.brauner@ubuntu.com>:
>   - Place (dev->power.wakeup && dev->power.wakeup->dev) check under
>     CONFIG_PM_SLEEP ifdefine since it will wakeup_source will only be available
>     when this config option is set.
>
> /* v3 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Add explicit uid/gid parameters.
> ---
>  drivers/base/core.c        |  4 ++++
>  drivers/base/power/power.h |  3 +++
>  drivers/base/power/sysfs.c | 42 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index ec0d5e8cfd0f..efec2792f5d7 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3522,6 +3522,10 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
>         if (error)
>                 goto out;
>
> +       error = dpm_sysfs_change_owner(dev, kuid, kgid);
> +       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..54292cdd7808 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, kuid_t kuid, kgid_t kgid);
>
>  #else /* CONFIG_PM */
>
> @@ -88,6 +89,8 @@ 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, kuid_t kuid,
> +                                        kgid_t kgid) { return 0; }
>
>  #endif
>
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index d7d82db2e4bc..4e79afcd5ca8 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -684,6 +684,48 @@ int dpm_sysfs_add(struct device *dev)
>         return rc;
>  }
>
> +int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> +{
> +       int rc;
> +
> +       if (device_pm_not_required(dev))
> +               return 0;
> +
> +       rc = sysfs_group_change_owner(&dev->kobj, &pm_attr_group, kuid, kgid);
> +       if (rc)
> +               return rc;
> +
> +       if (pm_runtime_callbacks_present(dev)) {
> +               rc = sysfs_group_change_owner(
> +                       &dev->kobj, &pm_runtime_attr_group, kuid, kgid);
> +               if (rc)
> +                       return rc;
> +       }
> +       if (device_can_wakeup(dev)) {
> +               rc = sysfs_group_change_owner(&dev->kobj, &pm_wakeup_attr_group,
> +                                             kuid, kgid);
> +               if (rc)
> +                       return rc;
> +
> +#ifdef CONFIG_PM_SLEEP
> +               if (dev->power.wakeup && dev->power.wakeup->dev) {
> +                       rc = device_change_owner(dev->power.wakeup->dev, kuid,
> +                                                kgid);
> +                       if (rc)
> +                               return rc;
> +               }
> +#endif

First off, I don't particularly like #ifdefs in function bodies.  In
particular, there is a CONFIG_PM_SLEEP block in this file already and
you could define a new function in there to carry out the above
operations, and provide an empty stub of it for the "unset" case.
Failing to do so is somewhat on the "rushing things in" side in my
view.

Second, the #ifdef should cover the entire if (device_can_wakeup(dev))
{} block, because wakeup_sysfs_add() is only called if
device_can_wakeup(dev) returns 'true' for the device in question (and
arguably you could have checked that easily enough).

> +       }
> +       if (dev->power.set_latency_tolerance) {
> +               rc = sysfs_group_change_owner(
> +                       &dev->kobj, &pm_qos_latency_tolerance_attr_group, kuid,
> +                       kgid);
> +               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
>

Re: [PATCH net-next v3 6/9] drivers/base/power: add dpm_sysfs_change_owner()

[Christian Brauner]

On Thu, Feb 20, 2020 at 11:02:04AM +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 18, 2020 at 5:30 PM 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.
> > 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>
> > ---
> > /* v2 */
> > - "Rafael J. Wysocki" <rafael@kernel.org>:
> >   -  Fold if (dev->power.wakeup && dev->power.wakeup->dev) check into
> >      if (device_can_wakeup(dev)) check since the former can never be true if
> >      the latter is false.
> >
> > - Christian Brauner <christian.brauner@ubuntu.com>:
> >   - Place (dev->power.wakeup && dev->power.wakeup->dev) check under
> >     CONFIG_PM_SLEEP ifdefine since it will wakeup_source will only be available
> >     when this config option is set.
> >
> > /* v3 */
> > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >    - Add explicit uid/gid parameters.
> > ---
> >  drivers/base/core.c        |  4 ++++
> >  drivers/base/power/power.h |  3 +++
> >  drivers/base/power/sysfs.c | 42 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index ec0d5e8cfd0f..efec2792f5d7 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3522,6 +3522,10 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> >         if (error)
> >                 goto out;
> >
> > +       error = dpm_sysfs_change_owner(dev, kuid, kgid);
> > +       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..54292cdd7808 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, kuid_t kuid, kgid_t kgid);
> >
> >  #else /* CONFIG_PM */
> >
> > @@ -88,6 +89,8 @@ 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, kuid_t kuid,
> > +                                        kgid_t kgid) { return 0; }
> >
> >  #endif
> >
> > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > index d7d82db2e4bc..4e79afcd5ca8 100644
> > --- a/drivers/base/power/sysfs.c
> > +++ b/drivers/base/power/sysfs.c
> > @@ -684,6 +684,48 @@ int dpm_sysfs_add(struct device *dev)
> >         return rc;
> >  }
> >
> > +int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > +{
> > +       int rc;
> > +
> > +       if (device_pm_not_required(dev))
> > +               return 0;
> > +
> > +       rc = sysfs_group_change_owner(&dev->kobj, &pm_attr_group, kuid, kgid);
> > +       if (rc)
> > +               return rc;
> > +
> > +       if (pm_runtime_callbacks_present(dev)) {
> > +               rc = sysfs_group_change_owner(
> > +                       &dev->kobj, &pm_runtime_attr_group, kuid, kgid);
> > +               if (rc)
> > +                       return rc;
> > +       }
> > +       if (device_can_wakeup(dev)) {
> > +               rc = sysfs_group_change_owner(&dev->kobj, &pm_wakeup_attr_group,
> > +                                             kuid, kgid);
> > +               if (rc)
> > +                       return rc;
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +               if (dev->power.wakeup && dev->power.wakeup->dev) {
> > +                       rc = device_change_owner(dev->power.wakeup->dev, kuid,
> > +                                                kgid);
> > +                       if (rc)
> > +                               return rc;
> > +               }
> > +#endif
> 
> First off, I don't particularly like #ifdefs in function bodies.  In
> particular, there is a CONFIG_PM_SLEEP block in this file already and
> you could define a new function in there to carry out the above
> operations, and provide an empty stub of it for the "unset" case.
> Failing to do so is somewhat on the "rushing things in" side in my
> view.

How ifdefines are used is highly dependent on the subsystem; networking
ofen uses in-place ifdefines in some parts and not in others. That has
nothing to do with rushing things. I'm happy to change it to your
preferences. Thanks for pointing out your expectations. But please don't
assume bad intentions on my part because I'm not meeting them right
away. It often is the case that adding a helper that is called in one
place is not well-received.

> 
> Second, the #ifdef should cover the entire if (device_can_wakeup(dev))
> {} block, because wakeup_sysfs_add() is only called if
> device_can_wakeup(dev) returns 'true' for the device in question (and
> arguably you could have checked that easily enough).

I've looked at the header definitions for device_can_wakeup() and with
and without CONFIG_PM_SLEEP it is defined as:

static inline bool device_can_wakeup(struct device *dev)
{
	return dev->power.can_wakeup;
}

which to me looks like it would neet to be called in all cases.

I'll rework this to you preferences.

Thanks!
Christian

Re: [PATCH net-next v3 6/9] drivers/base/power: add dpm_sysfs_change_owner()

[Rafael J. Wysocki]

On Thu, Feb 20, 2020 at 11:21 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Thu, Feb 20, 2020 at 11:02:04AM +0100, Rafael J. Wysocki wrote:
> > On Tue, Feb 18, 2020 at 5:30 PM 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.
> > > 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>
> > > ---
> > > /* v2 */
> > > - "Rafael J. Wysocki" <rafael@kernel.org>:
> > >   -  Fold if (dev->power.wakeup && dev->power.wakeup->dev) check into
> > >      if (device_can_wakeup(dev)) check since the former can never be true if
> > >      the latter is false.
> > >
> > > - Christian Brauner <christian.brauner@ubuntu.com>:
> > >   - Place (dev->power.wakeup && dev->power.wakeup->dev) check under
> > >     CONFIG_PM_SLEEP ifdefine since it will wakeup_source will only be available
> > >     when this config option is set.
> > >
> > > /* v3 */
> > > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> > >    - Add explicit uid/gid parameters.
> > > ---
> > >  drivers/base/core.c        |  4 ++++
> > >  drivers/base/power/power.h |  3 +++
> > >  drivers/base/power/sysfs.c | 42 ++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 49 insertions(+)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index ec0d5e8cfd0f..efec2792f5d7 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -3522,6 +3522,10 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > >         if (error)
> > >                 goto out;
> > >
> > > +       error = dpm_sysfs_change_owner(dev, kuid, kgid);
> > > +       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..54292cdd7808 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, kuid_t kuid, kgid_t kgid);
> > >
> > >  #else /* CONFIG_PM */
> > >
> > > @@ -88,6 +89,8 @@ 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, kuid_t kuid,
> > > +                                        kgid_t kgid) { return 0; }
> > >
> > >  #endif
> > >
> > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > > index d7d82db2e4bc..4e79afcd5ca8 100644
> > > --- a/drivers/base/power/sysfs.c
> > > +++ b/drivers/base/power/sysfs.c
> > > @@ -684,6 +684,48 @@ int dpm_sysfs_add(struct device *dev)
> > >         return rc;
> > >  }
> > >
> > > +int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > > +{
> > > +       int rc;
> > > +
> > > +       if (device_pm_not_required(dev))
> > > +               return 0;
> > > +
> > > +       rc = sysfs_group_change_owner(&dev->kobj, &pm_attr_group, kuid, kgid);
> > > +       if (rc)
> > > +               return rc;
> > > +
> > > +       if (pm_runtime_callbacks_present(dev)) {
> > > +               rc = sysfs_group_change_owner(
> > > +                       &dev->kobj, &pm_runtime_attr_group, kuid, kgid);
> > > +               if (rc)
> > > +                       return rc;
> > > +       }
> > > +       if (device_can_wakeup(dev)) {
> > > +               rc = sysfs_group_change_owner(&dev->kobj, &pm_wakeup_attr_group,
> > > +                                             kuid, kgid);
> > > +               if (rc)
> > > +                       return rc;
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +               if (dev->power.wakeup && dev->power.wakeup->dev) {
> > > +                       rc = device_change_owner(dev->power.wakeup->dev, kuid,
> > > +                                                kgid);
> > > +                       if (rc)
> > > +                               return rc;
> > > +               }
> > > +#endif
> >
> > First off, I don't particularly like #ifdefs in function bodies.  In
> > particular, there is a CONFIG_PM_SLEEP block in this file already and
> > you could define a new function in there to carry out the above
> > operations, and provide an empty stub of it for the "unset" case.
> > Failing to do so is somewhat on the "rushing things in" side in my
> > view.
>
> How ifdefines are used is highly dependent on the subsystem; networking
> ofen uses in-place ifdefines in some parts and not in others. That has
> nothing to do with rushing things. I'm happy to change it to your
> preferences.

Thanks!

> Thanks for pointing out your expectations. But please don't
> assume bad intentions on my part because I'm not meeting them right
> away. It often is the case that adding a helper that is called in one
> place is not well-received.

Fair enough, sorry for being harsh.

Re: [PATCH net-next v3 6/9] drivers/base/power: add dpm_sysfs_change_owner()

[Christian Brauner]

On Thu, Feb 20, 2020 at 11:30:32AM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 20, 2020 at 11:21 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > On Thu, Feb 20, 2020 at 11:02:04AM +0100, Rafael J. Wysocki wrote:
> > > On Tue, Feb 18, 2020 at 5:30 PM 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.
> > > > 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>
> > > > ---
> > > > /* v2 */
> > > > - "Rafael J. Wysocki" <rafael@kernel.org>:
> > > >   -  Fold if (dev->power.wakeup && dev->power.wakeup->dev) check into
> > > >      if (device_can_wakeup(dev)) check since the former can never be true if
> > > >      the latter is false.
> > > >
> > > > - Christian Brauner <christian.brauner@ubuntu.com>:
> > > >   - Place (dev->power.wakeup && dev->power.wakeup->dev) check under
> > > >     CONFIG_PM_SLEEP ifdefine since it will wakeup_source will only be available
> > > >     when this config option is set.
> > > >
> > > > /* v3 */
> > > > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> > > >    - Add explicit uid/gid parameters.
> > > > ---
> > > >  drivers/base/core.c        |  4 ++++
> > > >  drivers/base/power/power.h |  3 +++
> > > >  drivers/base/power/sysfs.c | 42 ++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 49 insertions(+)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index ec0d5e8cfd0f..efec2792f5d7 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -3522,6 +3522,10 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > > >         if (error)
> > > >                 goto out;
> > > >
> > > > +       error = dpm_sysfs_change_owner(dev, kuid, kgid);
> > > > +       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..54292cdd7808 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, kuid_t kuid, kgid_t kgid);
> > > >
> > > >  #else /* CONFIG_PM */
> > > >
> > > > @@ -88,6 +89,8 @@ 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, kuid_t kuid,
> > > > +                                        kgid_t kgid) { return 0; }
> > > >
> > > >  #endif
> > > >
> > > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > > > index d7d82db2e4bc..4e79afcd5ca8 100644
> > > > --- a/drivers/base/power/sysfs.c
> > > > +++ b/drivers/base/power/sysfs.c
> > > > @@ -684,6 +684,48 @@ int dpm_sysfs_add(struct device *dev)
> > > >         return rc;
> > > >  }
> > > >
> > > > +int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > > > +{
> > > > +       int rc;
> > > > +
> > > > +       if (device_pm_not_required(dev))
> > > > +               return 0;
> > > > +
> > > > +       rc = sysfs_group_change_owner(&dev->kobj, &pm_attr_group, kuid, kgid);
> > > > +       if (rc)
> > > > +               return rc;
> > > > +
> > > > +       if (pm_runtime_callbacks_present(dev)) {
> > > > +               rc = sysfs_group_change_owner(
> > > > +                       &dev->kobj, &pm_runtime_attr_group, kuid, kgid);
> > > > +               if (rc)
> > > > +                       return rc;
> > > > +       }
> > > > +       if (device_can_wakeup(dev)) {
> > > > +               rc = sysfs_group_change_owner(&dev->kobj, &pm_wakeup_attr_group,
> > > > +                                             kuid, kgid);
> > > > +               if (rc)
> > > > +                       return rc;
> > > > +
> > > > +#ifdef CONFIG_PM_SLEEP
> > > > +               if (dev->power.wakeup && dev->power.wakeup->dev) {
> > > > +                       rc = device_change_owner(dev->power.wakeup->dev, kuid,
> > > > +                                                kgid);
> > > > +                       if (rc)
> > > > +                               return rc;
> > > > +               }
> > > > +#endif
> > >
> > > First off, I don't particularly like #ifdefs in function bodies.  In
> > > particular, there is a CONFIG_PM_SLEEP block in this file already and
> > > you could define a new function in there to carry out the above
> > > operations, and provide an empty stub of it for the "unset" case.
> > > Failing to do so is somewhat on the "rushing things in" side in my
> > > view.
> >
> > How ifdefines are used is highly dependent on the subsystem; networking
> > ofen uses in-place ifdefines in some parts and not in others. That has
> > nothing to do with rushing things. I'm happy to change it to your
> > preferences.
> 
> Thanks!
> 
> > Thanks for pointing out your expectations. But please don't
> > assume bad intentions on my part because I'm not meeting them right
> > away. It often is the case that adding a helper that is called in one
> > place is not well-received.
> 
> Fair enough, sorry for being harsh.

Np, I didn't read it as such. I was really just worried you thought
that I was trying to rush things in. It's Thursday anyway, usually about
the time where we're all grumpy because we can't wait until we made it
to Friday. :)

Christian

Re: [PATCH net-next v3 1/9] sysfs: add sysfs_file_change_owner{_by_name}()

[Greg Kroah-Hartman]

On Tue, Feb 18, 2020 at 05:29:35PM +0100, Christian Brauner wrote:
> Add helpers to change the owner of a sysfs files.
> 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>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH net-next v3 1/9] sysfs: add sysfs_file_change_owner{_by_name}()

[Greg Kroah-Hartman]

On Tue, Feb 18, 2020 at 05:29:35PM +0100, Christian Brauner wrote:
> Add helpers to change the owner of a sysfs files.
> 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>
> ---
> /* v2 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Better naming for sysfs_file_change_owner() to reflect the fact that it
>      can be used to change the owner of the kobject itself by passing NULL as
>      argument.
> - Christian Brauner <christian.brauner@ubuntu.com>:
>   - Split sysfs_file_change_owner() into two helpers sysfs_change_owner() and
>     sysfs_change_owner_by_name(). The former changes the owner of the kobject
>     itself, the latter the owner of the kobject looked up via the name
>     argument.
> 
> /* v3 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Add explicit uid/gid parameters.

Looks much better, thanks for doing these changes.  I'll review the
whole series now...

greg k-h

Re: [PATCH net-next v3 2/9] sysfs: add sysfs_link_change_owner()

[Greg Kroah-Hartman]

On Tue, Feb 18, 2020 at 05:29:36PM +0100, Christian Brauner wrote:
> Add a helper to change the owner of a sysfs link.
> 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>
> ---
> /* v2 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Add comment how ownership of sysfs object is changed.
> 
> /* v3 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Add explicit uid/gid parameters.
> ---
>  fs/sysfs/file.c       | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h | 10 ++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 32bb04b4d9d9..df5107d7b3fd 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -570,6 +570,46 @@ 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.
> + *	@kuid:	new owner's kuid
> + *	@kgid:	new owner's kgid
> + */
> +int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
> +			    const char *name, kuid_t kuid, kgid_t kgid)
> +{
> +	struct kernfs_node *parent, *kn = NULL;
> +	int error;
> +
> +	if (!kobj)
> +		parent = sysfs_root_kn;
> +	else
> +		parent = kobj->sd;

I don't understand this, why would (!kobj) ever be a valid situation?

> +	if (!targ->state_in_sysfs)
> +		return -EINVAL;

Should you also check kobj->state_in_sysfs as well?

thanks,

greg k-h

Re: [PATCH net-next v3 3/9] sysfs: add sysfs_group{s}_change_owner()

[Greg Kroah-Hartman]

On Tue, Feb 18, 2020 at 05:29:37PM +0100, Christian Brauner wrote:
> Add helpers to change the owner of sysfs groups.
> 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>
> ---
> /* v2 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Add comment how ownership of sysfs object is changed.
> 
> /* v3 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Add explicit uid/gid parameters.
> - Christian Brauner <christian.brauner@ubuntu.com>:
>   - Collapse groups ownership helper patches into a single patch.
> ---
>  fs/sysfs/group.c      | 117 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  20 ++++++++
>  2 files changed, 137 insertions(+)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index c4ab045926b7..bae562d3cba1 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,119 @@ 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.
> + * @kuid:	new owner's kuid
> + * @kgid:	new owner's kgid
> + *
> + * Returns 0 on success or error code on failure.

This is fine to document, just funny it's the only one documented about
the return value so far in this series.

Anyway, looks good to me:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH net-next v3 1/9] sysfs: add sysfs_file_change_owner{_by_name}()

[Greg Kroah-Hartman]

On Tue, Feb 18, 2020 at 05:29:35PM +0100, Christian Brauner wrote:
> +/**
> + *	sysfs_file_change_owner - change owner of a file.
> + *	@kobj:	object.
> + *	@kuid: new owner's kuid
> + *	@kgid: new owner's kgid
> + */
> +int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
> +{
> +	struct kernfs_node *kn;
> +	int error;
> +
> +	if (!kobj->state_in_sysfs)
> +		return -EINVAL;
> +
> +	kernfs_get(kobj->sd);
> +
> +	kn = kobj->sd;
> +	error = internal_change_owner(kn, kobj, kuid, kgid);
> +
> +	kernfs_put(kn);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(sysfs_file_change_owner);

Oops, wait, what "file" are you changing here?  You aren't changing the
kobject's attributes, but rather a file in the kobject's directory,
right?  But kobj->sd is the directory of the kobject itself, so why
isn't this function just the same thing as sysfs_change_owner()?

Why would you call this function at all?

confused,

greg k-h

Re: [PATCH net-next v3 4/9] sysfs: add sysfs_change_owner()

[Greg Kroah-Hartman]

On Tue, Feb 18, 2020 at 05:29:38PM +0100, Christian Brauner wrote:
> Add a helper to change the owner of sysfs objects.
> 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>
> ---
> /* v2 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Add comment how ownership of sysfs object is changed.
> 
> /* v3 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Add explicit uid/gid parameters.
> ---
>  fs/sysfs/file.c       | 39 +++++++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  6 ++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index df5107d7b3fd..02f7e852aad4 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -665,3 +665,42 @@ int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
> +
> +/**
> + *	sysfs_change_owner - change owner of the given object.

"and all of the files associated with this kobject", right?

> + *	@kobj:	object.
> + *	@kuid:	new owner's kuid
> + *	@kgid:	new owner's kgid
> + */
> +int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
> +{
> +	int error;
> +	const struct kobj_type *ktype;
> +
> +	if (!kobj->state_in_sysfs)
> +		return -EINVAL;
> +
> +	error = sysfs_file_change_owner(kobj, kuid, kgid);

Ok, this changes the attributes of the sysfs directory for the kobject
itself.

> +	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_by_name(
> +				kobj, (*kattr)->name, kuid, kgid);
> +			if (error)
> +				return error;
> +		}

And here you change all of the files of the kobject.

But what about files that have a subdir?  Does that also happen here?

> +
> +		error = sysfs_groups_change_owner(kobj, ktype->default_groups,
> +						  kuid, kgid);

Then what are you changing here?

I think the kerneldoc needs a lot more explaination as to what is going
on in this function and why you would call it, and not some of the other
functions you are adding.

thanks,

greg k-h

Re: [PATCH net-next v3 5/9] device: add device_change_owner()

[Greg Kroah-Hartman]

On Tue, Feb 18, 2020 at 05:29:39PM +0100, Christian Brauner wrote:
> 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.
> 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>
> ---
> /* v2 */
> unchanged
> 
> /* v3 */
> -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>    - Add explicit uid/gid parameters.
> ---
>  drivers/base/core.c    | 80 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |  1 +
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 42a672456432..ec0d5e8cfd0f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3458,6 +3458,86 @@ int device_move(struct device *dev, struct device *new_parent,
>  }
>  EXPORT_SYMBOL_GPL(device_move);
>  
> +static int device_attrs_change_owner(struct device *dev, kuid_t kuid,
> +				     kgid_t kgid)
> +{
> +	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, kuid,
> +						  kgid);
> +		if (error)
> +			return error;
> +	}
> +
> +	if (type) {
> +		error = sysfs_groups_change_owner(kobj, type->groups, kuid,
> +						  kgid);
> +		if (error)
> +			return error;
> +	}
> +
> +	error = sysfs_groups_change_owner(kobj, dev->groups, kuid, kgid);
> +	if (error)
> +		return error;
> +
> +	if (device_supports_offline(dev) && !dev->offline_disabled) {
> +		error = sysfs_file_change_owner_by_name(
> +			kobj, dev_attr_online.attr.name, kuid, kgid);
> +		if (error)
> +			return error;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * device_change_owner - change the owner of an existing device.

The "owner" and what else gets changed here?  Please document this
better.


> + * @dev: device.
> + * @kuid: new owner's kuid
> + * @kgid: new owner's kgid
> + */
> +int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> +{
> +	int error;
> +	struct kobject *kobj = &dev->kobj;
> +
> +	dev = get_device(dev);
> +	if (!dev)
> +		return -EINVAL;
> +
> +	error = sysfs_change_owner(kobj, kuid, kgid);

the kobject of the device is changed, good.

> +	if (error)
> +		goto out;
> +
> +	error = sysfs_file_change_owner_by_name(kobj, dev_attr_uevent.attr.name,
> +						kuid, kgid);

Why call out the uevent file explicitly here?

> +	if (error)
> +		goto out;
> +
> +	error = device_attrs_change_owner(dev, kuid, kgid);
> +	if (error)
> +		goto out;

Doesn't this also change the uevent file?

> +
> +#ifdef CONFIG_BLOCK
> +	if (sysfs_deprecated && dev->class == &block_class)
> +		goto out;
> +#endif

Ugh, we still need this?

> +
> +	error = sysfs_link_change_owner(&dev->class->p->subsys.kobj, &dev->kobj,
> +					dev_name(dev), kuid, kgid);

Now what is this changing?

Again, more documentation please as to exactly what is being changed in
this function is needed.

thanks,

greg k-h

Re: [PATCH net-next v3 0/9] net: fix sysfs permssions when device changes network

[Greg KH]

On Wed, Feb 19, 2020 at 04:24:16PM -0800, David Miller wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> Date: Tue, 18 Feb 2020 17:29:34 +0100
> 
> > This is v3 with explicit uid and gid parameters added to functions that
> > change sysfs object ownership as Greg requested.
> 
> Greg, please review.

Give me a chance :)

It's looking better, still needs a little bit of work before I'm happy
with the driver core and sysfs bits, see my review comments so far.

thanks,

greg k-h

Re: [PATCH net-next v3 3/9] sysfs: add sysfs_group{s}_change_owner()

[Christian Brauner]

On Thu, Feb 20, 2020 at 12:15:50PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 18, 2020 at 05:29:37PM +0100, Christian Brauner wrote:
> > Add helpers to change the owner of sysfs groups.
> > 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>
> > ---
> > /* v2 */
> > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >    - Add comment how ownership of sysfs object is changed.
> > 
> > /* v3 */
> > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >    - Add explicit uid/gid parameters.
> > - Christian Brauner <christian.brauner@ubuntu.com>:
> >   - Collapse groups ownership helper patches into a single patch.
> > ---
> >  fs/sysfs/group.c      | 117 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/sysfs.h |  20 ++++++++
> >  2 files changed, 137 insertions(+)
> > 
> > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > index c4ab045926b7..bae562d3cba1 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,119 @@ 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.
> > + * @kuid:	new owner's kuid
> > + * @kgid:	new owner's kgid
> > + *
> > + * Returns 0 on success or error code on failure.
> 
> This is fine to document, just funny it's the only one documented about
> the return value so far in this series.

I stuck to the documentation style common to the file. Most of the
functions in fs/syfs/file.c did not mention return codes
sysfs_remove_bin_file(), sysfs_create_bin_file(),
sysfs_remove_file_from_group() etc. But I'll document all in this series
with return codes now.

Thanks!
Christian

Re: [PATCH net-next v3 2/9] sysfs: add sysfs_link_change_owner()

[Christian Brauner]

On Thu, Feb 20, 2020 at 12:14:43PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 18, 2020 at 05:29:36PM +0100, Christian Brauner wrote:
> > Add a helper to change the owner of a sysfs link.
> > 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>
> > ---
> > /* v2 */
> > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >    - Add comment how ownership of sysfs object is changed.
> > 
> > /* v3 */
> > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >    - Add explicit uid/gid parameters.
> > ---
> >  fs/sysfs/file.c       | 40 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/sysfs.h | 10 ++++++++++
> >  2 files changed, 50 insertions(+)
> > 
> > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > index 32bb04b4d9d9..df5107d7b3fd 100644
> > --- a/fs/sysfs/file.c
> > +++ b/fs/sysfs/file.c
> > @@ -570,6 +570,46 @@ 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.
> > + *	@kuid:	new owner's kuid
> > + *	@kgid:	new owner's kgid
> > + */
> > +int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
> > +			    const char *name, kuid_t kuid, kgid_t kgid)
> > +{
> > +	struct kernfs_node *parent, *kn = NULL;
> > +	int error;
> > +
> > +	if (!kobj)
> > +		parent = sysfs_root_kn;
> > +	else
> > +		parent = kobj->sd;
> 
> I don't understand this, why would (!kobj) ever be a valid situation?

Yeah, a caller could just pass in "sysfs_root_kn" itself if they for
some reason needed to.

> 
> > +	if (!targ->state_in_sysfs)
> > +		return -EINVAL;
> 
> Should you also check kobj->state_in_sysfs as well?

Probably. I'll take a closer look now.

Thanks!
Christian

Re: [PATCH net-next v3 4/9] sysfs: add sysfs_change_owner()

[Christian Brauner]

On Thu, Feb 20, 2020 at 12:23:14PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 18, 2020 at 05:29:38PM +0100, Christian Brauner wrote:
> > Add a helper to change the owner of sysfs objects.
> > 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>
> > ---
> > /* v2 */
> > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >    - Add comment how ownership of sysfs object is changed.
> > 
> > /* v3 */
> > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >    - Add explicit uid/gid parameters.
> > ---
> >  fs/sysfs/file.c       | 39 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/sysfs.h |  6 ++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > index df5107d7b3fd..02f7e852aad4 100644
> > --- a/fs/sysfs/file.c
> > +++ b/fs/sysfs/file.c
> > @@ -665,3 +665,42 @@ int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
> >  	return error;
> >  }
> >  EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
> > +
> > +/**
> > + *	sysfs_change_owner - change owner of the given object.
> 
> "and all of the files associated with this kobject", right?
> 
> > + *	@kobj:	object.
> > + *	@kuid:	new owner's kuid
> > + *	@kgid:	new owner's kgid
> > + */
> > +int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
> > +{
> > +	int error;
> > +	const struct kobj_type *ktype;
> > +
> > +	if (!kobj->state_in_sysfs)
> > +		return -EINVAL;
> > +
> > +	error = sysfs_file_change_owner(kobj, kuid, kgid);
> 
> Ok, this changes the attributes of the sysfs directory for the kobject
> itself.

Yes.

> 
> > +	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_by_name(
> > +				kobj, (*kattr)->name, kuid, kgid);
> > +			if (error)
> > +				return error;
> > +		}
> 
> And here you change all of the files of the kobject.

This changes the default attributes associated with that ktype (if any)
and mirrors how a kobject is registered in sysfs.

> 
> But what about files that have a subdir?  Does that also happen here?

Maybe that was all to brief on my end, sorry:
So all of this mirrors how a kobject is added through driver core which
in its guts is done via kobject_add_internal() which in summary does:
- create the main directory via create_dir()
- populate the directory with the groups associated with that ktype (if any)
- populate the directory with the basic attributes associated with that
  ktype (if any)
These are the basic steps that are associated with adding a kobject in
sysfs.
Any additional properties are added by the specific subsystem
itself (not by driver core) after it has registered the device. So for
the example of network devices, a network device will e.g. register a
queue subdirectory under the basic sysfs directory for the network
device and than further subdirectories within that queues subdirectory.
But that is all specific to network devices and they call the
corresponding sysfs functions to do that directly when they create those
queue objects. So anything that a subsystem adds outside of what driver
core does must also be changed by them (That's already true for removal
of files it created outside of driver core.) and it's the same for
ownership changes. :)

I'll document that.

> 
> > +
> > +		error = sysfs_groups_change_owner(kobj, ktype->default_groups,
> > +						  kuid, kgid);
> 
> Then what are you changing here?

So this changes the default groups associated with the ktype for that
kobject and again mirrors how a kobject is registered in sysfs.

> 
> I think the kerneldoc needs a lot more explaination as to what is going
> on in this function and why you would call it, and not some of the other
> functions you are adding.

Will do for sure.

Thanks!
Christian

Re: [PATCH net-next v3 1/9] sysfs: add sysfs_file_change_owner{_by_name}()

[Christian Brauner]

On Thu, Feb 20, 2020 at 12:20:49PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 18, 2020 at 05:29:35PM +0100, Christian Brauner wrote:
> > +/**
> > + *	sysfs_file_change_owner - change owner of a file.
> > + *	@kobj:	object.
> > + *	@kuid: new owner's kuid
> > + *	@kgid: new owner's kgid
> > + */
> > +int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
> > +{
> > +	struct kernfs_node *kn;
> > +	int error;
> > +
> > +	if (!kobj->state_in_sysfs)
> > +		return -EINVAL;
> > +
> > +	kernfs_get(kobj->sd);
> > +
> > +	kn = kobj->sd;
> > +	error = internal_change_owner(kn, kobj, kuid, kgid);
> > +
> > +	kernfs_put(kn);
> > +
> > +	return error;
> > +}
> > +EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
> 
> Oops, wait, what "file" are you changing here?  You aren't changing the
> kobject's attributes, but rather a file in the kobject's directory,
> right?  But kobj->sd is the directory of the kobject itself, so why
> isn't this function just the same thing as sysfs_change_owner()?

I've moved it directly into sysfs_change_owner(), removed the function,
and renamed "_by_name()" back to sysfs_file_change_owner() which is
easier to parse and makes more sense.

Re: [PATCH net-next v3 5/9] device: add device_change_owner()

[Christian Brauner]

On Thu, Feb 20, 2020 at 12:25:13PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 18, 2020 at 05:29:39PM +0100, Christian Brauner wrote:
> > 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.
> > 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>
> > ---
> > /* v2 */
> > unchanged
> > 
> > /* v3 */
> > -  Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >    - Add explicit uid/gid parameters.
> > ---
> >  drivers/base/core.c    | 80 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/device.h |  1 +
> >  2 files changed, 81 insertions(+)
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 42a672456432..ec0d5e8cfd0f 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3458,6 +3458,86 @@ int device_move(struct device *dev, struct device *new_parent,
> >  }
> >  EXPORT_SYMBOL_GPL(device_move);
> >  
> > +static int device_attrs_change_owner(struct device *dev, kuid_t kuid,
> > +				     kgid_t kgid)
> > +{
> > +	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, kuid,
> > +						  kgid);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	if (type) {
> > +		error = sysfs_groups_change_owner(kobj, type->groups, kuid,
> > +						  kgid);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	error = sysfs_groups_change_owner(kobj, dev->groups, kuid, kgid);
> > +	if (error)
> > +		return error;
> > +
> > +	if (device_supports_offline(dev) && !dev->offline_disabled) {
> > +		error = sysfs_file_change_owner_by_name(
> > +			kobj, dev_attr_online.attr.name, kuid, kgid);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * device_change_owner - change the owner of an existing device.
> 
> The "owner" and what else gets changed here?  Please document this
> better.
> 
> 
> > + * @dev: device.
> > + * @kuid: new owner's kuid
> > + * @kgid: new owner's kgid
> > + */
> > +int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > +{
> > +	int error;
> > +	struct kobject *kobj = &dev->kobj;
> > +
> > +	dev = get_device(dev);
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	error = sysfs_change_owner(kobj, kuid, kgid);
> 
> the kobject of the device is changed, good.
> 
> > +	if (error)
> > +		goto out;
> > +
> > +	error = sysfs_file_change_owner_by_name(kobj, dev_attr_uevent.attr.name,
> > +						kuid, kgid);
> 
> Why call out the uevent file explicitly here?

This again, mirrors the creation of a kobject in sysfs. The uevent file
is created separately and thus should be chowned separately.

> 
> > +	if (error)
> > +		goto out;
> > +
> > +	error = device_attrs_change_owner(dev, kuid, kgid);
> > +	if (error)
> > +		goto out;
> 
> Doesn't this also change the uevent file?

No, not as far as I can tell. The uevent file is created in an extra
step when the kobject/sysfs entries are created.

> 
> > +
> > +#ifdef CONFIG_BLOCK
> > +	if (sysfs_deprecated && dev->class == &block_class)
> > +		goto out;
> > +#endif
> 
> Ugh, we still need this?

Yeah, apparently. It's what I gather from how a device is added.

> 
> > +
> > +	error = sysfs_link_change_owner(&dev->class->p->subsys.kobj, &dev->kobj,
> > +					dev_name(dev), kuid, kgid);
> 
> Now what is this changing?

So, this changed the ownership of the class link for the device to match
the directory entry for that device, so e.g. given a network device
symlink (or any other type) that points to the actual directory entry
for that device:

/sys/class/net/my-dev -> ../../devices/virtual/net/my-dev

it makes my-dev show the same permissions as the directory my-dev has.
If we don't do this this will look weird, because the symlink will show
different permissions than the target it is pointoing to.

> 
> Again, more documentation please as to exactly what is being changed in
> this function is needed.

Sure!