Add kobject_is_added() to avoid consumers from directly accessing
the internal variables of kobject.
Signed-off-by: Yangtao Li <[email protected]>
---
include/linux/kobject.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index bdab370a24f4..b5cdb0c58729 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -203,6 +203,11 @@ static inline const struct kobj_type *get_ktype(const struct kobject *kobj)
return kobj->ktype;
}
+static inline int kobject_is_added(struct kobject *kobj)
+{
+ return kobj->state_in_sysfs;
+}
+
extern struct kobject *kset_find_obj(struct kset *, const char *);
/* The global /sys/kernel/ kobject for people to chain off of */
--
2.35.1
Use kobject_is_added() instead of local `s_sysfs_registered` variables.
BTW kill kobject_del() directly, because kobject_put() actually covers
kobject removal automatically.
Signed-off-by: Yangtao Li <[email protected]>
---
fs/zonefs/sysfs.c | 11 +++++------
fs/zonefs/zonefs.h | 1 -
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
index 8ccb65c2b419..f0783bf7a25c 100644
--- a/fs/zonefs/sysfs.c
+++ b/fs/zonefs/sysfs.c
@@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
return ret;
}
- sbi->s_sysfs_registered = true;
-
return 0;
}
@@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
{
struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
- if (!sbi || !sbi->s_sysfs_registered)
+ if (!sbi)
return;
- kobject_del(&sbi->s_kobj);
- kobject_put(&sbi->s_kobj);
- wait_for_completion(&sbi->s_kobj_unregister);
+ if (kobject_is_added(&sbi->s_kobj)) {
+ kobject_put(&sbi->s_kobj);
+ wait_for_completion(&sbi->s_kobj_unregister);
+ }
}
int __init zonefs_sysfs_init(void)
diff --git a/fs/zonefs/zonefs.h b/fs/zonefs/zonefs.h
index 8175652241b5..4db0ea173220 100644
--- a/fs/zonefs/zonefs.h
+++ b/fs/zonefs/zonefs.h
@@ -238,7 +238,6 @@ struct zonefs_sb_info {
unsigned int s_max_active_seq_files;
atomic_t s_active_seq_files;
- bool s_sysfs_registered;
struct kobject s_kobj;
struct completion s_kobj_unregister;
};
--
2.35.1
Use kobject_is_added() instead of directly accessing the internal
variables of kobject. BTW kill kobject_del() directly, because
kobject_put() actually covers kobject removal automatically.
Signed-off-by: Yangtao Li <[email protected]>
---
fs/erofs/sysfs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
index 435e515c0792..daac23e32026 100644
--- a/fs/erofs/sysfs.c
+++ b/fs/erofs/sysfs.c
@@ -240,8 +240,7 @@ void erofs_unregister_sysfs(struct super_block *sb)
{
struct erofs_sb_info *sbi = EROFS_SB(sb);
- if (sbi->s_kobj.state_in_sysfs) {
- kobject_del(&sbi->s_kobj);
+ if (kobject_is_added(&sbi->s_kobj)) {
kobject_put(&sbi->s_kobj);
wait_for_completion(&sbi->s_kobj_unregister);
}
--
2.35.1
On Thu, Apr 06, 2023 at 05:30:54PM +0800, Yangtao Li wrote:
> Add kobject_is_added() to avoid consumers from directly accessing
> the internal variables of kobject.
>
> Signed-off-by: Yangtao Li <[email protected]>
> ---
> include/linux/kobject.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index bdab370a24f4..b5cdb0c58729 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -203,6 +203,11 @@ static inline const struct kobj_type *get_ktype(const struct kobject *kobj)
> return kobj->ktype;
> }
>
> +static inline int kobject_is_added(struct kobject *kobj)
> +{
> + return kobj->state_in_sysfs;
> +}
> +
No, this implies that the caller is not doing something correctly as it
should always know if it has added a kobject or not. Let me review the
please where you used this to find the problems there...
thanks,
greg k-h
On Thu, Apr 06, 2023 at 05:30:55PM +0800, Yangtao Li wrote:
> Use kobject_is_added() instead of directly accessing the internal
> variables of kobject. BTW kill kobject_del() directly, because
> kobject_put() actually covers kobject removal automatically.
>
> Signed-off-by: Yangtao Li <[email protected]>
> ---
> fs/erofs/sysfs.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
> index 435e515c0792..daac23e32026 100644
> --- a/fs/erofs/sysfs.c
> +++ b/fs/erofs/sysfs.c
> @@ -240,8 +240,7 @@ void erofs_unregister_sysfs(struct super_block *sb)
> {
> struct erofs_sb_info *sbi = EROFS_SB(sb);
>
> - if (sbi->s_kobj.state_in_sysfs) {
> - kobject_del(&sbi->s_kobj);
> + if (kobject_is_added(&sbi->s_kobj)) {
I do not understand why this check is even needed, I do not think it
should be there at all as obviously the kobject was registered if it now
needs to not be registered.
Meta-comment, we need to come up with a "filesystem kobject type" to get
rid of lots of the boilerplate filesystem kobject logic as it's
duplicated in every filesystem in tiny different ways and lots of times
(like here), it's wrong.
kobjects were not designed to be "used raw" like this, ideally they
would be wrapped in a subsystem that makes them easier to be used (like
the driver model), but filesystems decided to use them and that usage
just grew over the years. That's evolution for you...
thanks,
greg k-h
On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
> BTW kill kobject_del() directly, because kobject_put() actually covers
> kobject removal automatically.
>
> Signed-off-by: Yangtao Li <[email protected]>
> ---
> fs/zonefs/sysfs.c | 11 +++++------
> fs/zonefs/zonefs.h | 1 -
> 2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
> index 8ccb65c2b419..f0783bf7a25c 100644
> --- a/fs/zonefs/sysfs.c
> +++ b/fs/zonefs/sysfs.c
> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
> return ret;
> }
>
> - sbi->s_sysfs_registered = true;
You know this, why do you need to have a variable tell you this or not?
> -
> return 0;
> }
>
> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
> {
> struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>
> - if (!sbi || !sbi->s_sysfs_registered)
How can either of these ever be true? Note, sbi should be passed here
to this function, not the super block as that is now unregistered from
the system. Looks like no one has really tested this codepath that much
:(
> + if (!sbi)
> return;
this can not ever be true, right?
>
> - kobject_del(&sbi->s_kobj);
> - kobject_put(&sbi->s_kobj);
> - wait_for_completion(&sbi->s_kobj_unregister);
> + if (kobject_is_added(&sbi->s_kobj)) {
Again, not needed.
thanks,
greg k-h
On 4/6/23 19:05, Greg KH wrote:
> On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
>> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
>> BTW kill kobject_del() directly, because kobject_put() actually covers
>> kobject removal automatically.
>>
>> Signed-off-by: Yangtao Li <[email protected]>
>> ---
>> fs/zonefs/sysfs.c | 11 +++++------
>> fs/zonefs/zonefs.h | 1 -
>> 2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
>> index 8ccb65c2b419..f0783bf7a25c 100644
>> --- a/fs/zonefs/sysfs.c
>> +++ b/fs/zonefs/sysfs.c
>> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
>> return ret;
>> }
>>
>> - sbi->s_sysfs_registered = true;
>
> You know this, why do you need to have a variable tell you this or not?
If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
fill_super will also return that error. vfs will then call kill_super, which
calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
added the kobj.
>
>> -
>> return 0;
>> }
>>
>> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
>> {
>> struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>>
>> - if (!sbi || !sbi->s_sysfs_registered)
>
> How can either of these ever be true? Note, sbi should be passed here
> to this function, not the super block as that is now unregistered from
> the system. Looks like no one has really tested this codepath that much
> :(
>
>> + if (!sbi)
>> return;
>
> this can not ever be true, right?
Yes it can, if someone attempt to mount a non zoned device. In that case,
fill_super returns early without setting sb->s_fs_info but vfs still calls
kill_super.
>
>
>>
>> - kobject_del(&sbi->s_kobj);
>> - kobject_put(&sbi->s_kobj);
>> - wait_for_completion(&sbi->s_kobj_unregister);
>> + if (kobject_is_added(&sbi->s_kobj)) {
>
> Again, not needed.
>
> thanks,
>
> greg k-h
Hi Greg,
On 2023/4/6 18:03, Greg KH wrote:
> On Thu, Apr 06, 2023 at 05:30:55PM +0800, Yangtao Li wrote:
>> Use kobject_is_added() instead of directly accessing the internal
>> variables of kobject. BTW kill kobject_del() directly, because
>> kobject_put() actually covers kobject removal automatically.
>>
>> Signed-off-by: Yangtao Li <[email protected]>
>> ---
>> fs/erofs/sysfs.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
>> index 435e515c0792..daac23e32026 100644
>> --- a/fs/erofs/sysfs.c
>> +++ b/fs/erofs/sysfs.c
>> @@ -240,8 +240,7 @@ void erofs_unregister_sysfs(struct super_block *sb)
>> {
>> struct erofs_sb_info *sbi = EROFS_SB(sb);
>>
>> - if (sbi->s_kobj.state_in_sysfs) {
>> - kobject_del(&sbi->s_kobj);
>> + if (kobject_is_added(&sbi->s_kobj)) {
>
> I do not understand why this check is even needed, I do not think it
> should be there at all as obviously the kobject was registered if it now
> needs to not be registered.
I think Yangtao sent a new patchset which missed the whole previous
background discussions as below:
https://lore.kernel.org/r/[email protected]
It's needed because once a syzbot complaint as below:
https://lore.kernel.org/r/CAD-N9QXNx=p3-QoWzk6pCznF32CZy8kM3vvo8mamfZZ9CpUKdw@mail.gmail.com
I'd suggest including the previous backgrounds at least in the newer patchset,
otherwise it makes me explain again and again...
Thanks,
Gao Xiang
>
> Meta-comment, we need to come up with a "filesystem kobject type" to get
> rid of lots of the boilerplate filesystem kobject logic as it's
> duplicated in every filesystem in tiny different ways and lots of times
> (like here), it's wrong.
>
> kobjects were not designed to be "used raw" like this, ideally they
> would be wrapped in a subsystem that makes them easier to be used (like
> the driver model), but filesystems decided to use them and that usage
> just grew over the years. That's evolution for you...>
> thanks,
>
> greg k-h
On Thu, Apr 06, 2023 at 07:13:38PM +0900, Damien Le Moal wrote:
> On 4/6/23 19:05, Greg KH wrote:
> > On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
> >> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
> >> BTW kill kobject_del() directly, because kobject_put() actually covers
> >> kobject removal automatically.
> >>
> >> Signed-off-by: Yangtao Li <[email protected]>
> >> ---
> >> fs/zonefs/sysfs.c | 11 +++++------
> >> fs/zonefs/zonefs.h | 1 -
> >> 2 files changed, 5 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
> >> index 8ccb65c2b419..f0783bf7a25c 100644
> >> --- a/fs/zonefs/sysfs.c
> >> +++ b/fs/zonefs/sysfs.c
> >> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
> >> return ret;
> >> }
> >>
> >> - sbi->s_sysfs_registered = true;
> >
> > You know this, why do you need to have a variable tell you this or not?
>
> If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
> fill_super will also return that error. vfs will then call kill_super, which
> calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
> added the kobj.
Ok, but then why not just 0 out the kobject pointer here instead? That
way you will always know if it's a valid pointer or not and you don't
have to rely on some other variable? Use the one that you have already :)
And you really don't even need to check anything, just pass in NULL to
kobject_del() and friends, it should handle it.
> >> -
> >> return 0;
> >> }
> >>
> >> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
> >> {
> >> struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
> >>
> >> - if (!sbi || !sbi->s_sysfs_registered)
> >
> > How can either of these ever be true? Note, sbi should be passed here
> > to this function, not the super block as that is now unregistered from
> > the system. Looks like no one has really tested this codepath that much
> > :(
> >
> >> + if (!sbi)
> >> return;
> >
> > this can not ever be true, right?
>
> Yes it can, if someone attempt to mount a non zoned device. In that case,
> fill_super returns early without setting sb->s_fs_info but vfs still calls
> kill_super.
But you already had a sbi pointer in the place that this was called, so
you "know" if you need to even call into here or not. You are having to
look up the same pointer multiple times in this call chain, there's no
need for that.
thanks,
greg k-h
On Thu, Apr 06, 2023 at 06:13:05PM +0800, Gao Xiang wrote:
> Hi Greg,
>
> On 2023/4/6 18:03, Greg KH wrote:
> > On Thu, Apr 06, 2023 at 05:30:55PM +0800, Yangtao Li wrote:
> > > Use kobject_is_added() instead of directly accessing the internal
> > > variables of kobject. BTW kill kobject_del() directly, because
> > > kobject_put() actually covers kobject removal automatically.
> > >
> > > Signed-off-by: Yangtao Li <[email protected]>
> > > ---
> > > fs/erofs/sysfs.c | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
> > > index 435e515c0792..daac23e32026 100644
> > > --- a/fs/erofs/sysfs.c
> > > +++ b/fs/erofs/sysfs.c
> > > @@ -240,8 +240,7 @@ void erofs_unregister_sysfs(struct super_block *sb)
> > > {
> > > struct erofs_sb_info *sbi = EROFS_SB(sb);
> > > - if (sbi->s_kobj.state_in_sysfs) {
> > > - kobject_del(&sbi->s_kobj);
> > > + if (kobject_is_added(&sbi->s_kobj)) {
> >
> > I do not understand why this check is even needed, I do not think it
> > should be there at all as obviously the kobject was registered if it now
> > needs to not be registered.
>
> I think Yangtao sent a new patchset which missed the whole previous
> background discussions as below:
> https://lore.kernel.org/r/[email protected]
>
> It's needed because once a syzbot complaint as below:
> https://lore.kernel.org/r/CAD-N9QXNx=p3-QoWzk6pCznF32CZy8kM3vvo8mamfZZ9CpUKdw@mail.gmail.com
>
> I'd suggest including the previous backgrounds at least in the newer patchset,
> otherwise it makes me explain again and again...
That would be good, as I do not think this is correct, it should be
fixed in a different way, see my response to the zonefs patch in this
series as a much simpler method to use.
thanks,
greg k-h
On 4/6/23 19:26, Greg KH wrote:
> On Thu, Apr 06, 2023 at 07:13:38PM +0900, Damien Le Moal wrote:
>> On 4/6/23 19:05, Greg KH wrote:
>>> On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
>>>> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
>>>> BTW kill kobject_del() directly, because kobject_put() actually covers
>>>> kobject removal automatically.
>>>>
>>>> Signed-off-by: Yangtao Li <[email protected]>
>>>> ---
>>>> fs/zonefs/sysfs.c | 11 +++++------
>>>> fs/zonefs/zonefs.h | 1 -
>>>> 2 files changed, 5 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
>>>> index 8ccb65c2b419..f0783bf7a25c 100644
>>>> --- a/fs/zonefs/sysfs.c
>>>> +++ b/fs/zonefs/sysfs.c
>>>> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
>>>> return ret;
>>>> }
>>>>
>>>> - sbi->s_sysfs_registered = true;
>>>
>>> You know this, why do you need to have a variable tell you this or not?
>>
>> If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
>> fill_super will also return that error. vfs will then call kill_super, which
>> calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
>> added the kobj.
>
> Ok, but then why not just 0 out the kobject pointer here instead? That
> way you will always know if it's a valid pointer or not and you don't
> have to rely on some other variable? Use the one that you have already :)
but sbi->s_kobj is the kobject itself, not a pointer. I can still zero it out in
case of error to avoid using the added s_sysfs_registered bool. I would need to
check a field of s_kobj though, which is not super clean and makes the code
dependent on kobject internals. Not super nice in my opinion, unless I am
missing something.
> And you really don't even need to check anything, just pass in NULL to
> kobject_del() and friends, it should handle it.>
>>>> -
>>>> return 0;
>>>> }
>>>>
>>>> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
>>>> {
>>>> struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>>>>
>>>> - if (!sbi || !sbi->s_sysfs_registered)
>>>
>>> How can either of these ever be true? Note, sbi should be passed here
>>> to this function, not the super block as that is now unregistered from
>>> the system. Looks like no one has really tested this codepath that much
>>> :(
>>>
>>>> + if (!sbi)
>>>> return;
>>>
>>> this can not ever be true, right?
>>
>> Yes it can, if someone attempt to mount a non zoned device. In that case,
>> fill_super returns early without setting sb->s_fs_info but vfs still calls
>> kill_super.
>
> But you already had a sbi pointer in the place that this was called, so
> you "know" if you need to even call into here or not. You are having to
> look up the same pointer multiple times in this call chain, there's no
> need for that.
I am not following here. Either we check that we have sbi here in
zonefs_sysfs_unregister(), or we conditionally call this function in
zonefs_kill_super() with a "if (sbi)". Either way, we need to check since sbi
can be NULL.
>
> thanks,
>
> greg k-h
On 2023/4/6 18:27, Greg KH wrote:
> On Thu, Apr 06, 2023 at 06:13:05PM +0800, Gao Xiang wrote:
>> Hi Greg,
>>
>> On 2023/4/6 18:03, Greg KH wrote:
>>> On Thu, Apr 06, 2023 at 05:30:55PM +0800, Yangtao Li wrote:
>>>> Use kobject_is_added() instead of directly accessing the internal
>>>> variables of kobject. BTW kill kobject_del() directly, because
>>>> kobject_put() actually covers kobject removal automatically.
>>>>
>>>> Signed-off-by: Yangtao Li <[email protected]>
>>>> ---
>>>> fs/erofs/sysfs.c | 3 +--
>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
>>>> index 435e515c0792..daac23e32026 100644
>>>> --- a/fs/erofs/sysfs.c
>>>> +++ b/fs/erofs/sysfs.c
>>>> @@ -240,8 +240,7 @@ void erofs_unregister_sysfs(struct super_block *sb)
>>>> {
>>>> struct erofs_sb_info *sbi = EROFS_SB(sb);
>>>> - if (sbi->s_kobj.state_in_sysfs) {
>>>> - kobject_del(&sbi->s_kobj);
>>>> + if (kobject_is_added(&sbi->s_kobj)) {
>>>
>>> I do not understand why this check is even needed, I do not think it
>>> should be there at all as obviously the kobject was registered if it now
>>> needs to not be registered.
>>
>> I think Yangtao sent a new patchset which missed the whole previous
>> background discussions as below:
>> https://lore.kernel.org/r/[email protected]
>>
>> It's needed because once a syzbot complaint as below:
>> https://lore.kernel.org/r/CAD-N9QXNx=p3-QoWzk6pCznF32CZy8kM3vvo8mamfZZ9CpUKdw@mail.gmail.com
>>
>> I'd suggest including the previous backgrounds at least in the newer patchset,
>> otherwise it makes me explain again and again...
>
> That would be good, as I do not think this is correct, it should be
> fixed in a different way, see my response to the zonefs patch in this
> series as a much simpler method to use.
Yes, but here (sbi->s_kobj) is not a kobject pointer (also at a quick
glance it seems that zonefs has similar code), and also we couldn't
just check the sbi is NULL or not here only, since sbi is already
non-NULL in this path and there are some others in sbi to free in
other functions.
s_kobj could be changed into a pointer if needed. I'm all fine with
either way since as you said, it's a boilerplate filesystem kobject
logic duplicated from somewhere. Hopefully Yangtao could help take
this task since he sent me patches about this multiple times.
Thanks,
Gao Xiang
>
> thanks,
>
> greg k-h
On Thu, Apr 06, 2023 at 07:58:38PM +0900, Damien Le Moal wrote:
> On 4/6/23 19:26, Greg KH wrote:
> > On Thu, Apr 06, 2023 at 07:13:38PM +0900, Damien Le Moal wrote:
> >> On 4/6/23 19:05, Greg KH wrote:
> >>> On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
> >>>> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
> >>>> BTW kill kobject_del() directly, because kobject_put() actually covers
> >>>> kobject removal automatically.
> >>>>
> >>>> Signed-off-by: Yangtao Li <[email protected]>
> >>>> ---
> >>>> fs/zonefs/sysfs.c | 11 +++++------
> >>>> fs/zonefs/zonefs.h | 1 -
> >>>> 2 files changed, 5 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
> >>>> index 8ccb65c2b419..f0783bf7a25c 100644
> >>>> --- a/fs/zonefs/sysfs.c
> >>>> +++ b/fs/zonefs/sysfs.c
> >>>> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
> >>>> return ret;
> >>>> }
> >>>>
> >>>> - sbi->s_sysfs_registered = true;
> >>>
> >>> You know this, why do you need to have a variable tell you this or not?
> >>
> >> If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
> >> fill_super will also return that error. vfs will then call kill_super, which
> >> calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
> >> added the kobj.
> >
> > Ok, but then why not just 0 out the kobject pointer here instead? That
> > way you will always know if it's a valid pointer or not and you don't
> > have to rely on some other variable? Use the one that you have already :)
>
> but sbi->s_kobj is the kobject itself, not a pointer.
Then it should not be there if the kobject is not valid as it should
have been freed when the kobject_init_and_add() call failed, right?
> I can still zero it out in
> case of error to avoid using the added s_sysfs_registered bool. I would need to
> check a field of s_kobj though, which is not super clean and makes the code
> dependent on kobject internals. Not super nice in my opinion, unless I am
> missing something.
See above, if a kobject fails to be registered, just remove the whole
object as it's obviously "dead" now and you can not trust it.
> > And you really don't even need to check anything, just pass in NULL to
> > kobject_del() and friends, it should handle it.>
> >>>> -
> >>>> return 0;
> >>>> }
> >>>>
> >>>> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
> >>>> {
> >>>> struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
> >>>>
> >>>> - if (!sbi || !sbi->s_sysfs_registered)
> >>>
> >>> How can either of these ever be true? Note, sbi should be passed here
> >>> to this function, not the super block as that is now unregistered from
> >>> the system. Looks like no one has really tested this codepath that much
> >>> :(
> >>>
> >>>> + if (!sbi)
> >>>> return;
> >>>
> >>> this can not ever be true, right?
> >>
> >> Yes it can, if someone attempt to mount a non zoned device. In that case,
> >> fill_super returns early without setting sb->s_fs_info but vfs still calls
> >> kill_super.
> >
> > But you already had a sbi pointer in the place that this was called, so
> > you "know" if you need to even call into here or not. You are having to
> > look up the same pointer multiple times in this call chain, there's no
> > need for that.
>
> I am not following here. Either we check that we have sbi here in
> zonefs_sysfs_unregister(), or we conditionally call this function in
> zonefs_kill_super() with a "if (sbi)". Either way, we need to check since sbi
> can be NULL.
In zonefs_kill_super() you have get the spi at the top of the function,
so use that, don't make zonefs_sysfs_unregister() have to compute it
again.
But again, if the kobject fails to be registered, you have to treat the
memory contained there as not valid and get rid of it as soon as
possible.
thanks,
greg k-h
On Thu, Apr 06, 2023 at 06:55:40PM +0800, Gao Xiang wrote:
>
>
> On 2023/4/6 18:27, Greg KH wrote:
> > On Thu, Apr 06, 2023 at 06:13:05PM +0800, Gao Xiang wrote:
> > > Hi Greg,
> > >
> > > On 2023/4/6 18:03, Greg KH wrote:
> > > > On Thu, Apr 06, 2023 at 05:30:55PM +0800, Yangtao Li wrote:
> > > > > Use kobject_is_added() instead of directly accessing the internal
> > > > > variables of kobject. BTW kill kobject_del() directly, because
> > > > > kobject_put() actually covers kobject removal automatically.
> > > > >
> > > > > Signed-off-by: Yangtao Li <[email protected]>
> > > > > ---
> > > > > fs/erofs/sysfs.c | 3 +--
> > > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
> > > > > index 435e515c0792..daac23e32026 100644
> > > > > --- a/fs/erofs/sysfs.c
> > > > > +++ b/fs/erofs/sysfs.c
> > > > > @@ -240,8 +240,7 @@ void erofs_unregister_sysfs(struct super_block *sb)
> > > > > {
> > > > > struct erofs_sb_info *sbi = EROFS_SB(sb);
> > > > > - if (sbi->s_kobj.state_in_sysfs) {
> > > > > - kobject_del(&sbi->s_kobj);
> > > > > + if (kobject_is_added(&sbi->s_kobj)) {
> > > >
> > > > I do not understand why this check is even needed, I do not think it
> > > > should be there at all as obviously the kobject was registered if it now
> > > > needs to not be registered.
> > >
> > > I think Yangtao sent a new patchset which missed the whole previous
> > > background discussions as below:
> > > https://lore.kernel.org/r/[email protected]
> > >
> > > It's needed because once a syzbot complaint as below:
> > > https://lore.kernel.org/r/CAD-N9QXNx=p3-QoWzk6pCznF32CZy8kM3vvo8mamfZZ9CpUKdw@mail.gmail.com
> > >
> > > I'd suggest including the previous backgrounds at least in the newer patchset,
> > > otherwise it makes me explain again and again...
> >
> > That would be good, as I do not think this is correct, it should be
> > fixed in a different way, see my response to the zonefs patch in this
> > series as a much simpler method to use.
>
> Yes, but here (sbi->s_kobj) is not a kobject pointer (also at a quick
> glance it seems that zonefs has similar code), and also we couldn't
> just check the sbi is NULL or not here only, since sbi is already
> non-NULL in this path and there are some others in sbi to free in
> other functions.
>
> s_kobj could be changed into a pointer if needed. I'm all fine with
> either way since as you said, it's a boilerplate filesystem kobject
> logic duplicated from somewhere. Hopefully Yangtao could help take
> this task since he sent me patches about this multiple times.
I made the same mistake with the zonefs code. If the kobject in this
structure controls the lifespan of it (which makes it not a pointer, my
mistake), then that whole memory chunk can't be valid anymore if the
kobject registering function failed so you need to get rid of it then,
not later.
thanks,
greg k-h
On 4/6/23 20:18, Greg KH wrote:
> On Thu, Apr 06, 2023 at 07:58:38PM +0900, Damien Le Moal wrote:
>> On 4/6/23 19:26, Greg KH wrote:
>>> On Thu, Apr 06, 2023 at 07:13:38PM +0900, Damien Le Moal wrote:
>>>> On 4/6/23 19:05, Greg KH wrote:
>>>>> On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
>>>>>> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
>>>>>> BTW kill kobject_del() directly, because kobject_put() actually covers
>>>>>> kobject removal automatically.
>>>>>>
>>>>>> Signed-off-by: Yangtao Li <[email protected]>
>>>>>> ---
>>>>>> fs/zonefs/sysfs.c | 11 +++++------
>>>>>> fs/zonefs/zonefs.h | 1 -
>>>>>> 2 files changed, 5 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
>>>>>> index 8ccb65c2b419..f0783bf7a25c 100644
>>>>>> --- a/fs/zonefs/sysfs.c
>>>>>> +++ b/fs/zonefs/sysfs.c
>>>>>> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> - sbi->s_sysfs_registered = true;
>>>>>
>>>>> You know this, why do you need to have a variable tell you this or not?
>>>>
>>>> If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
>>>> fill_super will also return that error. vfs will then call kill_super, which
>>>> calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
>>>> added the kobj.
>>>
>>> Ok, but then why not just 0 out the kobject pointer here instead? That
>>> way you will always know if it's a valid pointer or not and you don't
>>> have to rely on some other variable? Use the one that you have already :)
>>
>> but sbi->s_kobj is the kobject itself, not a pointer.
>
> Then it should not be there if the kobject is not valid as it should
> have been freed when the kobject_init_and_add() call failed, right?
What do you mean freed ? the kboject itself is a field of zonefs sbi. So the
kobject gets freed together with sbi.
>> I can still zero it out in
>> case of error to avoid using the added s_sysfs_registered bool. I would need to
>> check a field of s_kobj though, which is not super clean and makes the code
>> dependent on kobject internals. Not super nice in my opinion, unless I am
>> missing something.
>
> See above, if a kobject fails to be registered, just remove the whole
> object as it's obviously "dead" now and you can not trust it.
Well yes, that is what s_sysfs_registered indicates, that the kobject is not
valid. I do not understand what you mean with "just remove the whole object".
>>> And you really don't even need to check anything, just pass in NULL to
>>> kobject_del() and friends, it should handle it.>
>>>>>> -
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
>>>>>> {
>>>>>> struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>>>>>>
>>>>>> - if (!sbi || !sbi->s_sysfs_registered)
>>>>>
>>>>> How can either of these ever be true? Note, sbi should be passed here
>>>>> to this function, not the super block as that is now unregistered from
>>>>> the system. Looks like no one has really tested this codepath that much
>>>>> :(
>>>>>
>>>>>> + if (!sbi)
>>>>>> return;
>>>>>
>>>>> this can not ever be true, right?
>>>>
>>>> Yes it can, if someone attempt to mount a non zoned device. In that case,
>>>> fill_super returns early without setting sb->s_fs_info but vfs still calls
>>>> kill_super.
>>>
>>> But you already had a sbi pointer in the place that this was called, so
>>> you "know" if you need to even call into here or not. You are having to
>>> look up the same pointer multiple times in this call chain, there's no
>>> need for that.
>>
>> I am not following here. Either we check that we have sbi here in
>> zonefs_sysfs_unregister(), or we conditionally call this function in
>> zonefs_kill_super() with a "if (sbi)". Either way, we need to check since sbi
>> can be NULL.
>
> In zonefs_kill_super() you have get the spi at the top of the function,
> so use that, don't make zonefs_sysfs_unregister() have to compute it
> again.
That I can do, yes.
>
> But again, if the kobject fails to be registered, you have to treat the
> memory contained there as not valid and get rid of it as soon as
> possible.
If the kobject add failed, we never touch it thanks to s_sysfs_registered. I
still do not see the issue here.
>
> thanks,
>
> greg k-h
> Meta-comment, we need to come up with a "filesystem kobject type" to get
> rid of lots of the boilerplate filesystem kobject logic as it's
> duplicated in every filesystem in tiny different ways and lots of times
> (like here), it's wrong.
Can we add the following structure?
struct filesystem_kobject {
struct kobject kobject;
struct completion unregister;
};
w/ it, we can simplify something:
1. merge init_completion and kobject_init_and_add
2. merge kobject_put and wait_for_completion
3. we can have a common release func for kobj_type release
MBR,
Yangtao
> > Meta-comment, we need to come up with a "filesystem kobject type" to get
> > rid of lots of the boilerplate filesystem kobject logic as it's
> > duplicated in every filesystem in tiny different ways and lots of times
> > (like here), it's wrong.
>
> Can we add the following structure?
>
> struct filesystem_kobject {
> struct kobject kobject;
> struct completion unregister;
> };
>
> w/ it, we can simplify something:
>
> 1. merge init_completion and kobject_init_and_add
>
> 2. merge kobject_put and wait_for_completion
>
> 3. we can have a common release func for kobj_type release
It seems that the above ideas are not crazy enough, maybe we should do more.
Any ideas and suggestions are very welcome.
MBR,
Yangtao
On Thu, Apr 06, 2023 at 08:07:16PM +0800, Yangtao Li wrote:
> > Meta-comment, we need to come up with a "filesystem kobject type" to get
> > rid of lots of the boilerplate filesystem kobject logic as it's
> > duplicated in every filesystem in tiny different ways and lots of times
> > (like here), it's wrong.
>
> Can we add the following structure?
>
> struct filesystem_kobject {
> struct kobject kobject;
> struct completion unregister;
> };
Ah, no, I see the problem.
The filesystem authors are treating the kobject NOT as the thing that
handles the lifespan of the structure it is embedded in, but rather as
something else (i.e. a convient place to put filesystem information to
userspace.)
That isn't going to work, and as proof of that, the release callback
should be a simple call to kfree(), NOT as a completion notification
which then something else will go off and free the memory here. That
implies that there are multiple reference counting structures happening
on the same structure, which is not ok.
Either we let the kobject code properly handle the lifespan of the
structure, OR we pull it out of the structure and just let it hang off
as a separate structure (i.e. a pointer to something else.)
As the superblock lifespan rules ALREADY control the reference counting
logic of the filesystem superblock structure, let's stick with that and
just tack-on the kobject as a separate structure entirely.
Does that make sense? Let me do a quick pass at this for zonefs as
that's pretty simple to show you what I mean...
thanks,
greg k-h
Hi Greg,
> That isn't going to work, and as proof of that, the release callback
> should be a simple call to kfree(), NOT as a completion notification
> which then something else will go off and free the memory here. That
> implies that there are multiple reference counting structures happening
> on the same structure, which is not ok.
The release() function did nothing inside, but we need to wait asynchronously...
Can we directly export the kobject_cleanup(kobj) interface so that
kobj_type->release() doesn't have to do anything?
If do it, the use of init_completion, wait_for_completion, etc. will no longer be needed.
> OR we pull it out of the structure and just let it hang off as a separate
> structure (i.e. a pointer to something else.)
Make something like sbi->s_kobj a pointer instead of data embedded in sbi?
When kobject_init_and_add fails, call kobject_put(sbi->s_kobj), and assign
sbi->s_kobj = NULL at the same time?
Thx,
Yangtao
Hi Greg,
> just let it hang off as a separate structure (i.e. a pointer to something else.)
I have made some attempts. According to my understanding, the reason why the
filesystem needs to embed the kobj structure (not a pointer) is that the kobj_to_sbi
method is required in the attr_store/attr_show method for subsequent data processing.
130 static ssize_t erofs_attr_store(struct kobject *kobj, struct attribute *attr,
131 const char *buf, size_t len)
132 {
133 struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
134 s_kobj);
If we turn the kobject in sbi into a pointer, then we need to insert a pointer
to sbi in the kobject, or perform the following encapsulation.
struct filesystem_kobject {
struct kobject kobject;
void *private;
};
Later, I thought I could send some demo code that strips the kobject in sbi into a pointer.
BTW, Now sysfs.c in many file systems is full of a lot of repetitive code, maybe we can abstract the common part?
Like filesystem_attr、filesystem_kobject_ops、filesystem_kobject_ktype...
Thx,
Yangtao
Hi all,
> Later, I thought I could send some demo code that strips the kobject in sbi into a pointer.
I made the following modifications, not sure if I'm going the right way.
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 1db018f8c2e8..8e1799f690c0 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -165,8 +165,7 @@ struct erofs_sb_info {
u32 feature_incompat;
/* sysfs support */
- struct kobject s_kobj; /* /sys/fs/erofs/<devname> */
- struct completion s_kobj_unregister;
+ struct filesystem_kobject *f_kobj;
/* fscache support */
struct fscache_volume *volume;
diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
index 435e515c0792..70e915906012 100644
--- a/fs/erofs/sysfs.c
+++ b/fs/erofs/sysfs.c
@@ -8,6 +8,33 @@
#include "internal.h"
+//maybe we should add following thins to include/linux/filesystem_kobject.h ?
+struct filesystem_kobject {
+ struct kobject kobject;
+ void *private;
+};
+
+void filesystem_kobject_put(struct filesystem_kobject *f_kobj)
+{
+ if (f_kobj)
+ kobject_put(&f_kobj->kobject);
+}
+
+void filesystem_kobject_set_private(struct filesystem_kobject *f_kobj, void *p)
+{
+ f_kobj->private = p;
+}
+
+void *filesystem_kobject_get_private(struct filesystem_kobject *f_kobj)
+{
+ return f_kobj->private;
+}
+
+struct kobject *filesystem_kobject_get_kobject(struct filesystem_kobject *f_kobj)
+{
+ return &f_kobj->kobject;
+}
+
enum {
attr_feature,
attr_pointer_ui,
@@ -107,8 +134,9 @@ static unsigned char *__struct_ptr(struct erofs_sb_info *sbi,
static ssize_t erofs_attr_show(struct kobject *kobj,
struct attribute *attr, char *buf)
{
- struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
- s_kobj);
+ struct filesystem_kobject *f_kobject = container_of(kobj, struct filesystem_kobject,
+ kobject);
+ struct erofs_sb_info *sbi = filesystem_kobject_get_private(f_kobject);
struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
@@ -130,8 +158,9 @@ static ssize_t erofs_attr_show(struct kobject *kobj,
static ssize_t erofs_attr_store(struct kobject *kobj, struct attribute *attr,
const char *buf, size_t len)
{
- struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
- s_kobj);
+ struct filesystem_kobject *f_kobject = container_of(kobj, struct filesystem_kobject,
+ kobject);
+ struct erofs_sb_info *sbi = filesystem_kobject_get_private(f_kobject);
struct erofs_attr *a = container_of(attr, struct erofs_attr, attr);
unsigned char *ptr = __struct_ptr(sbi, a->struct_type, a->offset);
unsigned long t;
@@ -169,9 +198,12 @@ static ssize_t erofs_attr_store(struct kobject *kobj, struct attribute *attr,
static void erofs_sb_release(struct kobject *kobj)
{
- struct erofs_sb_info *sbi = container_of(kobj, struct erofs_sb_info,
- s_kobj);
- complete(&sbi->s_kobj_unregister);
+ struct filesystem_kobject *f_kobject = container_of(kobj, struct filesystem_kobject,
+ kobject);
+ struct erofs_sb_info *sbi = filesystem_kobject_get_private(f_kobject);
+
+ kfree(f_kobject);
+ sbi->f_kobj = NULL;
}
static const struct sysfs_ops erofs_attr_ops = {
@@ -205,6 +237,7 @@ static struct kobject erofs_feat = {
int erofs_register_sysfs(struct super_block *sb)
{
struct erofs_sb_info *sbi = EROFS_SB(sb);
+ struct kobject *kobj;
char *name;
char *str = NULL;
int err;
@@ -222,17 +255,24 @@ int erofs_register_sysfs(struct super_block *sb)
} else {
name = sb->s_id;
}
- sbi->s_kobj.kset = &erofs_root;
- init_completion(&sbi->s_kobj_unregister);
- err = kobject_init_and_add(&sbi->s_kobj, &erofs_sb_ktype, NULL, "%s", name);
+
+ sbi->f_kobj = kzalloc(sizeof(struct filesystem_kobject), GFP_KERNEL);
+ if (!sbi->f_kobj) {
+ kfree(str);
+ return -ENOMEM;
+ }
+ filesystem_kobject_set_private(sbi->f_kobj, sbi);
+ kobj = filesystem_kobject_get_kobject(sbi->f_kobj);
+ kobj->kset = &erofs_root;
+
+ err = kobject_init_and_add(&sbi->f_kobj->kobject, &erofs_sb_ktype, NULL, "%s", name);
kfree(str);
if (err)
goto put_sb_kobj;
return 0;
put_sb_kobj:
- kobject_put(&sbi->s_kobj);
- wait_for_completion(&sbi->s_kobj_unregister);
+ filesystem_kobject_put(sbi->f_kobj);
return err;
}
@@ -240,11 +280,7 @@ void erofs_unregister_sysfs(struct super_block *sb)
{
struct erofs_sb_info *sbi = EROFS_SB(sb);
- if (sbi->s_kobj.state_in_sysfs) {
- kobject_del(&sbi->s_kobj);
- kobject_put(&sbi->s_kobj);
- wait_for_completion(&sbi->s_kobj_unregister);
- }
+ filesystem_kobject_put(sbi->f_kobj);
}
int __init erofs_init_sysfs(void)