[PATCH] misc: keba: Fix sysfs group creation

[PATCH] misc: keba: Fix sysfs group creation

[Gerhard Engleder]

From: Gerhard Engleder <eg@keba.com>

sysfs_create_group() races with userspace. Use dev_groups instead which
prevents all the problems of sysfs_create_group().

Fixes: a1944676767e ("misc: keba: Add basic KEBA CP500 system FPGA support")
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Gerhard Engleder <eg@keba.com>
---
 drivers/misc/keba/cp500.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/misc/keba/cp500.c b/drivers/misc/keba/cp500.c
index 9ba46f0f9392..ae0922817881 100644
--- a/drivers/misc/keba/cp500.c
+++ b/drivers/misc/keba/cp500.c
@@ -212,12 +212,12 @@ static ssize_t keep_cfg_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(keep_cfg);
 
-static struct attribute *attrs[] = {
+static struct attribute *cp500_attrs[] = {
 	&dev_attr_version.attr,
 	&dev_attr_keep_cfg.attr,
 	NULL
 };
-static const struct attribute_group attrs_group = { .attrs = attrs };
+ATTRIBUTE_GROUPS(cp500);
 
 static void cp500_i2c_release(struct device *dev)
 {
@@ -396,20 +396,15 @@ static int cp500_probe(struct pci_dev *pci_dev, const struct pci_device_id *id)
 
 	pci_set_drvdata(pci_dev, cp500);
 
-	ret = sysfs_create_group(&pci_dev->dev.kobj, &attrs_group);
-	if (ret != 0)
-		goto out_free_irq;
 
 	ret = cp500_enable(cp500);
 	if (ret != 0)
-		goto out_remove_group;
+		goto out_free_irq;
 
 	cp500_register_auxiliary_devs(cp500);
 
 	return 0;
 
-out_remove_group:
-	sysfs_remove_group(&pci_dev->dev.kobj, &attrs_group);
 out_free_irq:
 	pci_free_irq_vectors(pci_dev);
 out_disable:
@@ -427,8 +422,6 @@ static void cp500_remove(struct pci_dev *pci_dev)
 
 	cp500_disable(cp500);
 
-	sysfs_remove_group(&pci_dev->dev.kobj, &attrs_group);
-
 	pci_set_drvdata(pci_dev, 0);
 
 	pci_free_irq_vectors(pci_dev);
@@ -450,6 +443,7 @@ static struct pci_driver cp500_driver = {
 	.id_table = cp500_ids,
 	.probe = cp500_probe,
 	.remove = cp500_remove,
+	.dev_groups = cp500_groups,
 };
 module_pci_driver(cp500_driver);
 
-- 
2.39.2

Re: [PATCH] misc: keba: Fix sysfs group creation

[Gerhard Engleder]

On 19.08.24 21:26, Gerhard Engleder wrote:
> From: Gerhard Engleder <eg@keba.com>
> 
> sysfs_create_group() races with userspace. Use dev_groups instead which
> prevents all the problems of sysfs_create_group().
> 
> Fixes: a1944676767e ("misc: keba: Add basic KEBA CP500 system FPGA support")
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Gerhard Engleder <eg@keba.com>
> ---
>   drivers/misc/keba/cp500.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/misc/keba/cp500.c b/drivers/misc/keba/cp500.c
> index 9ba46f0f9392..ae0922817881 100644
> --- a/drivers/misc/keba/cp500.c
> +++ b/drivers/misc/keba/cp500.c
> @@ -212,12 +212,12 @@ static ssize_t keep_cfg_store(struct device *dev, struct device_attribute *attr,
>   }
>   static DEVICE_ATTR_RW(keep_cfg);
>   
> -static struct attribute *attrs[] = {
> +static struct attribute *cp500_attrs[] = {
>   	&dev_attr_version.attr,
>   	&dev_attr_keep_cfg.attr,
>   	NULL
>   };
> -static const struct attribute_group attrs_group = { .attrs = attrs };
> +ATTRIBUTE_GROUPS(cp500);
>   
>   static void cp500_i2c_release(struct device *dev)
>   {
> @@ -396,20 +396,15 @@ static int cp500_probe(struct pci_dev *pci_dev, const struct pci_device_id *id)
>   
>   	pci_set_drvdata(pci_dev, cp500);
>   
> -	ret = sysfs_create_group(&pci_dev->dev.kobj, &attrs_group);
> -	if (ret != 0)
> -		goto out_free_irq;
>   
>   	ret = cp500_enable(cp500);
>   	if (ret != 0)
> -		goto out_remove_group;
> +		goto out_free_irq;
>   
>   	cp500_register_auxiliary_devs(cp500);
>   
>   	return 0;
>   
> -out_remove_group:
> -	sysfs_remove_group(&pci_dev->dev.kobj, &attrs_group);
>   out_free_irq:
>   	pci_free_irq_vectors(pci_dev);
>   out_disable:
> @@ -427,8 +422,6 @@ static void cp500_remove(struct pci_dev *pci_dev)
>   
>   	cp500_disable(cp500);
>   
> -	sysfs_remove_group(&pci_dev->dev.kobj, &attrs_group);
> -
>   	pci_set_drvdata(pci_dev, 0);
>   
>   	pci_free_irq_vectors(pci_dev);
> @@ -450,6 +443,7 @@ static struct pci_driver cp500_driver = {
>   	.id_table = cp500_ids,
>   	.probe = cp500_probe,
>   	.remove = cp500_remove,
> +	.dev_groups = cp500_groups,
>   };
>   module_pci_driver(cp500_driver);
>   

Hello Greg,

did I made some mistakes that keeps this patch from being merged?
I think it can be included to 6.11-rc6, but maybe I'm wrong.

Thanks,
Gerhard

Re: [PATCH] misc: keba: Fix sysfs group creation

[Greg KH]

On Tue, Aug 27, 2024 at 08:27:21PM +0200, Gerhard Engleder wrote:
> On 19.08.24 21:26, Gerhard Engleder wrote:
> > From: Gerhard Engleder <eg@keba.com>
> > 
> > sysfs_create_group() races with userspace. Use dev_groups instead which
> > prevents all the problems of sysfs_create_group().
> > 
> > Fixes: a1944676767e ("misc: keba: Add basic KEBA CP500 system FPGA support")
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Gerhard Engleder <eg@keba.com>
> > ---
> >   drivers/misc/keba/cp500.c | 14 ++++----------
> >   1 file changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/misc/keba/cp500.c b/drivers/misc/keba/cp500.c
> > index 9ba46f0f9392..ae0922817881 100644
> > --- a/drivers/misc/keba/cp500.c
> > +++ b/drivers/misc/keba/cp500.c
> > @@ -212,12 +212,12 @@ static ssize_t keep_cfg_store(struct device *dev, struct device_attribute *attr,
> >   }
> >   static DEVICE_ATTR_RW(keep_cfg);
> > -static struct attribute *attrs[] = {
> > +static struct attribute *cp500_attrs[] = {
> >   	&dev_attr_version.attr,
> >   	&dev_attr_keep_cfg.attr,
> >   	NULL
> >   };
> > -static const struct attribute_group attrs_group = { .attrs = attrs };
> > +ATTRIBUTE_GROUPS(cp500);
> >   static void cp500_i2c_release(struct device *dev)
> >   {
> > @@ -396,20 +396,15 @@ static int cp500_probe(struct pci_dev *pci_dev, const struct pci_device_id *id)
> >   	pci_set_drvdata(pci_dev, cp500);
> > -	ret = sysfs_create_group(&pci_dev->dev.kobj, &attrs_group);
> > -	if (ret != 0)
> > -		goto out_free_irq;
> >   	ret = cp500_enable(cp500);
> >   	if (ret != 0)
> > -		goto out_remove_group;
> > +		goto out_free_irq;
> >   	cp500_register_auxiliary_devs(cp500);
> >   	return 0;
> > -out_remove_group:
> > -	sysfs_remove_group(&pci_dev->dev.kobj, &attrs_group);
> >   out_free_irq:
> >   	pci_free_irq_vectors(pci_dev);
> >   out_disable:
> > @@ -427,8 +422,6 @@ static void cp500_remove(struct pci_dev *pci_dev)
> >   	cp500_disable(cp500);
> > -	sysfs_remove_group(&pci_dev->dev.kobj, &attrs_group);
> > -
> >   	pci_set_drvdata(pci_dev, 0);
> >   	pci_free_irq_vectors(pci_dev);
> > @@ -450,6 +443,7 @@ static struct pci_driver cp500_driver = {
> >   	.id_table = cp500_ids,
> >   	.probe = cp500_probe,
> >   	.remove = cp500_remove,
> > +	.dev_groups = cp500_groups,
> >   };
> >   module_pci_driver(cp500_driver);
> 
> Hello Greg,
> 
> did I made some mistakes that keeps this patch from being merged?
> I think it can be included to 6.11-rc6, but maybe I'm wrong.

Please give me some time to catch up with reviews.

In the meantime, please feel free to review other changes on the mailing
lists to help us out!

thanks,

greg k-h