drivers core: lookup sysfs power group before removal

[PATCH] drivers core: lookup sysfs power group before removal

Posted by José Pekkarinen 2 years, 1 month ago

Hinted by syzboot, there is a few cases where the sysfs power group may
not be there, like the failure while adding it, or adding its runtime
group, or when the sysfs firmware loader fallback fail to populate. In
the last case, the device_del function will be called leading to attempt
to remove the sysfs group. This patch will lookup for it in advance to
grant that it is effectively there before cleaning it up.

Reported-by: syzbot+95f2e2439b97575ec3c0@syzkaller.appspotmail.com

Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
---
 drivers/base/power/sysfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index a1474fb67db9..6601729c4698 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -834,5 +834,7 @@ void dpm_sysfs_remove(struct device *dev)
 	dev_pm_qos_constraints_destroy(dev);
 	rpm_sysfs_remove(dev);
 	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
-	sysfs_remove_group(&dev->kobj, &pm_attr_group);
+
+	if (kernfs_find_and_get((&dev->kobj)->sd, pm_attr_group.name))
+		sysfs_remove_group(&dev->kobj, &pm_attr_group);
 }
-- 
2.39.2

Re: [PATCH] drivers core: lookup sysfs power group before removal

Posted by Greg KH 2 years, 1 month ago

On Wed, Nov 01, 2023 at 07:36:27PM +0200, José Pekkarinen wrote:
> Hinted by syzboot, there is a few cases where the sysfs power group may
> not be there, like the failure while adding it, or adding its runtime
> group, or when the sysfs firmware loader fallback fail to populate. In
> the last case, the device_del function will be called leading to attempt
> to remove the sysfs group. This patch will lookup for it in advance to
> grant that it is effectively there before cleaning it up.
> 
> Reported-by: syzbot+95f2e2439b97575ec3c0@syzkaller.appspotmail.com
> 
> Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
> ---
>  drivers/base/power/sysfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index a1474fb67db9..6601729c4698 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -834,5 +834,7 @@ void dpm_sysfs_remove(struct device *dev)
>  	dev_pm_qos_constraints_destroy(dev);
>  	rpm_sysfs_remove(dev);
>  	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> -	sysfs_remove_group(&dev->kobj, &pm_attr_group);
> +
> +	if (kernfs_find_and_get((&dev->kobj)->sd, pm_attr_group.name))
> +		sysfs_remove_group(&dev->kobj, &pm_attr_group);

What's to keep it from going away right after finding it?

In other words, what is wrong with removing a group that is not there?
What error happens?  It should be fine, or are you seeing real code
failures somewhere?

Also, I think you just leaked a reference count here, how was this
tested?

thanks,

greg k-h

Re: [PATCH] drivers core: lookup sysfs power group before removal

Posted by José Pekkarinen 2 years, 1 month ago

On 2023-11-01 19:54, Greg KH wrote:
> On Wed, Nov 01, 2023 at 07:36:27PM +0200, José Pekkarinen wrote:
>> Hinted by syzboot, there is a few cases where the sysfs power group 
>> may
>> not be there, like the failure while adding it, or adding its runtime
>> group, or when the sysfs firmware loader fallback fail to populate. In
>> the last case, the device_del function will be called leading to 
>> attempt
>> to remove the sysfs group. This patch will lookup for it in advance to
>> grant that it is effectively there before cleaning it up.
>> 
>> Reported-by: syzbot+95f2e2439b97575ec3c0@syzkaller.appspotmail.com
>> 
>> Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
>> ---
>>  drivers/base/power/sysfs.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
>> index a1474fb67db9..6601729c4698 100644
>> --- a/drivers/base/power/sysfs.c
>> +++ b/drivers/base/power/sysfs.c
>> @@ -834,5 +834,7 @@ void dpm_sysfs_remove(struct device *dev)
>>  	dev_pm_qos_constraints_destroy(dev);
>>  	rpm_sysfs_remove(dev);
>>  	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
>> -	sysfs_remove_group(&dev->kobj, &pm_attr_group);
>> +
>> +	if (kernfs_find_and_get((&dev->kobj)->sd, pm_attr_group.name))
>> +		sysfs_remove_group(&dev->kobj, &pm_attr_group);
> 
> What's to keep it from going away right after finding it?
> 
> In other words, what is wrong with removing a group that is not there?
> What error happens?  It should be fine, or are you seeing real code
> failures somewhere?

     No, this is just hitting a warning that sysbot complains about by
