[PATCH 0/4] nfs: kobject: use generic helpers and ownership

[PATCH 0/4] nfs: kobject: use generic helpers and ownership

[Alexander Aring]

Hi,

I currently have pending patches for fs/dlm (Distributed Lock Manager)
subsystem to introduce some helpers to udev. However, it seems it takes
more time that I can bring those changes upstream. I put those out now
and already figured out that nfs can also take advantage of those changes.

With this patch-series I try to try to reduce my patch-series for DLM
and already bring part of it upstream and nfs will be a user of it.

The ownership callback, I think it should be set as the
kset_create_and_add() sets this callback as default. I never had any
issues with it, but there might be container corner cases that requires
those changes?

- Alex

Alexander Aring (4):
  kobject: add kset_type_create_and_add() helper
  kobject: export generic helper ops
  nfs: sysfs: use kset_type_create_and_add()
  nfs: sysfs: use default get_ownership() callback

 fs/nfs/sysfs.c          | 30 +++----------------
 include/linux/kobject.h | 10 +++++--
 lib/kobject.c           | 65 ++++++++++++++++++++++++++++++-----------
 3 files changed, 60 insertions(+), 45 deletions(-)

-- 
2.43.0

[PATCH 1/4] kobject: add kset_type_create_and_add() helper

[Alexander Aring]

Currently there exists the kset_create_and_add() helper that does not
allow to have a different ktype for the created kset kobject. To allow
a different ktype this patch will introduce the function
kset_type_create_and_add() that allows to set a different ktype instead
of using the global default kset_ktype variable.

