kernel-boot: Tighten check if device is virtual

PATCH In review Leon Romanovsky 4 participants 9 messages Sep 26, 2019 – Oct 5, 2019 InfiniBand

lore

Share & tools

lore

Leon Romanovsky

author

2019-09-26 09:43 UTC #0 lore raw

PATCH 1/1 kernel-boot: Tighten check if device is virtual

From: Leon Romanovsky <[email protected]>

Virtual devices like SIW or RXE don't set FW version because they don't have one, use that fact to rely on having empty fw_ver file to sense such virtual devices.

Such change is needed to ensure that virtual devices which are attached to real hardware won't be renamed, because during device attachment, user already supplied desired name.

Signed-off-by Leon Romanovsky <[email protected]>

---
 kernel-boot/rdma_rename.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

kernel-boot/rdma_rename.c +37 -2

index 41649d7d0..1588a0f45 100644
@@ -356,9 +356,12 @@ err_free:
 
 static int by_pci(struct data *d)
 {
+	bool is_virtual = false;
 	struct pci_info p = {};
 	char *subsystem;
 	char buf[256] = {};
+	FILE *fw_ver_p;
+	char *fw_ver;
 	char *subs;
 	int ret;

@@ -373,9 +376,41 @@ static int by_pci(struct data *d)
 		goto out;
 	}
 	buf[ret] = 0;
-
 	subs = basename(buf);
