SR-IOV & virtfn/physfn sysfs links

SR-IOV & virtfn/physfn sysfs links

[Stuart Hayes]

Bjorn et al,

I'm hoping to enhance network device naming in systemd to parse sysfs "virtfn%u" and "physfn" links, so it understands virtual devices.

I've noticed that the udev event which triggers the systemd/biosdevname network device naming--the udev "add" event for the network device, which happens within pci_bus_add_device() when the network driver's probe function calls register_netdev()--occurs before the sysfs links "physfn" and "virtfn%u" are created.  This creates a race when the network naming code tries to look at those links (something biosdevname already does).

Is there any reason why we couldn't switch the order of those calls to eliminate the race, as shown in the patch below?  I couldn't think of any, since those links apply to the pci subsystem devices in sysfs (which are already created), not the net subsystem devices.  I'd be happy to submit a patch if that looks ok.

Thank you,
Stuart


--- linux-4.14-rc1/drivers/pci/iov.c.orig	2017-09-18 15:00:43.168255665 -0500
+++ linux-4.14-rc1/drivers/pci/iov.c	2017-09-18 15:07:04.792280999 -0500
@@ -162,7 +162,6 @@ int pci_iov_add_virtfn(struct pci_dev *d
 
 	pci_device_add(virtfn, virtfn->bus);
 
-	pci_bus_add_device(virtfn);
 	sprintf(buf, "virtfn%u", id);
 	rc = sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf);
 	if (rc)
