[PATCH] tty: nozomi: Return an error when failing to create the sysfs

[PATCH] tty: nozomi: Return an error when failing to create the sysfs

[Zheyu Ma]

The driver does not handle the error of the creation of sysfs, resulting
in duplicate file names being created.

The following log can reveal it:

[   52.907211] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:05.0/card_type'
[   52.907224] Call Trace:
[   52.907269]  sysfs_add_file_mode_ns+0x23f/0x2b0
[   52.907281]  sysfs_create_file_ns+0xe9/0x170
[   52.907321]  nozomi_card_init+0x97f/0x12c0 [nozomi]

Fix this bug by returning an error when failing to create the sysfs.

Fixes: 20fd1e3bea55 ("nozomi driver")
Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 drivers/tty/nozomi.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/nozomi.c b/drivers/tty/nozomi.c
index 0454c78deee6..d0ad1b9898f5 100644
--- a/drivers/tty/nozomi.c
+++ b/drivers/tty/nozomi.c
@@ -1282,14 +1282,26 @@ static ssize_t open_ttys_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(open_ttys);
 
-static void make_sysfs_files(struct nozomi *dc)
+static int make_sysfs_files(struct nozomi *dc)
 {
-	if (device_create_file(&dc->pdev->dev, &dev_attr_card_type))
+	int err;
+
+	err = device_create_file(&dc->pdev->dev, &dev_attr_card_type);
+	if (err) {
 		dev_err(&dc->pdev->dev,
 			"Could not create sysfs file for card_type\n");
-	if (device_create_file(&dc->pdev->dev, &dev_attr_open_ttys))
+		return err;
+	}
+
+	err = device_create_file(&dc->pdev->dev, &dev_attr_open_ttys);
+	if (err) {
+		device_remove_file(&dc->pdev->dev, &dev_attr_card_type);
 		dev_err(&dc->pdev->dev,
 			"Could not create sysfs file for open_ttys\n");
+		return err;
+	}
+
+	return 0;
 }
 
 static void remove_sysfs_files(struct nozomi *dc)
