[PATCH v4 0/3] IB/ipoib: Use dev_port to disambiguate port numbers

[PATCH v4 0/3] IB/ipoib: Use dev_port to disambiguate port numbers

[Arseny Maslennikov]

Pre-3.15 userspace had trouble distinguishing different ports
of a NIC on a single PCI bus/device/function. To solve this,
a sysfs field `dev_port' was introduced quite a while ago
(commit v3.14-rc3-739-g3f85944fe207), and some relevant device
drivers were fixed to use it, but not in case of IPoIB.

The convention for some reason never got documented in the kernel, but
was immediately adopted by userspace (notably udev[1][2], biosdevname[3])

1/3 documents the sysfs field — that's why I'm CC-ing netdev.

This series was tested on and applies to 4.19-rc2.

[1] https://lists.freedesktop.org/archives/systemd-devel/2014-June/020788.html
[2] https://lists.freedesktop.org/archives/systemd-devel/2014-July/020804.html
[3] https://github.com/CloudAutomationNTools/biosdevname/blob/c795d51dd93a5309652f0d635f12a3ecfabfaa72/src/eths.c#L38

v1->v2: replace a line instead of inserting and then removing.
v2->v3: restore both attributes, output a notice of deprecation to kmsg.
v3->v4: style adjustments, join the deprecation notice to single line.

Arseny Maslennikov (3):
  Documentation/ABI: document /sys/class/net/*/dev_port
  IB/ipoib: Use dev_port to expose network interface port numbers
  IB/ipoib: Log sysfs 'dev_id' accesses from userspace

 Documentation/ABI/testing/sysfs-class-net | 18 +++++++++++++
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 33 +++++++++++++++++++++++
 2 files changed, 51 insertions(+)

-- 
2.19.0.rc2

[PATCH v4 1/3] Documentation/ABI: document /sys/class/net/*/dev_port

[Arseny Maslennikov]

The sysfs field was introduced 4 years ago along with fixes to various
drivers that erroneously used `dev_id' for that purpose, but it was not
properly documented anywhere.
See commit v3.14-rc3-739-g3f85944fe207.

Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
 Documentation/ABI/testing/sysfs-class-net | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-net b/Documentation/ABI/testing/sysfs-class-net
index 2f1788111cd9..ec2232f6a949 100644
--- a/Documentation/ABI/testing/sysfs-class-net
+++ b/Documentation/ABI/testing/sysfs-class-net
@@ -91,6 +91,24 @@ Description:
 		stacked (e.g: VLAN interfaces) but still have the same MAC
 		address as their parent device.
 
+What:		/sys/class/net/<iface>/dev_port
+Date:		February 2014
+KernelVersion:	3.15
+Contact:	netdev@vger.kernel.org
+Description:
+		Indicates the port number of this network device, formatted
+		as a decimal value. Some NICs have multiple independent ports
+		on the same PCI bus, device and function. This attribute allows
+		userspace to distinguish the respective interfaces.
+
+		Note: some device drivers started to use 'dev_id' for this
+		purpose since long before 3.15 and have not adopted the new
+		attribute ever since. To query the port number, some tools look
+		exclusively at 'dev_port', while others only consult 'dev_id'.
+		If a network device has multiple client adapter ports as
+		described in the previous paragraph and does not set this
+		attribute to its port number, it's a kernel bug.
+
 What:		/sys/class/net/<iface>/dormant
 Date:		March 2006
 KernelVersion:	2.6.17
-- 
2.19.0.rc2

[PATCH v4 2/3] IB/ipoib: Use dev_port to expose network interface port numbers

[Arseny Maslennikov]

Some InfiniBand network devices have multiple ports on the same PCI
function. This initializes the `dev_port' sysfs field of those
network interfaces with their port number.

Prior to this the kernel erroneously used the `dev_id' sysfs
field of those network interfaces to convey the port number to userspace.