setting panic on warning, no big deal, though it can be a wrong 
behaviour
in ueagle-atm driver, since it defines to disconnect the device if the
firmware is not there, no matter the sysfs fallback.

> Also, I think you just leaked a reference count here, how was this
> tested?

     Both by setting up a local vm following syzkaller instructions and
using the syzkaller itself to test it. You can take a look in the link
if you feel like it.

     José.

Re: [PATCH] drivers core: lookup sysfs power group before removal

Posted by Greg KH 2 years, 1 month ago

On Fri, Nov 03, 2023 at 07:49:39PM +0200, José Pekkarinen wrote:
> On 2023-11-01 19:54, Greg KH wrote:
> > On Wed, Nov 01, 2023 at 07:36:27PM +0200, José Pekkarinen wrote:
> > > Hinted by syzboot, there is a few cases where the sysfs power group
> > > may
> > > not be there, like the failure while adding it, or adding its runtime
> > > group, or when the sysfs firmware loader fallback fail to populate. In
> > > the last case, the device_del function will be called leading to
> > > attempt
> > > to remove the sysfs group. This patch will lookup for it in advance to
> > > grant that it is effectively there before cleaning it up.
> > > 
> > > Reported-by: syzbot+95f2e2439b97575ec3c0@syzkaller.appspotmail.com
> > > 
> > > Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
> > > ---
> > >  drivers/base/power/sysfs.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > > index a1474fb67db9..6601729c4698 100644
> > > --- a/drivers/base/power/sysfs.c
> > > +++ b/drivers/base/power/sysfs.c
> > > @@ -834,5 +834,7 @@ void dpm_sysfs_remove(struct device *dev)
> > >  	dev_pm_qos_constraints_destroy(dev);
> > >  	rpm_sysfs_remove(dev);
> > >  	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> > > -	sysfs_remove_group(&dev->kobj, &pm_attr_group);
> > > +
> > > +	if (kernfs_find_and_get((&dev->kobj)->sd, pm_attr_group.name))
> > > +		sysfs_remove_group(&dev->kobj, &pm_attr_group);
> > 
> > What's to keep it from going away right after finding it?
> > 
> > In other words, what is wrong with removing a group that is not there?
> > What error happens?  It should be fine, or are you seeing real code
> > failures somewhere?
> 
>     No, this is just hitting a warning that sysbot complains about by
> setting panic on warning, no big deal, though it can be a wrong behaviour
> in ueagle-atm driver, since it defines to disconnect the device if the
> firmware is not there, no matter the sysfs fallback.

Then fix the driver please.

And what warning are you seeing exactly?

thanks,

greg k-h

Re: [PATCH] drivers core: lookup sysfs power group before removal

Posted by José Pekkarinen 2 years, 1 month ago