@@ -173,6 +172,8 @@ int pci_iov_add_virtfn(struct pci_dev *d
 
 	kobject_uevent(&virtfn->dev.kobj, KOBJ_CHANGE);
 
+	pci_bus_add_device(virtfn);
+
 	return 0;
 
 failed2:


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: SR-IOV & virtfn/physfn sysfs links

[Alexander Duyck]

On Mon, Sep 18, 2017 at 6:53 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wr=
ote:
> Bjorn et al,
>
> I'm hoping to enhance network device naming in systemd to parse sysfs "vi=
rtfn%u" and "physfn" links, so it understands virtual devices.
>
> I've noticed that the udev event which triggers the systemd/biosdevname n=
etwork device naming--the udev "add" event for the network device, which ha=
ppens within pci_bus_add_device() when the network driver's probe function =
calls register_netdev()--occurs before the sysfs links "physfn" and "virtfn=
%u" are created.  This creates a race when the network naming code tries to=
 look at those links (something biosdevname already does).
>
> Is there any reason why we couldn't switch the order of those calls to el=
iminate the race, as shown in the patch below?  I couldn't think of any, si=
nce those links apply to the pci subsystem devices in sysfs (which are alre=
ady created), not the net subsystem devices.  I'd be happy to submit a patc=
h if that looks ok.
>
> Thank you,
> Stuart
>
>
> --- linux-4.14-rc1/drivers/pci/iov.c.orig       2017-09-18 15:00:43.16825=
5665 -0500
> +++ linux-4.14-rc1/drivers/pci/iov.c    2017-09-18 15:07:04.792280999 -05=
00
> @@ -162,7 +162,6 @@ int pci_iov_add_virtfn(struct pci_dev *d
>
>         pci_device_add(virtfn, virtfn->bus);
>
> -       pci_bus_add_device(virtfn);
>         sprintf(buf, "virtfn%u", id);
>         rc =3D sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf);
>         if (rc)
> @@ -173,6 +172,8 @@ int pci_iov_add_virtfn(struct pci_dev *d
>
>         kobject_uevent(&virtfn->dev.kobj, KOBJ_CHANGE);
>
> +       pci_bus_add_device(virtfn);
> +
>         return 0;
>
>  failed2:
>
>

So looking over the change the only thing I wasn't sure about was if
the exception handling paths all worked out correctly, and from what I
can tell it looks like this change didn't introduce any new errors.

The one thing I think might be here though before you patch was
introduced is a possible memory leak if pci_setup_device() fails as we
don't ever free the memory allocated to virtfn. It looks liike
checking for the error was added somewhat recently and I suspect
nobody was checking for the memory leak. I don't know if you would
want to get that as a part of your patchset or not. If not I can
probably try to get around to it later this week.

- Alex

Re: SR-IOV & virtfn/physfn sysfs links

[Bjorn Helgaas]

On Mon, Sep 18, 2017 at 08:53:45PM -0500, Stuart Hayes wrote:
> Bjorn et al,
> 
> I'm hoping to enhance network device naming in systemd to parse
> sysfs "virtfn%u" and "physfn" links, so it understands virtual
> devices.
> 
> I've noticed that the udev event which triggers the
> systemd/biosdevname network device naming--the udev "add" event for
> the network device, which happens within pci_bus_add_device() when
> the network driver's probe function calls register_netdev()--occurs
> before the sysfs links "physfn" and "virtfn%u" are created.  This
> creates a race when the network naming code tries to look at those
> links (something biosdevname already does).
> 
> Is there any reason why we couldn't switch the order of those calls
> to eliminate the race, as shown in the patch below?  I couldn't
> think of any, since those links apply to the pci subsystem devices
> in sysfs (which are already created), not the net subsystem devices.
> I'd be happy to submit a patch if that looks ok.

Seems plausible.  I think the sequence you're describing is this:

  pci_iov_add_virtfn
    pci_bus_add_device
      pci_create_sysfs_dev_files
      device_attach
        ...
	  register_netdev
	    ...
	      # udev "add" event
    sysfs_create_link("virtfn%u)
    sysfs_create_link("physfn")
    kobject_uevent(KOBJ_CHANGE)

We create most of the sysfs files before attaching the driver, and it
makes sense to me that we should set up the IOV sysfs files as well.

Bjorn

> --- linux-4.14-rc1/drivers/pci/iov.c.orig	2017-09-18 15:00:43.168255665 -0500
> +++ linux-4.14-rc1/drivers/pci/iov.c	2017-09-18 15:07:04.792280999 -0500
> @@ -162,7 +162,6 @@ int pci_iov_add_virtfn(struct pci_dev *d
>  
>  	pci_device_add(virtfn, virtfn->bus);
>  
> -	pci_bus_add_device(virtfn);
>  	sprintf(buf, "virtfn%u", id);
>  	rc = sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf);
>  	if (rc)
> @@ -173,6 +172,8 @@ int pci_iov_add_virtfn(struct pci_dev *d
>  
>  	kobject_uevent(&virtfn->dev.kobj, KOBJ_CHANGE);
>  
> +	pci_bus_add_device(virtfn);
> +
>  	return 0;
>  
>  failed2:
> 
> 
> ---
> This email has been checked for viruses by Avast antivirus software.
> https://www.avast.com/antivirus
> 

[PATCH] PCI: Create iov virtfn/physfn links before attaching driver

[Stuart Hayes]

When creating virtual functions, create the "virtfn%u" and "physfn" links 
in sysfs before attaching the driver.  Without this, there is a race when 
the driver attaches to the new virtual network interface and sends out an 
"add" udev event, and the network interface naming software (biosdevname
or systemd, for example) tries to look at these links.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---

--- linux-4.14-rc1/drivers/pci/iov.c.orig	2017-09-18 15:00:43.168255665 -0500
+++ linux-4.14-rc1/drivers/pci/iov.c	2017-09-18 15:07:04.792280999 -0500
@@ -162,7 +162,6 @@ int pci_iov_add_virtfn(struct pci_dev *d
 
 	pci_device_add(virtfn, virtfn->bus);
 
-	pci_bus_add_device(virtfn);
 	sprintf(buf, "virtfn%u", id);
 	rc = sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf);
 	if (rc)
@@ -173,6 +172,8 @@ int pci_iov_add_virtfn(struct pci_dev *d
 
 	kobject_uevent(&virtfn->dev.kobj, KOBJ_CHANGE);
 
+	pci_bus_add_device(virtfn);
+
 	return 0;
 
 failed2:

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: [PATCH] PCI: Create iov virtfn/physfn links before attaching driver

[Bjorn Helgaas]

On Wed, Oct 04, 2017 at 10:57:52AM -0500, Stuart Hayes wrote:
> When creating virtual functions, create the "virtfn%u" and "physfn" links 
> in sysfs before attaching the driver.  Without this, there is a race when 
> the driver attaches to the new virtual network interface and sends out an 
> "add" udev event, and the network interface naming software (biosdevname
> or systemd, for example) tries to look at these links.
> 
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>

Applied to pci/virtualization for v4.15, thanks!

> ---
> 
> --- linux-4.14-rc1/drivers/pci/iov.c.orig	2017-09-18 15:00:43.168255665 -0500
> +++ linux-4.14-rc1/drivers/pci/iov.c	2017-09-18 15:07:04.792280999 -0500
> @@ -162,7 +162,6 @@ int pci_iov_add_virtfn(struct pci_dev *d
>  
>  	pci_device_add(virtfn, virtfn->bus);
>  
> -	pci_bus_add_device(virtfn);
>  	sprintf(buf, "virtfn%u", id);
>  	rc = sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf);
>  	if (rc)
> @@ -173,6 +172,8 @@ int pci_iov_add_virtfn(struct pci_dev *d
>  
>  	kobject_uevent(&virtfn->dev.kobj, KOBJ_CHANGE);
>  
> +	pci_bus_add_device(virtfn);
> +
>  	return 0;
>  
>  failed2:
> 
> ---
> This email has been checked for viruses by Avast antivirus software.
> https://www.avast.com/antivirus
>