atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

[Sergei Trofimovich]

Hia atl1c maintainers!

This cosmetics bothered me for some time: atl1c driver
shows unexpanded % in kernel thread names. Looks like a
minor bug:

    $ ping -f 172.16.0.1  # host1
    $ top  # host2
    ...
    621 root 20 0 0 0 0 S 11.0 0.0 0:05.01 napi/eth%d-385
    622 root 20 0 0 0 0 S  5.6 0.0 0:02.64 napi/eth%d-386
    ...

Was happening for a few years. Likely not a recent regression.

System:
- linux-5.16.1
- x86_64
- 02:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0)

From what I understand thread name comes from somewhere around:

  net/core/dev.c:
    int dev_set_threaded(struct net_device *dev, bool threaded)
    ...
        err = napi_kthread_create(napi);
    ...
    static int napi_kthread_create(struct napi_struct *n)
    ...
        n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",

  drivers/net/ethernet/atheros/atl1c/atl1c_main.c:
    static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
    ...
        dev_set_threaded(netdev, true);

  ${somewhere} (not sure where):
    ...
      strcpy(netdev->name, "eth%d");

I was not able to pinpoint where expansion should ideally happen.
Looks like many driver do `strcpy(netdev->name, "eth%d");` style
initialization and almost none call `dev_set_threaded(netdev, true);`.

Can you help me find it out how it should be fixed?

Thank you!

-- 

  Sergei

Re: atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

[Andrew Lunn]

On Fri, Jan 21, 2022 at 09:57:47PM +0000, Sergei Trofimovich wrote:
> Hia atl1c maintainers!
> 
> This cosmetics bothered me for some time: atl1c driver
> shows unexpanded % in kernel thread names. Looks like a
> minor bug:
> 
>     $ ping -f 172.16.0.1  # host1
>     $ top  # host2
>     ...
>     621 root 20 0 0 0 0 S 11.0 0.0 0:05.01 napi/eth%d-385
>     622 root 20 0 0 0 0 S  5.6 0.0 0:02.64 napi/eth%d-386
>     ...
> 
> Was happening for a few years. Likely not a recent regression.
> 
> System:
> - linux-5.16.1
> - x86_64
> - 02:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0)
> 
> >From what I understand thread name comes from somewhere around:
> 
>   net/core/dev.c:
>     int dev_set_threaded(struct net_device *dev, bool threaded)
>     ...
>         err = napi_kthread_create(napi);
>     ...
>     static int napi_kthread_create(struct napi_struct *n)
>     ...
>         n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",
> 
>   drivers/net/ethernet/atheros/atl1c/atl1c_main.c:
>     static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>     ...
>         dev_set_threaded(netdev, true);
> 
>   ${somewhere} (not sure where):
>     ...
>       strcpy(netdev->name, "eth%d");
> 
> I was not able to pinpoint where expansion should ideally happen.
> Looks like many driver do `strcpy(netdev->name, "eth%d");` style
> initialization and almost none call `dev_set_threaded(netdev, true);`.
> 
> Can you help me find it out how it should be fixed?

Hi Sergei

This is a fun one.

So, the driver does the usual alloc_etherdev_mq()

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/atheros/atl1c/atl1c_main.c#L2703

which ends up here:

https://elixir.bootlin.com/linux/latest/source/net/ethernet/eth.c#L391

struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
                                      unsigned int rxqs)
{
        return alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_UNKNOWN,
                                ether_setup, txqs, rxqs);
}

So at this point in time, the device has the name "eth%d".

The normal flow is that sometime later in probe, it calls
register_netdev().

https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L10454

if you follow that down, you get to: __dev_alloc_name(), which does
the expansion of the %d to an actual number:

https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L1087

So between alloc_etherdev_mq() and register_netdev(), the device name
is not valid. And as you pointed out, dev_set_threaded() tries to use
the name, and is called between these two.

The atl1c driver appears to be the only driver actually doing
this. There is a sysfs interface which can call dev_set_threaded(),
but the sysfs interface is probably not available until after
register_netdev() has given the interface its name.

There is a fix for atl1c. Any time after alloc_etherdev_mq(), the
driver can call dev_alloc_name().

So please give this a try. I've not even compile tested it...

iff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index da595242bc13..983a52f77bda 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -2706,6 +2706,10 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
                goto err_alloc_etherdev;
        }
 