On 2023-11-03 20:36, Greg KH wrote:
> On Fri, Nov 03, 2023 at 07:49:39PM +0200, José Pekkarinen wrote:
>> On 2023-11-01 19:54, Greg KH wrote:
>> > On Wed, Nov 01, 2023 at 07:36:27PM +0200, José Pekkarinen wrote:
>> > > Hinted by syzboot, there is a few cases where the sysfs power group
>> > > may
>> > > not be there, like the failure while adding it, or adding its runtime
>> > > group, or when the sysfs firmware loader fallback fail to populate. In
>> > > the last case, the device_del function will be called leading to
>> > > attempt
>> > > to remove the sysfs group. This patch will lookup for it in advance to
>> > > grant that it is effectively there before cleaning it up.
>> > >
>> > > Reported-by: syzbot+95f2e2439b97575ec3c0@syzkaller.appspotmail.com
>> > >
>> > > Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
>> > > ---
>> > >  drivers/base/power/sysfs.c | 4 +++-
>> > >  1 file changed, 3 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
>> > > index a1474fb67db9..6601729c4698 100644
>> > > --- a/drivers/base/power/sysfs.c
>> > > +++ b/drivers/base/power/sysfs.c
>> > > @@ -834,5 +834,7 @@ void dpm_sysfs_remove(struct device *dev)
>> > >  	dev_pm_qos_constraints_destroy(dev);
>> > >  	rpm_sysfs_remove(dev);
>> > >  	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
>> > > -	sysfs_remove_group(&dev->kobj, &pm_attr_group);
>> > > +
>> > > +	if (kernfs_find_and_get((&dev->kobj)->sd, pm_attr_group.name))
>> > > +		sysfs_remove_group(&dev->kobj, &pm_attr_group);
>> >
>> > What's to keep it from going away right after finding it?
>> >
>> > In other words, what is wrong with removing a group that is not there?
>> > What error happens?  It should be fine, or are you seeing real code
>> > failures somewhere?
>> 
>>     No, this is just hitting a warning that sysbot complains about by
>> setting panic on warning, no big deal, though it can be a wrong 
>> behaviour
>> in ueagle-atm driver, since it defines to disconnect the device if the
>> firmware is not there, no matter the sysfs fallback.
> 
> Then fix the driver please.

     I'm afraid I was wrong in the assumption that the probe return value
of the driver would influence the testing result, so this no longer 
seems
fixable from driver side. It may be fixed here in the base, in the sysfs
or not to fix it at all since this seem to be a very difficult to reach
situation. I'm fine with any approach, and I can do the work, I'd just
like to read what are your preferences on the topic.

     Thanks!

     José.

Re: [PATCH] drivers core: lookup sysfs power group before removal

Posted by Greg KH 2 years, 1 month ago

On Tue, Nov 07, 2023 at 10:56:20AM +0200, José Pekkarinen wrote:
> On 2023-11-03 20:36, Greg KH wrote:
> > On Fri, Nov 03, 2023 at 07:49:39PM +0200, José Pekkarinen wrote:
> > > On 2023-11-01 19:54, Greg KH wrote:
> > > > On Wed, Nov 01, 2023 at 07:36:27PM +0200, José Pekkarinen wrote:
> > > > > Hinted by syzboot, there is a few cases where the sysfs power group
> > > > > may
> > > > > not be there, like the failure while adding it, or adding its runtime
> > > > > group, or when the sysfs firmware loader fallback fail to populate. In
> > > > > the last case, the device_del function will be called leading to
> > > > > attempt
> > > > > to remove the sysfs group. This patch will lookup for it in advance to
> > > > > grant that it is effectively there before cleaning it up.
> > > > >
> > > > > Reported-by: syzbot+95f2e2439b97575ec3c0@syzkaller.appspotmail.com
> > > > >
> > > > > Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
> > > > > ---
> > > > >  drivers/base/power/sysfs.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > > > > index a1474fb67db9..6601729c4698 100644
> > > > > --- a/drivers/base/power/sysfs.c
> > > > > +++ b/drivers/base/power/sysfs.c
> > > > > @@ -834,5 +834,7 @@ void dpm_sysfs_remove(struct device *dev)
> > > > >  	dev_pm_qos_constraints_destroy(dev);
> > > > >  	rpm_sysfs_remove(dev);
> > > > >  	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> > > > > -	sysfs_remove_group(&dev->kobj, &pm_attr_group);
> > > > > +
> > > > > +	if (kernfs_find_and_get((&dev->kobj)->sd, pm_attr_group.name))
> > > > > +		sysfs_remove_group(&dev->kobj, &pm_attr_group);
> > > >
> > > > What's to keep it from going away right after finding it?
> > > >
> > > > In other words, what is wrong with removing a group that is not there?
> > > > What error happens?  It should be fine, or are you seeing real code
> > > > failures somewhere?
> > > 
> > >     No, this is just hitting a warning that sysbot complains about by
> > > setting panic on warning, no big deal, though it can be a wrong
> > > behaviour
> > > in ueagle-atm driver, since it defines to disconnect the device if the
> > > firmware is not there, no matter the sysfs fallback.
> > 
> > Then fix the driver please.
> 
>     I'm afraid I was wrong in the assumption that the probe return value
> of the driver would influence the testing result, so this no longer seems
> fixable from driver side.