In my example I need to separate the created kobject inside the kset by
net-namespaces. This patch allows me to do that by providing a user
defined kobj_type structure that implements the necessary namespace
functionality.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 include/linux/kobject.h |  8 ++++--
 lib/kobject.c           | 59 ++++++++++++++++++++++++++++++-----------
 2 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index c8219505a79f..7504b7547ed2 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -175,8 +175,12 @@ struct kset {
 void kset_init(struct kset *kset);
 int __must_check kset_register(struct kset *kset);
 void kset_unregister(struct kset *kset);
-struct kset * __must_check kset_create_and_add(const char *name, const struct kset_uevent_ops *u,
-					       struct kobject *parent_kobj);
+struct kset * __must_check
+kset_type_create_and_add(const char *name, const struct kset_uevent_ops *u,
+			 struct kobject *parent_kobj, const struct kobj_type *ktype);
+struct kset * __must_check
+kset_create_and_add(const char *name, const struct kset_uevent_ops *u,
+		    struct kobject *parent_kobj);
 
 static inline struct kset *to_kset(struct kobject *kobj)
 {
diff --git a/lib/kobject.c b/lib/kobject.c
index 72fa20f405f1..09dd3d4c7f56 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -946,6 +946,7 @@ static const struct kobj_type kset_ktype = {
  * @name: the name for the kset
  * @uevent_ops: a struct kset_uevent_ops for the kset
  * @parent_kobj: the parent kobject of this kset, if any.
+ * @ktype: a struct kobj_type for the kset
  *
  * This function creates a kset structure dynamically.  This structure can
  * then be registered with the system and show up in sysfs with a call to
@@ -957,7 +958,8 @@ static const struct kobj_type kset_ktype = {
  */
 static struct kset *kset_create(const char *name,
 				const struct kset_uevent_ops *uevent_ops,
-				struct kobject *parent_kobj)
+				struct kobject *parent_kobj,
+				const struct kobj_type *ktype)
 {
 	struct kset *kset;
 	int retval;
@@ -973,39 +975,38 @@ static struct kset *kset_create(const char *name,
 	kset->uevent_ops = uevent_ops;
 	kset->kobj.parent = parent_kobj;
 
-	/*
-	 * The kobject of this kset will have a type of kset_ktype and belong to
-	 * no kset itself.  That way we can properly free it when it is
-	 * finished being used.
-	 */
-	kset->kobj.ktype = &kset_ktype;
+	kset->kobj.ktype = ktype;
 	kset->kobj.kset = NULL;
 
 	return kset;
 }
 
 /**
- * kset_create_and_add() - Create a struct kset dynamically and add it to sysfs.
+ * kset_type_create_and_add() - Create a struct kset with kobj_type dynamically
+ *                              and add it to sysfs.
  *
  * @name: the name for the kset
  * @uevent_ops: a struct kset_uevent_ops for the kset
  * @parent_kobj: the parent kobject of this kset, if any.
+ * @ktype: a struct kobj_type for the kset
  *
- * This function creates a kset structure dynamically and registers it
- * with sysfs.  When you are finished with this structure, call
+ * This function creates a kset structure with ktype structure dynamically and
+ * registers it with sysfs.  When you are finished with this structure, call
  * kset_unregister() and the structure will be dynamically freed when it
- * is no longer being used.
+ * is no longer being used. Works like kset_create_and_add() just with the
+ * possibility to assign kobj_type to the kset.
  *
  * If the kset was not able to be created, NULL will be returned.
  */
-struct kset *kset_create_and_add(const char *name,
-				 const struct kset_uevent_ops *uevent_ops,
-				 struct kobject *parent_kobj)
+struct kset *kset_type_create_and_add(const char *name,
+				      const struct kset_uevent_ops *uevent_ops,
+				      struct kobject *parent_kobj,
+				      const struct kobj_type *ktype)
 {
 	struct kset *kset;
 	int error;
 
-	kset = kset_create(name, uevent_ops, parent_kobj);
+	kset = kset_create(name, uevent_ops, parent_kobj, ktype);
 	if (!kset)
 		return NULL;
 	error = kset_register(kset);
@@ -1015,6 +1016,34 @@ struct kset *kset_create_and_add(const char *name,
 	}
 	return kset;
 }
+EXPORT_SYMBOL_GPL(kset_type_create_and_add);
+
+/**
+ * kset_create_and_add() - Create a struct kset dynamically and add it to sysfs.
+ *
+ * @name: the name for the kset
+ * @uevent_ops: a struct kset_uevent_ops for the kset
+ * @parent_kobj: the parent kobject of this kset, if any.
+ *
+ * This function creates a kset structure dynamically and registers it
+ * with sysfs.  When you are finished with this structure, call
+ * kset_unregister() and the structure will be dynamically freed when it
+ * is no longer being used.
+ *
+ * If the kset was not able to be created, NULL will be returned.
+ */
+struct kset *kset_create_and_add(const char *name,
+				 const struct kset_uevent_ops *uevent_ops,
+				 struct kobject *parent_kobj)
+{
+	/*
+	 * The kobject of this kset will have a type of kset_ktype and belong to
+	 * no kset itself.  That way we can properly free it when it is
+	 * finished being used.
+	 */
+	return kset_type_create_and_add(name, uevent_ops, parent_kobj,
+					&kset_ktype);
+}
 EXPORT_SYMBOL_GPL(kset_create_and_add);
 
 
-- 
2.43.0

[PATCH 2/4] kobject: export generic helper ops

[Alexander Aring]

This patch exports generic helpers like kset_release() and
kset_get_ownership() so users can use them in their own struct kobj_type
implementation instead of implementing their own functions that do the
same.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 include/linux/kobject.h | 2 ++
 lib/kobject.c           | 6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 7504b7547ed2..5fbc358e2be6 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -181,6 +181,8 @@ kset_type_create_and_add(const char *name, const struct kset_uevent_ops *u,
 struct kset * __must_check
 kset_create_and_add(const char *name, const struct kset_uevent_ops *u,
 		    struct kobject *parent_kobj);
+void kset_release(struct kobject *kobj);
+void kset_get_ownership(const struct kobject *kobj, kuid_t *uid, kgid_t *gid);
 
 static inline struct kset *to_kset(struct kobject *kobj)
 {
diff --git a/lib/kobject.c b/lib/kobject.c
index 09dd3d4c7f56..ccd2f6282c81 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -920,19 +920,21 @@ struct kobject *kset_find_obj(struct kset *kset, const char *name)
 }
 EXPORT_SYMBOL_GPL(kset_find_obj);
 
-static void kset_release(struct kobject *kobj)
+void kset_release(struct kobject *kobj)
 {
 	struct kset *kset = container_of(kobj, struct kset, kobj);
 	pr_debug("'%s' (%p): %s\n",
 		 kobject_name(kobj), kobj, __func__);
 	kfree(kset);
 }
+EXPORT_SYMBOL_GPL(kset_release);
 
-static void kset_get_ownership(const struct kobject *kobj, kuid_t *uid, kgid_t *gid)
+void kset_get_ownership(const struct kobject *kobj, kuid_t *uid, kgid_t *gid)
 {
 	if (kobj->parent)
 		kobject_get_ownership(kobj->parent, uid, gid);
 }
+EXPORT_SYMBOL_GPL(kset_get_ownership);
 
 static const struct kobj_type kset_ktype = {
 	.sysfs_ops	= &kobj_sysfs_ops,
-- 
2.43.0

[PATCH 3/4] nfs: sysfs: use kset_type_create_and_add()

[Alexander Aring]

Since we have the new udev helper kset_type_create_and_add() helper we
can use it as it allows to have an own kobj_type ktype that is necessary
for nfs for the nfs_netns_object_child_ns_type() kset type callback.

We lose some errno information related to kset_register() in probably
non-existing cases. We return always -ENOMEM as other uses of
kset_create_and_add() does.

The nfs_kset_release() can be replaced by the default udev helper
kset_release() as it does the same functionality.

Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 fs/nfs/sysfs.c | 29 +++--------------------------
 1 file changed, 3 insertions(+), 26 deletions(-)

diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
index bf378ecd5d9f..a6584203b7ff 100644
--- a/fs/nfs/sysfs.c
+++ b/fs/nfs/sysfs.c
@@ -20,12 +20,6 @@
 
 static struct kset *nfs_kset;
 
-static void nfs_kset_release(struct kobject *kobj)
-{
-	struct kset *kset = container_of(kobj, struct kset, kobj);
-	kfree(kset);
-}
-
 static const struct kobj_ns_type_operations *nfs_netns_object_child_ns_type(
 		const struct kobject *kobj)
 {
@@ -33,35 +27,18 @@ static const struct kobj_ns_type_operations *nfs_netns_object_child_ns_type(
 }
 
 static struct kobj_type nfs_kset_type = {
-	.release = nfs_kset_release,
+	.release = kset_release,
 	.sysfs_ops = &kobj_sysfs_ops,
 	.child_ns_type = nfs_netns_object_child_ns_type,
 };
 
 int nfs_sysfs_init(void)
 {
-	int ret;
-
-	nfs_kset = kzalloc(sizeof(*nfs_kset), GFP_KERNEL);
+	nfs_kset = kset_type_create_and_add("nfs", NULL, fs_kobj,
+					    &nfs_kset_type);
 	if (!nfs_kset)
 		return -ENOMEM;
 
-	ret = kobject_set_name(&nfs_kset->kobj, "nfs");
-	if (ret) {
-		kfree(nfs_kset);
-		return ret;
-	}
-
-	nfs_kset->kobj.parent = fs_kobj;
-	nfs_kset->kobj.ktype = &nfs_kset_type;
-	nfs_kset->kobj.kset = NULL;
-
-	ret = kset_register(nfs_kset);
-	if (ret) {
-		kfree(nfs_kset);
-		return ret;
-	}
-
 	return 0;
 }
 
-- 
2.43.0

[PATCH 4/4] nfs: sysfs: use default get_ownership() callback

[Alexander Aring]

Since commit 5f81880d5204 ("sysfs, kobject: allow creating kobject
belonging to arbitrary users") it seems that there could be cases for
kobjects belonging to arbitrary users. This callback is set by default
when using kset_create_and_add() to allow creating kobjects with
different ownerships according to its parent.

This patch will assign the default callback now for nfs kobjects for
cases when the parent has different ownership than the default one.

Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 fs/nfs/sysfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
index a6584203b7ff..b5737464b892 100644
--- a/fs/nfs/sysfs.c
+++ b/fs/nfs/sysfs.c
@@ -27,6 +27,7 @@ static const struct kobj_ns_type_operations *nfs_netns_object_child_ns_type(
 }
 
 static struct kobj_type nfs_kset_type = {
+	.get_ownership = kset_get_ownership,
 	.release = kset_release,
 	.sysfs_ops = &kobj_sysfs_ops,
 	.child_ns_type = nfs_netns_object_child_ns_type,
-- 
2.43.0

Re: [PATCH 0/4] nfs: kobject: use generic helpers and ownership

[Alexander Aring]

Dear nfs tree maintainers,

On Thu, Oct 3, 2024 at 11:15 AM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> I currently have pending patches for fs/dlm (Distributed Lock Manager)
> subsystem to introduce some helpers to udev. However, it seems it takes
> more time that I can bring those changes upstream. I put those out now
> and already figured out that nfs can also take advantage of those changes.
>
> With this patch-series I try to try to reduce my patch-series for DLM
> and already bring part of it upstream and nfs will be a user of it.
>
> The ownership callback, I think it should be set as the
> kset_create_and_add() sets this callback as default. I never had any
> issues with it, but there might be container corner cases that requires
> those changes?
>
> - Alex
>

Can we have those patches applied to the nfs tree? According to the
udev/kobject maintainer it is fine to take this in any tree that it
needs to go through. [0]
I made nfs as a first user of these new udev helpers functionality, so
it should go through the nfs tree if there are no other issues with
this series?

Thanks.

- Alex

[0] https://lore.kernel.org/gfs2/2024081519-caddy-monstrous-b37d@gregkh/

Re: [PATCH 4/4] nfs: sysfs: use default get_ownership() callback

[Benjamin Coddington]

On 3 Oct 2024, at 11:14, Alexander Aring wrote:

> Since commit 5f81880d5204 ("sysfs, kobject: allow creating kobject
> belonging to arbitrary users") it seems that there could be cases for
> kobjects belonging to arbitrary users. This callback is set by default
> when using kset_create_and_add() to allow creating kobjects with
> different ownerships according to its parent.
>
> This patch will assign the default callback now for nfs kobjects for
> cases when the parent has different ownership than the default one.
>
> Signed-off-by: Alexander Aring <aahringo@redhat.com>
> ---
>  fs/nfs/sysfs.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
> index a6584203b7ff..b5737464b892 100644
> --- a/fs/nfs/sysfs.c
> +++ b/fs/nfs/sysfs.c
> @@ -27,6 +27,7 @@ static const struct kobj_ns_type_operations *nfs_netns_object_child_ns_type(
>  }
>
>  static struct kobj_type nfs_kset_type = {
> +	.get_ownership = kset_get_ownership,
>  	.release = kset_release,
>  	.sysfs_ops = &kobj_sysfs_ops,
>  	.child_ns_type = nfs_netns_object_child_ns_type,
> -- 
> 2.43.0

Hi Alex, if I understand this correctly, this patch just punts the ownership
callback up to fs_kobj, which, because it has no .get_ownership is just
going to be the same result: root.

Does this patch add value?

Ben

Re: [PATCH 0/4] nfs: kobject: use generic helpers and ownership

[Benjamin Coddington]

On 3 Oct 2024, at 11:14, Alexander Aring wrote:

> Hi,
>
> I currently have pending patches for fs/dlm (Distributed Lock Manager)
> subsystem to introduce some helpers to udev. However, it seems it takes
> more time that I can bring those changes upstream. I put those out now
> and already figured out that nfs can also take advantage of those changes.
>
> With this patch-series I try to try to reduce my patch-series for DLM
> and already bring part of it upstream and nfs will be a user of it.
>
> The ownership callback, I think it should be set as the
> kset_create_and_add() sets this callback as default. I never had any
> issues with it, but there might be container corner cases that requires
> those changes?
>
> - Alex
>
> Alexander Aring (4):
>   kobject: add kset_type_create_and_add() helper
>   kobject: export generic helper ops
>   nfs: sysfs: use kset_type_create_and_add()
>   nfs: sysfs: use default get_ownership() callback
>
>  fs/nfs/sysfs.c          | 30 +++----------------
>  include/linux/kobject.h | 10 +++++--
>  lib/kobject.c           | 65 ++++++++++++++++++++++++++++++-----------
>  3 files changed, 60 insertions(+), 45 deletions(-)
>
> -- 
> 2.43.0

These look good to me, though patch 4/4 seems superfluous, I responded there.

For the series:
Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben

Re: [PATCH 4/4] nfs: sysfs: use default get_ownership() callback

[Alexander Aring]

Hi,

On Tue, Oct 8, 2024 at 4:09 PM Benjamin Coddington <bcodding@redhat.com> wrote:
>
> On 3 Oct 2024, at 11:14, Alexander Aring wrote:
>
> > Since commit 5f81880d5204 ("sysfs, kobject: allow creating kobject
> > belonging to arbitrary users") it seems that there could be cases for
> > kobjects belonging to arbitrary users. This callback is set by default
> > when using kset_create_and_add() to allow creating kobjects with
> > different ownerships according to its parent.
> >
> > This patch will assign the default callback now for nfs kobjects for
> > cases when the parent has different ownership than the default one.
> >
> > Signed-off-by: Alexander Aring <aahringo@redhat.com>
> > ---
> >  fs/nfs/sysfs.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
> > index a6584203b7ff..b5737464b892 100644
> > --- a/fs/nfs/sysfs.c
> > +++ b/fs/nfs/sysfs.c
> > @@ -27,6 +27,7 @@ static const struct kobj_ns_type_operations *nfs_netns_object_child_ns_type(
> >  }
> >
> >  static struct kobj_type nfs_kset_type = {
> > +     .get_ownership = kset_get_ownership,
> >       .release = kset_release,
> >       .sysfs_ops = &kobj_sysfs_ops,
> >       .child_ns_type = nfs_netns_object_child_ns_type,
> > --
> > 2.43.0
>
> Hi Alex, if I understand this correctly, this patch just punts the ownership
> callback up to fs_kobj, which, because it has no .get_ownership is just
> going to be the same result: root.
>

Yes, from my understanding this has to do with user namespaces and
being able to write to the related sysfs/kobject files?

> Does this patch add value?
>

For me no, but it is there now as default and I think somebody
probably forgot to update all the other users that created their kset
in a more low-level kind of way.
I think the whole "transitioning process" to make the same things with
namespaces created under root vs user is still kind of still in
process.

I am not sure if any of the nfs userspace namespace aware tools are
also limited by other things that require additional changes to do
something else as a "normal" user.

For me it's fine to drop this patch series and if somebody wants to
make a user namespace working with nfs tooling can come back again and
find out such patch might be necessary?

Thanks.

- Alex

Re: [PATCH 0/4] nfs: kobject: use generic helpers and ownership

[Anna Schumaker]

On 10/8/24 4:12 PM, Benjamin Coddington wrote:
> On 3 Oct 2024, at 11:14, Alexander Aring wrote:
> 
>> Hi,
>>
>> I currently have pending patches for fs/dlm (Distributed Lock Manager)
>> subsystem to introduce some helpers to udev. However, it seems it takes
>> more time that I can bring those changes upstream. I put those out now
>> and already figured out that nfs can also take advantage of those changes.
>>
>> With this patch-series I try to try to reduce my patch-series for DLM
>> and already bring part of it upstream and nfs will be a user of it.
>>
>> The ownership callback, I think it should be set as the
>> kset_create_and_add() sets this callback as default. I never had any
>> issues with it, but there might be container corner cases that requires
>> those changes?
>>
>> - Alex
>>
>> Alexander Aring (4):
>>   kobject: add kset_type_create_and_add() helper
>>   kobject: export generic helper ops
>>   nfs: sysfs: use kset_type_create_and_add()
>>   nfs: sysfs: use default get_ownership() callback
>>
>>  fs/nfs/sysfs.c          | 30 +++----------------
>>  include/linux/kobject.h | 10 +++++--
>>  lib/kobject.c           | 65 ++++++++++++++++++++++++++++++-----------
>>  3 files changed, 60 insertions(+), 45 deletions(-)
>>
>> -- 
>> 2.43.0
> 
> These look good to me, though patch 4/4 seems superfluous, I responded there.

They look good to me, too. They're not really a bugfix, so they'll probably go in through v6.13 (assuming Trond has no objections, it's his turn to do the merge) and not v6.12-rc.

Anna

> 
> For the series:
> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
> 
> Ben
>