+       err = dev_alloc_name(netdev, netdev->name);
+       if (err < 0)
+               goto err_init_netdev;
+
        err = atl1c_init_netdev(netdev, pdev);
        if (err) {
                dev_err(&pdev->dev, "init netdevice failed\n");

If this works, i can turn it into a real patch submission.

   Andrew

Re: atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

[Stephen Hemminger]

On Sat, 22 Jan 2022 00:45:53 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Jan 21, 2022 at 09:57:47PM +0000, Sergei Trofimovich wrote:
> > Hia atl1c maintainers!
> > 
> > This cosmetics bothered me for some time: atl1c driver
> > shows unexpanded % in kernel thread names. Looks like a
> > minor bug:
> > 
> >     $ ping -f 172.16.0.1  # host1
> >     $ top  # host2
> >     ...
> >     621 root 20 0 0 0 0 S 11.0 0.0 0:05.01 napi/eth%d-385
> >     622 root 20 0 0 0 0 S  5.6 0.0 0:02.64 napi/eth%d-386
> >     ...
> > 
> > Was happening for a few years. Likely not a recent regression.
> > 
> > System:
> > - linux-5.16.1
> > - x86_64
> > - 02:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0)
> >   
> > >From what I understand thread name comes from somewhere around:  
> > 
> >   net/core/dev.c:
> >     int dev_set_threaded(struct net_device *dev, bool threaded)
> >     ...
> >         err = napi_kthread_create(napi);
> >     ...
> >     static int napi_kthread_create(struct napi_struct *n)
> >     ...
> >         n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",
> > 
> >   drivers/net/ethernet/atheros/atl1c/atl1c_main.c:
> >     static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >     ...
> >         dev_set_threaded(netdev, true);
> > 
> >   ${somewhere} (not sure where):
> >     ...
> >       strcpy(netdev->name, "eth%d");
> > 
> > I was not able to pinpoint where expansion should ideally happen.
> > Looks like many driver do `strcpy(netdev->name, "eth%d");` style
> > initialization and almost none call `dev_set_threaded(netdev, true);`.
> > 
> > Can you help me find it out how it should be fixed?  
> 
> Hi Sergei
> 
> This is a fun one.
> 
> So, the driver does the usual alloc_etherdev_mq()
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/atheros/atl1c/atl1c_main.c#L2703
> 
> which ends up here:
> 
> https://elixir.bootlin.com/linux/latest/source/net/ethernet/eth.c#L391
> 
> struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
>                                       unsigned int rxqs)
> {
>         return alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_UNKNOWN,
>                                 ether_setup, txqs, rxqs);
> }
> 
> So at this point in time, the device has the name "eth%d".
> 
> The normal flow is that sometime later in probe, it calls
> register_netdev().
> 
> https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L10454
> 
> if you follow that down, you get to: __dev_alloc_name(), which does
> the expansion of the %d to an actual number:
> 
> https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L1087
> 
> So between alloc_etherdev_mq() and register_netdev(), the device name
> is not valid. And as you pointed out, dev_set_threaded() tries to use
> the name, and is called between these two.
> 
> The atl1c driver appears to be the only driver actually doing
> this. There is a sysfs interface which can call dev_set_threaded(),
> but the sysfs interface is probably not available until after
> register_netdev() has given the interface its name.
> 
> There is a fix for atl1c. Any time after alloc_etherdev_mq(), the
> driver can call dev_alloc_name().
> 
> So please give this a try. I've not even compile tested it...
> 
> iff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index da595242bc13..983a52f77bda 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -2706,6 +2706,10 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>                 goto err_alloc_etherdev;
>         }
>  
> +       err = dev_alloc_name(netdev, netdev->name);
> +       if (err < 0)
> +               goto err_init_netdev;
> +
>         err = atl1c_init_netdev(netdev, pdev);
>         if (err) {
>                 dev_err(&pdev->dev, "init netdevice failed\n");
> 
> If this works, i can turn it into a real patch submission.
> 
>    Andrew


This may not work right because probe is not called with RTNL.
And the alloc_name is using RTNL to prevent two devices from
getting the same name.

Re: atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

[Andrew Lunn]

> > So please give this a try. I've not even compile tested it...
> > 
> > iff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > index da595242bc13..983a52f77bda 100644
> > --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > @@ -2706,6 +2706,10 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >                 goto err_alloc_etherdev;
> >         }
> >  
> > +       err = dev_alloc_name(netdev, netdev->name);
> > +       if (err < 0)
> > +               goto err_init_netdev;
> > +
> >         err = atl1c_init_netdev(netdev, pdev);
> >         if (err) {
> >                 dev_err(&pdev->dev, "init netdevice failed\n");
> > 
> > If this works, i can turn it into a real patch submission.
> > 
> >    Andrew
> 
> 
> This may not work right because probe is not called with RTNL.
> And the alloc_name is using RTNL to prevent two devices from
> getting the same name.

Oh, yes. I looked at some of the users. And some do take rtnl before
calling it. And some don't!

Looking at register_netdev(), it seems we need something like:

	if (rtnl_lock_killable()) {
	       err = -EINTR;
	       goto err_init_netdev;
	}
	err = dev_alloc_name(netdev, netdev->name);
	rtnl_unlock();
	if (err < 0)
		goto err_init_netdev;


It might also be a good idea to put a ASSERT_RTNL() in
__dev_alloc_name() to catch any driver doing this wrong.

	   Andrew

Re: atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

[Sergei Trofimovich]

On Sat, 22 Jan 2022 02:53:42 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > > So please give this a try. I've not even compile tested it...
> > > 
> > > iff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > > index da595242bc13..983a52f77bda 100644
> > > --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > > +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > > @@ -2706,6 +2706,10 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >                 goto err_alloc_etherdev;
> > >         }
> > >  
> > > +       err = dev_alloc_name(netdev, netdev->name);
> > > +       if (err < 0)
> > > +               goto err_init_netdev;
> > > +
> > >         err = atl1c_init_netdev(netdev, pdev);
> > >         if (err) {
> > >                 dev_err(&pdev->dev, "init netdevice failed\n");
> > > 
> > > If this works, i can turn it into a real patch submission.
> > > 
> > >    Andrew  
> > 
> > 
> > This may not work right because probe is not called with RTNL.
> > And the alloc_name is using RTNL to prevent two devices from
> > getting the same name.  
> 
> Oh, yes. I looked at some of the users. And some do take rtnl before
> calling it. And some don't!
> 
> Looking at register_netdev(), it seems we need something like:
> 
> 	if (rtnl_lock_killable()) {
> 	       err = -EINTR;
> 	       goto err_init_netdev;
> 	}
> 	err = dev_alloc_name(netdev, netdev->name);
> 	rtnl_unlock();
> 	if (err < 0)
> 		goto err_init_netdev;
> 
> 
> It might also be a good idea to put a ASSERT_RTNL() in
> __dev_alloc_name() to catch any driver doing this wrong.

Thank you Andrew! I used this second version of your patch
against 5.16.1 and it seems to work:

    $ sudo ping -f 172.16.0.1

    613 root 20 0 0 0 0 S 11.0 0.0 0:07.46 napi/eth0-385
    614 root 20 0 0 0 0 R  5.3 0.0 0:03.96 napi/eth0-386

Posting used diff as is just in case:

Tested-by: Sergei Trofimovich <slyich@gmail.com>

--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -2706,6 +2706,15 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto err_alloc_etherdev;
 	}