Why is it not fixable from the driver side?  It is the code that is
creating, and then removing, the files, not the driver core, or am I
missing something here?

> It may be fixed here in the base, in the sysfs or not to fix it at all
> since this seem to be a very difficult to reach situation. I'm fine
> with any approach, and I can do the work, I'd just like to read what
> are your preferences on the topic.

Please prove that the driver is not the offending one here first please,
otherwise, why isn't this an issue for all drivers?

thanks,

greg k-h

Re: [PATCH] drivers core: lookup sysfs power group before removal

Posted by José Pekkarinen 2 years, 1 month ago

On 2023-11-07 11:53, Greg KH wrote:
> On Tue, Nov 07, 2023 at 10:56:20AM +0200, José Pekkarinen wrote:
>> On 2023-11-03 20:36, Greg KH wrote:
>> > On Fri, Nov 03, 2023 at 07:49:39PM +0200, José Pekkarinen wrote:
>> > > On 2023-11-01 19:54, Greg KH wrote:
>> > > > On Wed, Nov 01, 2023 at 07:36:27PM +0200, José Pekkarinen wrote:
>> > > > > Hinted by syzboot, there is a few cases where the sysfs power group
>> > > > > may
>> > > > > not be there, like the failure while adding it, or adding its runtime
>> > > > > group, or when the sysfs firmware loader fallback fail to populate. In
>> > > > > the last case, the device_del function will be called leading to
>> > > > > attempt
>> > > > > to remove the sysfs group. This patch will lookup for it in advance to
>> > > > > grant that it is effectively there before cleaning it up.
>> > > > >
>> > > > > Reported-by: syzbot+95f2e2439b97575ec3c0@syzkaller.appspotmail.com
>> > > > >
>> > > > > Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
>> > > > > ---
>> > > > >  drivers/base/power/sysfs.c | 4 +++-
>> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
>> > > > >
>> > > > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
>> > > > > index a1474fb67db9..6601729c4698 100644
>> > > > > --- a/drivers/base/power/sysfs.c
>> > > > > +++ b/drivers/base/power/sysfs.c
>> > > > > @@ -834,5 +834,7 @@ void dpm_sysfs_remove(struct device *dev)
>> > > > >  	dev_pm_qos_constraints_destroy(dev);
>> > > > >  	rpm_sysfs_remove(dev);
>> > > > >  	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
>> > > > > -	sysfs_remove_group(&dev->kobj, &pm_attr_group);
>> > > > > +
>> > > > > +	if (kernfs_find_and_get((&dev->kobj)->sd, pm_attr_group.name))
>> > > > > +		sysfs_remove_group(&dev->kobj, &pm_attr_group);
>> > > >
>> > > > What's to keep it from going away right after finding it?
>> > > >
>> > > > In other words, what is wrong with removing a group that is not there?
>> > > > What error happens?  It should be fine, or are you seeing real code
>> > > > failures somewhere?
>> > >
>> > >     No, this is just hitting a warning that sysbot complains about by
>> > > setting panic on warning, no big deal, though it can be a wrong
>> > > behaviour
>> > > in ueagle-atm driver, since it defines to disconnect the device if the
>> > > firmware is not there, no matter the sysfs fallback.
>> >
>> > Then fix the driver please.
>> 
>>     I'm afraid I was wrong in the assumption that the probe return 
>> value
>> of the driver would influence the testing result, so this no longer 
>> seems
>> fixable from driver side.
> 
> Why is it not fixable from the driver side?  It is the code that is
> creating, and then removing, the files, not the driver core, or am I
> missing something here?

     I don't think this is much of a problem since the situation is 
unlikely to
happen. The reproducer is attempting to register a simulated ueagle 
modem and
checks if the device becomes ready. If it doesn't for any reason, in 
this case,
the lack of the firmware, it just disconnect it and try to populate it 
again.
After a few minutes banging the kernel this way it hits the situation 
where the
asynchronous firmware loader didn't yet populate the sysfs files before 
the
device is getting removed, and hit the warning of missing the power 
attributes
in sysfs, while removing the sysfs groups. If it is a problem, it is 
certainly
not a top priority problem to solve for sure.

     José.