-	if (strcmp(subs, "pci")) {
+
+	ret =  asprintf(&fw_ver, "/sys/class/infiniband/%s/fw_ver", d->curr);
+	if (ret < 0) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = 0;
+	fw_ver_p = fopen(fw_ver, "r");
+	if (fw_ver_p) {
+		char *s = fgets(buf, 3, fw_ver_p);
+
+		fclose(fw_ver_p);
+
+		if (!s)
+			/* Signal that we can't read fw_ver file */
+			ret = -EINVAL;
+		/*
+		 * Virtual devices like SIW and RXE
+		 * don't set their FW version
+		 */
+		if (strlen(s) < 2)
+			is_virtual = true;
+	} else {
+		ret = -EINVAL;
+	}
+
+	if (ret) {
+		/* Something very wrong, all IB devices have fw_ver file */
+		pr_err("%s: Can't open/read fw_ver file\n", d->curr);
+		goto out;
+	}
+
+	if (strcmp(subs, "pci") || is_virtual) {
 		/* Ball out virtual devices */
 		pr_dbg("%s: Non-PCI device (%s) was detected\n", d->curr, subs);
 		ret = -EINVAL;

Signature · 1 line

2.20.1

Leon Romanovsky

2019-09-26 12:29 UTC #1 lore raw

Quoted 6 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>
Virtual devices like SIW or RXE don't set FW version because they don't have one, use that fact to rely on having empty fw_ver file to sense such virtual devices.

Quoted 8 lines

Such change is needed to ensure that virtual devices which are attached to real hardware won't be renamed, because during device attachment, user already supplied desired name.
Signed-off-by Leon Romanovsky <[email protected]>
---
 kernel-boot/rdma_rename.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

I have slightly better variant of this patch, but want to get feedback on the concept before reposting.

Thanks

Jason Gunthorpe

2019-09-26 12:34 UTC #2 lore raw

In reply to Leon Romanovsky

Quoted 5 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>
Virtual devices like SIW or RXE don't set FW version because they don't have one, use that fact to rely on having empty fw_ver file to sense such virtual devices.

Have you checked that every physical device does set fw version?

Seems hacky

Jason

Leon Romanovsky

author

2019-09-26 12:42 UTC #3 lore raw

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Virtual devices like SIW or RXE don't set FW version because they don't have one, use that fact to rely on having empty fw_ver file to sense such virtual devices.
Have you checked that every physical device does set fw version?

I checked it with "grep", another solution which I had in mind is to write "virtual" keyword as FW version.

Thanks

Quoted 4 lines

Seems hacky
Jason

Jonathan Toppins

2019-09-26 17:58 UTC #4 lore raw

In reply to Jason Gunthorpe

Quoted 10 lines · Jason Gunthorpe

Quoted 5 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Virtual devices like SIW or RXE don't set FW version because they don't have one, use that fact to rely on having empty fw_ver file to sense such virtual devices.
Have you checked that every physical device does set fw version?
Seems hacky

agreed, how are tuntap devices handled, is there a similar handling that can be applied here?

Leon Romanovsky

author

2019-09-28 16:54 UTC #5 lore raw

Quoted 8 lines · Jonathan Toppins

Quoted 7 lines · Jason Gunthorpe
Quoted 5 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Virtual devices like SIW or RXE don't set FW version because they don't have one, use that fact to rely on having empty fw_ver file to sense such virtual devices.

Quoted 6 lines

Quoted 3 lines
Have you checked that every physical device does set fw version?
Seems hacky
agreed, how are tuntap devices handled, is there a similar handling that can be applied here?

Unfortunately, we can't do the same, RDMA doesn't have notion of stacked devices.

1.
TUN devices are initialized with ARPHRD_NONE type.
https://elixir.bootlin.com/linux/latest/source/drivers/net/tun.c#L1396

It causes for systemd-udev to skip their rename.
https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L781

2.
TAP devices are skipped due to the fact that iflink != ifindex on such devices.
https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L810

So, yes hacky, but the solution is tailored to RDMA subsystem where ALL devices have FW and we can ensure that ALL future devices will report any sort of string through fw_ver file.

Thanks

Quoted 1 line

Jason Gunthorpe

2019-10-03 16:31 UTC #6 lore raw

Quoted 25 lines · Leon Romanovsky

Quoted 8 lines · Jonathan Toppins
Quoted 7 lines · Jason Gunthorpe
Quoted 5 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Virtual devices like SIW or RXE don't set FW version because they don't have one, use that fact to rely on having empty fw_ver file to sense such virtual devices.
Quoted 6 lines
Quoted 3 lines
Have you checked that every physical device does set fw version?
Seems hacky
agreed, how are tuntap devices handled, is there a similar handling that can be applied here?
Unfortunately, we can't do the same, RDMA doesn't have notion of stacked devices.
1.
TUN devices are initialized with ARPHRD_NONE type.
https://elixir.bootlin.com/linux/latest/source/drivers/net/tun.c#L1396
It causes for systemd-udev to skip their rename.
https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L781

Quoted 7 lines

2.
TAP devices are skipped due to the fact that iflink != ifindex on such devices.
https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L810
So, yes hacky, but the solution is tailored to RDMA subsystem where ALL devices have FW and we can ensure that ALL future devices will report any sort of string through fw_ver file.

It still seems really hacky, why not add some device flag or something instead? Is this better because it works with old kernels?

Jason

Leon Romanovsky

author

2019-10-05 06:12 UTC #7 lore raw

Quoted 30 lines · Jason Gunthorpe

Quoted 29 lines · Leon Romanovsky
Quoted 8 lines · Jonathan Toppins
Quoted 7 lines · Jason Gunthorpe
Quoted 5 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Virtual devices like SIW or RXE don't set FW version because they don't have one, use that fact to rely on having empty fw_ver file to sense such virtual devices.
Quoted 6 lines
Quoted 3 lines
Have you checked that every physical device does set fw version?
Seems hacky
agreed, how are tuntap devices handled, is there a similar handling that can be applied here?
Unfortunately, we can't do the same, RDMA doesn't have notion of stacked devices.
1.
TUN devices are initialized with ARPHRD_NONE type.
https://elixir.bootlin.com/linux/latest/source/drivers/net/tun.c#L1396
It causes for systemd-udev to skip their rename.
https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L781
2.
TAP devices are skipped due to the fact that iflink != ifindex on such devices.
https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L810

Quoted 6 lines

Quoted 3 lines
So, yes hacky, but the solution is tailored to RDMA subsystem where ALL devices have FW and we can ensure that ALL future devices will report any sort of string through fw_ver file.
It still seems really hacky, why not add some device flag or something instead? Is this better because it works with old kernels?

Yes, I'm trying to find a way to do such discovery without changing kernel, because we have only two virtual devices and I don't expect to see any new ones in forth coming future.

However, this patch is wrong because there are at least two drivers (hns and EFA) didn't implement .get_dev_fw_str() and they will have empty fw_ver.

805 void ib_get_device_fw_str(struct ib_device *dev, char *str) 806 {
807 if (dev->ops.get_dev_fw_str)
808 dev->ops.get_dev_fw_str(dev, str);
809 else
810 str[0] = '\0';
811 }
812 EXPORT_SYMBOL(ib_get_device_fw_str);

Special flag seems too much for me, what about writing special string to fw_ver to indicate virtual device? For example "virtual".

Thanks

Quoted 2 lines

Jason

Leon Romanovsky

2019-10-02 11:47 UTC #8 lore raw

In reply to Leon Romanovsky

Quoted 6 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>
Virtual devices like SIW or RXE don't set FW version because they don't have one, use that fact to rely on having empty fw_ver file to sense such virtual devices.

Quoted 8 lines

Such change is needed to ensure that virtual devices which are attached to real hardware won't be renamed, because during device attachment, user already supplied desired name.
Signed-off-by Leon Romanovsky <[email protected]>
---
 kernel-boot/rdma_rename.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

PR https://github.com/linux-rdma/rdma-core/pull/588

Thanks