The use of `dev_id' was considered correct until Linux 3.15,
when another field, `dev_port', was defined for this particular
purpose and `dev_id' was reserved for distinguishing stacked ifaces
(e.g: VLANs) with the same hardware address as their parent device.

Similar fixes to net/mlx4_en and many other drivers, which started
exporting this information through `dev_id' before 3.15, were accepted
into the kernel 4 years ago.
See 76a066f2a2a0 (`net/mlx4_en: Expose port number through sysfs').

Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index e3d28f9ad9c0..30f840f874b3 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1880,6 +1880,8 @@ static int ipoib_parent_init(struct net_device *ndev)
 	       sizeof(union ib_gid));
 
 	SET_NETDEV_DEV(priv->dev, priv->ca->dev.parent);
+	priv->dev->dev_port = priv->port - 1;
+	/* Let's set this one too for backwards compatibility. */
 	priv->dev->dev_id = priv->port - 1;
 
 	return 0;
-- 
2.19.0.rc2

[PATCH v4 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Arseny Maslennikov]

Some tools may currently be using only the deprecated attribute;
let's print an elaborate and clear deprecation notice to kmsg.

To do that, we have to replace the whole sysfs file, since we inherit
the original one from netdev.

Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 31 +++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 30f840f874b3..74732726ec6f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2386,6 +2386,35 @@ int ipoib_add_pkey_attr(struct net_device *dev)
 	return device_create_file(&dev->dev, &dev_attr_pkey);
 }
 
+/*
+ * We erroneously exposed the iface's port number in the dev_id
+ * sysfs field long after dev_port was introduced for that purpose[1],
+ * and we need to stop everyone from relying on that.
+ * Let's overload the shower routine for the dev_id file here
+ * to gently bring the issue up.
+ *
+ * [1] https://www.spinics.net/lists/netdev/msg272123.html
+ */
+static ssize_t dev_id_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct net_device *ndev = to_net_dev(dev);
+
+	if (ndev->dev_id == ndev->dev_port)
+		netdev_info_once(ndev,
+			"\"%s\" wants to know my dev_id. Should it look at dev_port instead? See Documentation/ABI/testing/sysfs-class-net for more info.\n",
+			current->comm);
+
+	return sprintf(buf, "%#x\n", ndev->dev_id);
+}
+static DEVICE_ATTR_RO(dev_id);
+
+int ipoib_intercept_dev_id_attr(struct net_device *dev)
+{
+	device_remove_file(&dev->dev, &dev_attr_dev_id);
+	return device_create_file(&dev->dev, &dev_attr_dev_id);
+}
+
 static struct net_device *ipoib_add_port(const char *format,
 					 struct ib_device *hca, u8 port)
 {
@@ -2427,6 +2456,8 @@ static struct net_device *ipoib_add_port(const char *format,
 	 */
 	ndev->priv_destructor = ipoib_intf_free;
 
+	if (ipoib_intercept_dev_id_attr(ndev))
+		goto sysfs_failed;
 	if (ipoib_cm_add_mode_attr(ndev))
 		goto sysfs_failed;
 	if (ipoib_add_pkey_attr(ndev))
-- 
2.19.0.rc2

Re: [PATCH v4 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Jason Gunthorpe]

On Thu, Sep 06, 2018 at 05:51:12PM +0300, Arseny Maslennikov wrote:
> Some tools may currently be using only the deprecated attribute;
> let's print an elaborate and clear deprecation notice to kmsg.
> 
> To do that, we have to replace the whole sysfs file, since we inherit
> the original one from netdev.
> 
> Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
>  drivers/infiniband/ulp/ipoib/ipoib_main.c | 31 +++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 30f840f874b3..74732726ec6f 100644
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -2386,6 +2386,35 @@ int ipoib_add_pkey_attr(struct net_device *dev)
>  	return device_create_file(&dev->dev, &dev_attr_pkey);
>  }
>  
> +/*
> + * We erroneously exposed the iface's port number in the dev_id
> + * sysfs field long after dev_port was introduced for that purpose[1],
> + * and we need to stop everyone from relying on that.
> + * Let's overload the shower routine for the dev_id file here
> + * to gently bring the issue up.
> + *
> + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> + */
> +static ssize_t dev_id_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct net_device *ndev = to_net_dev(dev);
> +
> +	if (ndev->dev_id == ndev->dev_port)
> +		netdev_info_once(ndev,
> +			"\"%s\" wants to know my dev_id. Should it look at dev_port instead? See Documentation/ABI/testing/sysfs-class-net for more info.\n",
> +			current->comm);
> +
> +	return sprintf(buf, "%#x\n", ndev->dev_id);
> +}
> +static DEVICE_ATTR_RO(dev_id);
> +
> +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> +{
> +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> +}

Isn't this racey with userspace? Ie what happens if udev is querying
the dev_id right here?

Do we know there is no userspace doing this?

>  static struct net_device *ipoib_add_port(const char *format,
>  					 struct ib_device *hca, u8 port)
>  {
> @@ -2427,6 +2456,8 @@ static struct net_device *ipoib_add_port(const char *format,
>  	 */
>  	ndev->priv_destructor = ipoib_intf_free;
>  
> +	if (ipoib_intercept_dev_id_attr(ndev))
> +		goto sysfs_failed;

No device_remove_file needed?

Jason

Re: [PATCH v4 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3054 bytes --]

On Fri, 2018-09-07 at 09:43 -0600, Jason Gunthorpe wrote:
> On Thu, Sep 06, 2018 at 05:51:12PM +0300, Arseny Maslennikov wrote:
> > Some tools may currently be using only the deprecated attribute;
> > let's print an elaborate and clear deprecation notice to kmsg.
> > 
> > To do that, we have to replace the whole sysfs file, since we inherit
> > the original one from netdev.
> > 
> > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 31 +++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index 30f840f874b3..74732726ec6f 100644
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -2386,6 +2386,35 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> >  }
> >  
> > +/*
> > + * We erroneously exposed the iface's port number in the dev_id
> > + * sysfs field long after dev_port was introduced for that purpose[1],
> > + * and we need to stop everyone from relying on that.
> > + * Let's overload the shower routine for the dev_id file here
> > + * to gently bring the issue up.
> > + *
> > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > + */
> > +static ssize_t dev_id_show(struct device *dev,
> > +			   struct device_attribute *attr, char *buf)
> > +{
> > +	struct net_device *ndev = to_net_dev(dev);
> > +
> > +	if (ndev->dev_id == ndev->dev_port)
> > +		netdev_info_once(ndev,
> > +			"\"%s\" wants to know my dev_id. Should it look at dev_port instead? See Documentation/ABI/testing/sysfs-class-net for more info.\n",
> > +			current->comm);
> > +
> > +	return sprintf(buf, "%#x\n", ndev->dev_id);
> > +}
> > +static DEVICE_ATTR_RO(dev_id);
> > +
> > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > +{
> > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> > +}
> 
> Isn't this racey with userspace? Ie what happens if udev is querying
> the dev_id right here?
> 
> Do we know there is no userspace doing this?

I don't think that it can race (or reasonably can).  The intercept
function is done as part of ipoib_add_port() so the port itself isn't
live yet.  Things like udev shouldn't be scanning it until after we've
finished bringing it up and added it to the system, so any race here is
unimportant IMO.

> >  static struct net_device *ipoib_add_port(const char *format,
> >  					 struct ib_device *hca, u8 port)
> >  {
> > @@ -2427,6 +2456,8 @@ static struct net_device *ipoib_add_port(const char *format,
> >  	 */
> >  	ndev->priv_destructor = ipoib_intf_free;
> >  
> > +	if (ipoib_intercept_dev_id_attr(ndev))
> > +		goto sysfs_failed;
> 
> No device_remove_file needed?
> 
> Jason

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Jason Gunthorpe]

On Fri, Sep 07, 2018 at 01:14:51PM -0400, Doug Ledford wrote:
> On Fri, 2018-09-07 at 09:43 -0600, Jason Gunthorpe wrote:
> > On Thu, Sep 06, 2018 at 05:51:12PM +0300, Arseny Maslennikov wrote:
> > > Some tools may currently be using only the deprecated attribute;
> > > let's print an elaborate and clear deprecation notice to kmsg.
> > > 
> > > To do that, we have to replace the whole sysfs file, since we inherit
> > > the original one from netdev.
> > > 
> > > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 31 +++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > index 30f840f874b3..74732726ec6f 100644
> > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > @@ -2386,6 +2386,35 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> > >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> > >  }
> > >  
> > > +/*
> > > + * We erroneously exposed the iface's port number in the dev_id
> > > + * sysfs field long after dev_port was introduced for that purpose[1],
> > > + * and we need to stop everyone from relying on that.
> > > + * Let's overload the shower routine for the dev_id file here
> > > + * to gently bring the issue up.
> > > + *
> > > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > > + */
> > > +static ssize_t dev_id_show(struct device *dev,
> > > +			   struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct net_device *ndev = to_net_dev(dev);
> > > +
> > > +	if (ndev->dev_id == ndev->dev_port)
> > > +		netdev_info_once(ndev,
> > > +			"\"%s\" wants to know my dev_id. Should it look at dev_port instead? See Documentation/ABI/testing/sysfs-class-net for more info.\n",
> > > +			current->comm);
> > > +
> > > +	return sprintf(buf, "%#x\n", ndev->dev_id);
> > > +}
> > > +static DEVICE_ATTR_RO(dev_id);
> > > +
> > > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > > +{
> > > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> > > +}
> > 
> > Isn't this racey with userspace? Ie what happens if udev is querying
> > the dev_id right here?
> > 
> > Do we know there is no userspace doing this?
> 
> I don't think that it can race (or reasonably can).  The intercept
> function is done as part of ipoib_add_port() so the port itself isn't
> live yet.

The above code is after register_netdev, so the port itself is
certainly live as far as userspace is concerned..

All the other sysfs stuff in add_port is already wrong/racy.. See
Parav's recent series fixing this for the main devices..

Jason

Re: [PATCH v4 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

On Fri, Sep 07, 2018 at 09:43:59AM -0600, Jason Gunthorpe wrote:
> On Thu, Sep 06, 2018 at 05:51:12PM +0300, Arseny Maslennikov wrote:
> > Some tools may currently be using only the deprecated attribute;
> > let's print an elaborate and clear deprecation notice to kmsg.
> > 
> > To do that, we have to replace the whole sysfs file, since we inherit
> > the original one from netdev.
> > 
> > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 31 +++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index 30f840f874b3..74732726ec6f 100644
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -2386,6 +2386,35 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> >  }
> >  
> > +/*
> > + * We erroneously exposed the iface's port number in the dev_id
> > + * sysfs field long after dev_port was introduced for that purpose[1],
> > + * and we need to stop everyone from relying on that.
> > + * Let's overload the shower routine for the dev_id file here
> > + * to gently bring the issue up.
> > + *
> > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > + */
> > +static ssize_t dev_id_show(struct device *dev,
> > +			   struct device_attribute *attr, char *buf)
> > +{
> > +	struct net_device *ndev = to_net_dev(dev);
> > +
> > +	if (ndev->dev_id == ndev->dev_port)
> > +		netdev_info_once(ndev,
> > +			"\"%s\" wants to know my dev_id. Should it look at dev_port instead? See Documentation/ABI/testing/sysfs-class-net for more info.\n",
> > +			current->comm);
> > +
> > +	return sprintf(buf, "%#x\n", ndev->dev_id);
> > +}
> > +static DEVICE_ATTR_RO(dev_id);
> > +
> > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > +{
> > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> > +}
> 
> Isn't this racey with userspace? Ie what happens if udev is querying
> the dev_id right here?

udev in particular does not use dev_id at all since 2014, because "why
would we keep using dev_id if it is not the right thing to use?".

> 
> Do we know there is no userspace doing this?
> 

Not for sure.

If we move all the sysfs handling stuff we introduce in _add_port():
 - pkey
 - umcast
 - {create,delete}_child
 - connected/datagram mode
to _ndo_init(), which is called by register_netdev before it sends
the netlink message, would that suffice to eliminate the race?
(Sysfs files for {create,delete}_child go to _parent_init() then).

> >  static struct net_device *ipoib_add_port(const char *format,
> >  					 struct ib_device *hca, u8 port)
> >  {
> > @@ -2427,6 +2456,8 @@ static struct net_device *ipoib_add_port(const char *format,
> >  	 */
> >  	ndev->priv_destructor = ipoib_intf_free;
> >  
> > +	if (ipoib_intercept_dev_id_attr(ndev))
> > +		goto sysfs_failed;
> 
> No device_remove_file needed?
> 

As far as I understand, unregister_netdevice takes care of it
while destroying the related kobject.

> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 3764 bytes --]

On Sun, Sep 09, 2018 at 09:11:46PM +0300, Arseny Maslennikov wrote:
> On Fri, Sep 07, 2018 at 09:43:59AM -0600, Jason Gunthorpe wrote:
> > On Thu, Sep 06, 2018 at 05:51:12PM +0300, Arseny Maslennikov wrote:
> > > Some tools may currently be using only the deprecated attribute;
> > > let's print an elaborate and clear deprecation notice to kmsg.
> > > 
> > > To do that, we have to replace the whole sysfs file, since we inherit
> > > the original one from netdev.
> > > 
> > > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 31 +++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > index 30f840f874b3..74732726ec6f 100644
> > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > @@ -2386,6 +2386,35 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> > >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> > >  }
> > >  
> > > +/*
> > > + * We erroneously exposed the iface's port number in the dev_id
> > > + * sysfs field long after dev_port was introduced for that purpose[1],
> > > + * and we need to stop everyone from relying on that.
> > > + * Let's overload the shower routine for the dev_id file here
> > > + * to gently bring the issue up.
> > > + *
> > > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > > + */
> > > +static ssize_t dev_id_show(struct device *dev,
> > > +			   struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct net_device *ndev = to_net_dev(dev);
> > > +
> > > +	if (ndev->dev_id == ndev->dev_port)
> > > +		netdev_info_once(ndev,
> > > +			"\"%s\" wants to know my dev_id. Should it look at dev_port instead? See Documentation/ABI/testing/sysfs-class-net for more info.\n",
> > > +			current->comm);
> > > +
> > > +	return sprintf(buf, "%#x\n", ndev->dev_id);
> > > +}
> > > +static DEVICE_ATTR_RO(dev_id);
> > > +
> > > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > > +{
> > > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> > > +}
> > 
> > Isn't this racey with userspace? Ie what happens if udev is querying
> > the dev_id right here?
> 
> udev in particular does not use dev_id at all since 2014, because "why
> would we keep using dev_id if it is not the right thing to use?".
> 
> > 
> > Do we know there is no userspace doing this?
> > 
> 
> Not for sure.
> 
> If we move all the sysfs handling stuff we introduce in _add_port():
>  - pkey
>  - umcast
>  - {create,delete}_child
>  - connected/datagram mode
> to _ndo_init(), which is called by register_netdev before it sends
> the netlink message, would that suffice to eliminate the race?
> (Sysfs files for {create,delete}_child go to _parent_init() then).
> 

No, we can't, sorry for the noise. ndo_init() runs before the kobject
becomes available.

Anyway, our sysfs attributes being racy is unrelated to the patch series
subject, and I can't come up with any other ideas what to do with them
that do not involve adjustments to register_netdev.

> > >  static struct net_device *ipoib_add_port(const char *format,
> > >  					 struct ib_device *hca, u8 port)
> > >  {
> > > @@ -2427,6 +2456,8 @@ static struct net_device *ipoib_add_port(const char *format,
> > >  	 */
> > >  	ndev->priv_destructor = ipoib_intf_free;
> > >  
> > > +	if (ipoib_intercept_dev_id_attr(ndev))
> > > +		goto sysfs_failed;
> > 
> > No device_remove_file needed?
> > 
> 
> As far as I understand, unregister_netdevice takes care of it
> while destroying the related kobject.
> 
> > Jason



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3657 bytes --]

On Sun, 2018-09-09 at 23:55 +0300, Arseny Maslennikov wrote:
> On Sun, Sep 09, 2018 at 09:11:46PM +0300, Arseny Maslennikov wrote:
> > On Fri, Sep 07, 2018 at 09:43:59AM -0600, Jason Gunthorpe wrote:
> > > On Thu, Sep 06, 2018 at 05:51:12PM +0300, Arseny Maslennikov wrote:
> > > > Some tools may currently be using only the deprecated attribute;
> > > > let's print an elaborate and clear deprecation notice to kmsg.
> > > > 
> > > > To do that, we have to replace the whole sysfs file, since we inherit
> > > > the original one from netdev.
> > > > 
> > > > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > > >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 31 +++++++++++++++++++++++
> > > >  1 file changed, 31 insertions(+)
> > > > 
> > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > index 30f840f874b3..74732726ec6f 100644
> > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > @@ -2386,6 +2386,35 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> > > >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> > > >  }
> > > >  
> > > > +/*
> > > > + * We erroneously exposed the iface's port number in the dev_id
> > > > + * sysfs field long after dev_port was introduced for that purpose[1],
> > > > + * and we need to stop everyone from relying on that.
> > > > + * Let's overload the shower routine for the dev_id file here
> > > > + * to gently bring the issue up.
> > > > + *
> > > > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > > > + */
> > > > +static ssize_t dev_id_show(struct device *dev,
> > > > +			   struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct net_device *ndev = to_net_dev(dev);
> > > > +
> > > > +	if (ndev->dev_id == ndev->dev_port)
> > > > +		netdev_info_once(ndev,
> > > > +			"\"%s\" wants to know my dev_id. Should it look at dev_port instead? See Documentation/ABI/testing/sysfs-class-net for more info.\n",
> > > > +			current->comm);
> > > > +
> > > > +	return sprintf(buf, "%#x\n", ndev->dev_id);
> > > > +}
> > > > +static DEVICE_ATTR_RO(dev_id);
> > > > +
> > > > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > > > +{
> > > > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > > > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> > > > +}
> > > 
> > > Isn't this racey with userspace? Ie what happens if udev is querying
> > > the dev_id right here?
> > 
> > udev in particular does not use dev_id at all since 2014, because "why
> > would we keep using dev_id if it is not the right thing to use?".
> > 
> > > 
> > > Do we know there is no userspace doing this?
> > > 
> > 
> > Not for sure.
> > 
> > If we move all the sysfs handling stuff we introduce in _add_port():
> >  - pkey
> >  - umcast
> >  - {create,delete}_child
> >  - connected/datagram mode
> > to _ndo_init(), which is called by register_netdev before it sends
> > the netlink message, would that suffice to eliminate the race?
> > (Sysfs files for {create,delete}_child go to _parent_init() then).
> > 
> 
> No, we can't, sorry for the noise. ndo_init() runs before the kobject
> becomes available.
> 
> Anyway, our sysfs attributes being racy is unrelated to the patch series
> subject, and I can't come up with any other ideas what to do with them
> that do not involve adjustments to register_netdev.

Agreed (that fixing the race issues is a different patch series).

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 0/3] IB/ipoib: Use dev_port to disambiguate port numbers

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

On Thu, 2018-09-06 at 17:51 +0300, Arseny Maslennikov wrote:
> Pre-3.15 userspace had trouble distinguishing different ports
> of a NIC on a single PCI bus/device/function. To solve this,
> a sysfs field `dev_port' was introduced quite a while ago
> (commit v3.14-rc3-739-g3f85944fe207), and some relevant device
> drivers were fixed to use it, but not in case of IPoIB.
> 
> The convention for some reason never got documented in the kernel, but
> was immediately adopted by userspace (notably udev[1][2], biosdevname[3])
> 
> 1/3 documents the sysfs field — that's why I'm CC-ing netdev.
> 
> This series was tested on and applies to 4.19-rc2.
> 
> [1] https://lists.freedesktop.org/archives/systemd-devel/2014-June/020788.html
> [2] https://lists.freedesktop.org/archives/systemd-devel/2014-July/020804.html
> [3] https://github.com/CloudAutomationNTools/biosdevname/blob/c795d51dd93a5309652f0d635f12a3ecfabfaa72/src/eths.c#L38
> 
> v1->v2: replace a line instead of inserting and then removing.
> v2->v3: restore both attributes, output a notice of deprecation to kmsg.
> v3->v4: style adjustments, join the deprecation notice to single line.
> 
> Arseny Maslennikov (3):
>   Documentation/ABI: document /sys/class/net/*/dev_port
>   IB/ipoib: Use dev_port to expose network interface port numbers
>   IB/ipoib: Log sysfs 'dev_id' accesses from userspace
> 
>  Documentation/ABI/testing/sysfs-class-net | 18 +++++++++++++
>  drivers/infiniband/ulp/ipoib/ipoib_main.c | 33 +++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 

Series applied to for-next.  But I think we should watch feedback from
people, and if people think the notification about using the wrong
variable is too noisy, then we might want to revert it or modify it to
only print out once per specific executable instead of once per run of
each executable.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]