+	if (rtnl_lock_killable()) {
+		err = -EINTR;
+		goto err_init_netdev;
+	}
+	err = dev_alloc_name(netdev, netdev->name);
+	rtnl_unlock();
+	if (err < 0)
+		goto err_init_netdev;
+
 	err = atl1c_init_netdev(netdev, pdev);
 	if (err) {
 		dev_err(&pdev->dev, "init netdevice failed\n");

-- 

  Sergei

Re: atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

[Andrew Lunn]

> Thank you Andrew! I used this second version of your patch
> against 5.16.1 and it seems to work:
> 
>     $ sudo ping -f 172.16.0.1
> 
>     613 root 20 0 0 0 0 S 11.0 0.0 0:07.46 napi/eth0-385
>     614 root 20 0 0 0 0 R  5.3 0.0 0:03.96 napi/eth0-386
> 
> Posting used diff as is just in case:
> 
> Tested-by: Sergei Trofimovich <slyich@gmail.com>

Great, thanks for testing.

> 
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -2706,6 +2706,15 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto err_alloc_etherdev;
>  	}
> 
> +	if (rtnl_lock_killable()) {
> +		err = -EINTR;
> +		goto err_init_netdev;
> +	}
> +	err = dev_alloc_name(netdev, netdev->name);
> +	rtnl_unlock();
> +	if (err < 0)
> +		goto err_init_netdev;

Since there are multiple users of dev_alloc_name() and it appears some
get locking wrong, it makes sense to add a helper in the code which
does the locking. So i will work on a patchset to add such a helper
and convert other drivers.

    Andrew

Re: atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

[Andrew Lunn]

> > Oh, yes. I looked at some of the users. And some do take rtnl before
> > calling it. And some don't!
> > 
> > Looking at register_netdev(), it seems we need something like:
> > 
> > 	if (rtnl_lock_killable()) {
> > 	       err = -EINTR;
> > 	       goto err_init_netdev;
> > 	}
> > 	err = dev_alloc_name(netdev, netdev->name);
> > 	rtnl_unlock();
> > 	if (err < 0)
> > 		goto err_init_netdev;
> > 
> > 
> > It might also be a good idea to put a ASSERT_RTNL() in
> > __dev_alloc_name() to catch any driver doing this wrong.

I looked at it some more, and some of the current users. And this does
not really work. There is a race condition.

Taking rtnl means you at least get a valid name, while you hold
rtnl. But it does not keep track of the name it just gave out. As a
result, you can release rtnl, and another device can jump in and be
given the same name in register_netdev(). When this driver then calls
register_netdev() the core will notice the clash and return -EEXISTS,
causing the probe to fail.