@@ -1383,7 +1395,9 @@ static int nozomi_card_init(struct pci_dev *pdev,
 
 	DBG1("base_addr: %p", dc->base_addr);
 
-	make_sysfs_files(dc);
+	ret = make_sysfs_files(dc);
+	if (ret)
+		goto err_free_irq;
 
 	dc->index_start = ndev_idx * MAX_PORT;
 	ndevs[ndev_idx] = dc;
@@ -1420,6 +1434,8 @@ static int nozomi_card_init(struct pci_dev *pdev,
 		tty_unregister_device(ntty_driver, dc->index_start + i);
 		tty_port_destroy(&dc->port[i].port);
 	}
+	remove_sysfs_files(dc);
+err_free_irq:
 	free_irq(pdev->irq, dc);
 err_free_all_kfifo:
 	i = MAX_PORT;
-- 
2.25.1

Re: [PATCH] tty: nozomi: Return an error when failing to create the sysfs

[Greg KH]

On Thu, Jun 09, 2022 at 04:31:33PM +0800, Zheyu Ma wrote:
> The driver does not handle the error of the creation of sysfs, resulting
> in duplicate file names being created.
> 
> The following log can reveal it:
> 
> [   52.907211] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:05.0/card_type'

How is the same file being created in a normal codepath?

Is the same device being registered twice somehow?

> [   52.907224] Call Trace:
> [   52.907269]  sysfs_add_file_mode_ns+0x23f/0x2b0
> [   52.907281]  sysfs_create_file_ns+0xe9/0x170
> [   52.907321]  nozomi_card_init+0x97f/0x12c0 [nozomi]
> 
> Fix this bug by returning an error when failing to create the sysfs.
> 
> Fixes: 20fd1e3bea55 ("nozomi driver")
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
>  drivers/tty/nozomi.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/nozomi.c b/drivers/tty/nozomi.c
> index 0454c78deee6..d0ad1b9898f5 100644
> --- a/drivers/tty/nozomi.c
> +++ b/drivers/tty/nozomi.c
> @@ -1282,14 +1282,26 @@ static ssize_t open_ttys_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(open_ttys);
>  
> -static void make_sysfs_files(struct nozomi *dc)
> +static int make_sysfs_files(struct nozomi *dc)
>  {
> -	if (device_create_file(&dc->pdev->dev, &dev_attr_card_type))
> +	int err;
> +
> +	err = device_create_file(&dc->pdev->dev, &dev_attr_card_type);

Drivers shouldn't be calling this function anyway, it's a race
condition.  How about switching this to use the proper device groups
functionality instead which moves all of the handling of the sysfs files
to the driver core so that the driver can not get it wrong?

That should solve the error handling case.  The fact that this is trying
to be created twice is a symptom of a worse problem here.  How are you
duplicating this?

thanks,

greg k-h

Re: [PATCH] tty: nozomi: Return an error when failing to create the sysfs

[Zheyu Ma]

On Thu, Jun 9, 2022 at 4:38 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jun 09, 2022 at 04:31:33PM +0800, Zheyu Ma wrote:
> > The driver does not handle the error of the creation of sysfs, resulting
> > in duplicate file names being created.
> >
> > The following log can reveal it:
> >
> > [   52.907211] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:05.0/card_type'
>
> How is the same file being created in a normal codepath?
>
> Is the same device being registered twice somehow?

In fact, I tried to load the nozomi driver twice.
In the first load, the driver failed at tty_port_register_device(),
performed error handling and returned an error, but by this time the
make_sysfs_files() had been executed and the sysfs had been created.
In the second load, the make_sysfs_files() is executed again and this
warning is returned.

> > [   52.907224] Call Trace:
> > [   52.907269]  sysfs_add_file_mode_ns+0x23f/0x2b0
> > [   52.907281]  sysfs_create_file_ns+0xe9/0x170
> > [   52.907321]  nozomi_card_init+0x97f/0x12c0 [nozomi]
> >
> > Fix this bug by returning an error when failing to create the sysfs.
> >
> > Fixes: 20fd1e3bea55 ("nozomi driver")
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > ---
> >  drivers/tty/nozomi.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tty/nozomi.c b/drivers/tty/nozomi.c
> > index 0454c78deee6..d0ad1b9898f5 100644
> > --- a/drivers/tty/nozomi.c
> > +++ b/drivers/tty/nozomi.c
> > @@ -1282,14 +1282,26 @@ static ssize_t open_ttys_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(open_ttys);
> >
> > -static void make_sysfs_files(struct nozomi *dc)
> > +static int make_sysfs_files(struct nozomi *dc)
> >  {
> > -     if (device_create_file(&dc->pdev->dev, &dev_attr_card_type))
> > +     int err;
> > +
> > +     err = device_create_file(&dc->pdev->dev, &dev_attr_card_type);
>
> Drivers shouldn't be calling this function anyway, it's a race
> condition.  How about switching this to use the proper device groups
> functionality instead which moves all of the handling of the sysfs files
> to the driver core so that the driver can not get it wrong?

Thanks for your reminder, since I'm not familiar with this function, I
will learn how to use the device groups functionality and revise the
patch.

> That should solve the error handling case.  The fact that this is trying
> to be created twice is a symptom of a worse problem here.  How are you
> duplicating this?

As mentioned above, I tried to load the nozomi driver twice and
make_sysfs_files() is executed twice.

thanks,

Zheyu Ma

Re: [PATCH] tty: nozomi: Return an error when failing to create the sysfs

[Greg KH]

On Thu, Jun 09, 2022 at 07:03:32PM +0800, Zheyu Ma wrote:
> On Thu, Jun 9, 2022 at 4:38 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jun 09, 2022 at 04:31:33PM +0800, Zheyu Ma wrote:
> > > The driver does not handle the error of the creation of sysfs, resulting
> > > in duplicate file names being created.
> > >
> > > The following log can reveal it:
> > >
> > > [   52.907211] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:05.0/card_type'
> >
> > How is the same file being created in a normal codepath?
> >
> > Is the same device being registered twice somehow?
> 
> In fact, I tried to load the nozomi driver twice.

How?  Modules should not be able to be in memory twice.

> In the first load, the driver failed at tty_port_register_device(),
> performed error handling and returned an error, but by this time the
> make_sysfs_files() had been executed and the sysfs had been created.
> In the second load, the make_sysfs_files() is executed again and this
> warning is returned.

Ah, ok, if you make the other changes I suggested, this will not be a
problem.

thanks,

greg k-h