There are some drivers which take rtnl and keep it until after calling
register_netdevice(), rather than register_netdev(), but this is
rather ugly. And there are some drivers which don't take the lock, and
just hope they don't hit the race.

Maybe a better fix for this driver is:

From a5fc0e127bdc4b6ba4fb923012729cbf3d529996 Mon Sep 17 00:00:00 2001
From: Andrew Lunn <andrew@lunn.ch>
Date: Sat, 22 Jan 2022 13:33:58 -0600
Subject: [PATCH] net: ethernet: atl1c: Move dev_set_threaded() after
 register_netdev()

dev_set_threaded() creates new kernel threads to perform napi. The
threads are given a name based on the interface name. However, the
interface is not allocated a name until register_netdev() is called.
By moving the call to dev_set_threaded() to later in the probe
function, odd thread names like napi/eth%d-385 are avoided.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index da595242bc13..9b8088905946 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -2728,7 +2728,7 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
        adapter->mii.mdio_write = atl1c_mdio_write;
        adapter->mii.phy_id_mask = 0x1f;
        adapter->mii.reg_num_mask = MDIO_CTRL_REG_MASK;
-       dev_set_threaded(netdev, true);
+
        for (i = 0; i < adapter->rx_queue_count; ++i)
                netif_napi_add(netdev, &adapter->rrd_ring[i].napi,
                               atl1c_clean_rx, 64);
@@ -2781,6 +2781,8 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
                goto err_register;
        }
 
+       dev_set_threaded(netdev, true);
+
        cards_found++;
        return 0;
 
-- 
2.34.1

Re: atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

[Sergei Trofimovich]

On Sat, 22 Jan 2022 20:40:51 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > > Oh, yes. I looked at some of the users. And some do take rtnl before
> > > calling it. And some don't!
> > > 
> > > Looking at register_netdev(), it seems we need something like:
> > > 
> > > 	if (rtnl_lock_killable()) {
> > > 	       err = -EINTR;
> > > 	       goto err_init_netdev;
> > > 	}
> > > 	err = dev_alloc_name(netdev, netdev->name);
> > > 	rtnl_unlock();
> > > 	if (err < 0)
> > > 		goto err_init_netdev;
> > > 
> > > 
> > > It might also be a good idea to put a ASSERT_RTNL() in
> > > __dev_alloc_name() to catch any driver doing this wrong.  
> 
> I looked at it some more, and some of the current users. And this does
> not really work. There is a race condition.
> 
> Taking rtnl means you at least get a valid name, while you hold
> rtnl. But it does not keep track of the name it just gave out. As a
> result, you can release rtnl, and another device can jump in and be
> given the same name in register_netdev(). When this driver then calls
> register_netdev() the core will notice the clash and return -EEXISTS,
> causing the probe to fail.
> 
> There are some drivers which take rtnl and keep it until after calling
> register_netdevice(), rather than register_netdev(), but this is
> rather ugly. And there are some drivers which don't take the lock, and
> just hope they don't hit the race.
> 
> Maybe a better fix for this driver is:
> 
> From a5fc0e127bdc4b6ba4fb923012729cbf3d529996 Mon Sep 17 00:00:00 2001
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Sat, 22 Jan 2022 13:33:58 -0600
> Subject: [PATCH] net: ethernet: atl1c: Move dev_set_threaded() after
>  register_netdev()
> 
> dev_set_threaded() creates new kernel threads to perform napi. The
> threads are given a name based on the interface name. However, the
> interface is not allocated a name until register_netdev() is called.
> By moving the call to dev_set_threaded() to later in the probe
> function, odd thread names like napi/eth%d-385 are avoided.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

This patch also works:

    687 root 20 0 0 0 0 R 6.7   0.0 0:00.15 napi/eth0-386
    688 root 20 0 0 0 0 S 6.7   0.0 0:00.32 napi/eth0-385

tested in the same environment on top of 5.16.1:

Tested-by: Sergei Trofimovich <slyich@gmail.com>

Thank you!

> ---
>  drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index da595242bc13..9b8088905946 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -2728,7 +2728,7 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>         adapter->mii.mdio_write = atl1c_mdio_write;
>         adapter->mii.phy_id_mask = 0x1f;
>         adapter->mii.reg_num_mask = MDIO_CTRL_REG_MASK;
> -       dev_set_threaded(netdev, true);
> +
>         for (i = 0; i < adapter->rx_queue_count; ++i)
>                 netif_napi_add(netdev, &adapter->rrd_ring[i].napi,
>                                atl1c_clean_rx, 64);
> @@ -2781,6 +2781,8 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>                 goto err_register;
>         }
>  
> +       dev_set_threaded(netdev, true);
> +
>         cards_found++;
>         return 0;
>  
> -- 
> 2.34.1


-- 

  Sergei