[PATCH 1/2] net: RDMA: explicitly enumerate ib_device attribute groups

[PATCH 1/2] net: RDMA: explicitly enumerate ib_device attribute groups

[Roman Gushchin]

Explicitly enumerate ib_device's attribute groups.

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Maher Sanalla <msanalla@nvidia.com>
Cc: Parav Pandit <parav@mellanox.com>
Cc: linux-rdma@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/infiniband/core/device.c |  4 ++--
 include/rdma/ib_verbs.h          | 14 ++++++++------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 0ded91f056f3..8dea307addf1 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -1404,8 +1404,8 @@ int ib_register_device(struct ib_device *device, const char *name,
 		return ret;
 	}
 
-	device->groups[0] = &ib_dev_attr_group;
-	device->groups[1] = device->ops.device_group;
+	device->groups[IB_ATTR_GROUP_DEV_ATTR] = &ib_dev_attr_group;
+	device->groups[IB_ATTR_GROUP_DRIVER_ATTR] = device->ops.device_group;
 	ret = ib_setup_device_attrs(device);
 	if (ret)
 		goto cache_cleanup;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index b59bf30de430..9c4c4238e6fc 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2728,6 +2728,13 @@ struct ib_core_device {
 	struct ib_device *owner; /* reach back to owner ib_device */
 };
 
+enum ib_device_attr_group {
+	IB_ATTR_GROUP_DEV_ATTR = 0,	/* Device attributes */
+	IB_ATTR_GROUP_DRIVER_ATTR = 1,	/* Driver-provided attributes */
+	IB_ATTR_GROUP_HW_STATS = 2,	/* hw_stats */
+	IB_ATTR_GROUP_LAST,		/* NULL pointer terminating array */
+};
+
 struct rdma_restrack_root;
 struct ib_device {
 	/* Do not access @dma_device directly from ULP nor from HW drivers. */
@@ -2761,12 +2768,7 @@ struct ib_device {
 		struct ib_core_device	coredev;
 	};
 
-	/* First group is for device attributes,
-	 * Second group is for driver provided attributes (optional).
-	 * Third group is for the hw_stats
-	 * It is a NULL terminated array.
-	 */
-	const struct attribute_group	*groups[4];
+	const struct attribute_group	*groups[IB_ATTR_GROUP_LAST + 1];
 
 	u64			     uverbs_cmd_mask;
 
-- 
2.48.1.658.g4767266eb4-goog

[PATCH 2/2] net: RDMA: don't expose hw_stats into non-init net namespaces

[Roman Gushchin]

Commit 467f432a521a ("RDMA/core: Split port and device counter sysfs attributes")
accidentally exposed hw_counters to non-init net namespaces.

Fix this by hiding the IB_ATTR_GROUP_HW_STATS group when initializing
a non-init rdma device.

Fixes: 467f432a521a ("RDMA/core: Split port and device counter sysfs attributes")
Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Maher Sanalla <msanalla@nvidia.com>
Cc: Parav Pandit <parav@mellanox.com>
Cc: linux-rdma@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/infiniband/core/device.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 8dea307addf1..bf4a016ccb9d 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -528,6 +528,8 @@ static struct class ib_class = {
 static void rdma_init_coredev(struct ib_core_device *coredev,
 			      struct ib_device *dev, struct net *net)
 {
+	bool is_full_dev = net_eq(net, &init_net);
+
 	/* This BUILD_BUG_ON is intended to catch layout change
 	 * of union of ib_core_device and device.
 	 * dev must be the first element as ib_core and providers
@@ -539,6 +541,10 @@ static void rdma_init_coredev(struct ib_core_device *coredev,
 
 	coredev->dev.class = &ib_class;
 	coredev->dev.groups = dev->groups;
+
+	if (!is_full_dev)
+		coredev->dev.groups[IB_ATTR_GROUP_HW_STATS] = NULL;
+
 	device_initialize(&coredev->dev);
 	coredev->owner = dev;
 	INIT_LIST_HEAD(&coredev->port_list);
-- 
2.48.1.658.g4767266eb4-goog

RE: [PATCH 1/2] net: RDMA: explicitly enumerate ib_device attribute groups

[Parav Pandit]

> From: Roman Gushchin <roman.gushchin@linux.dev>
> Sent: Wednesday, February 26, 2025 9:05 AM
> 
> Explicitly enumerate ib_device's attribute groups.
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Maher Sanalla <msanalla@nvidia.com>
> Cc: Parav Pandit <parav@mellanox.com>
> Cc: linux-rdma@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/infiniband/core/device.c |  4 ++--
>  include/rdma/ib_verbs.h          | 14 ++++++++------
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 0ded91f056f3..8dea307addf1 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1404,8 +1404,8 @@ int ib_register_device(struct ib_device *device,
> const char *name,
>  		return ret;
>  	}
> 
> -	device->groups[0] = &ib_dev_attr_group;
> -	device->groups[1] = device->ops.device_group;
> +	device->groups[IB_ATTR_GROUP_DEV_ATTR] = &ib_dev_attr_group;
> +	device->groups[IB_ATTR_GROUP_DRIVER_ATTR] = device-
> >ops.device_group;
>  	ret = ib_setup_device_attrs(device);
This function may initialize the hw_stats at the index IB_ATTR_GROUP_DRIVER_ATTR when it is optional.
It runs through a for loop to find the first free entry.
This needs to be captured in the comment above, and it will have effect on your next fix patch.

>  	if (ret)
>  		goto cache_cleanup;
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index
> b59bf30de430..9c4c4238e6fc 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -2728,6 +2728,13 @@ struct ib_core_device {
>  	struct ib_device *owner; /* reach back to owner ib_device */  };
> 
> +enum ib_device_attr_group {
> +	IB_ATTR_GROUP_DEV_ATTR = 0,	/* Device attributes */
> +	IB_ATTR_GROUP_DRIVER_ATTR = 1,	/* Driver-provided attributes
> */
> +	IB_ATTR_GROUP_HW_STATS = 2,	/* hw_stats */
> +	IB_ATTR_GROUP_LAST,		/* NULL pointer terminating array */
> +};
> +
>  struct rdma_restrack_root;
>  struct ib_device {
>  	/* Do not access @dma_device directly from ULP nor from HW
> drivers. */ @@ -2761,12 +2768,7 @@ struct ib_device {
>  		struct ib_core_device	coredev;
>  	};
> 
> -	/* First group is for device attributes,
> -	 * Second group is for driver provided attributes (optional).
This text 'optional' is missing in above enums, please add it.
Also please add the optional text comment for hw_stats too, as its optional too.

> -	 * Third group is for the hw_stats
> -	 * It is a NULL terminated array.
> -	 */
> -	const struct attribute_group	*groups[4];
> +	const struct attribute_group	*groups[IB_ATTR_GROUP_LAST + 1];
> 
>  	u64			     uverbs_cmd_mask;
> 
> --
> 2.48.1.658.g4767266eb4-goog

RE: [PATCH 2/2] net: RDMA: don't expose hw_stats into non-init net namespaces

[Parav Pandit]

> From: Roman Gushchin <roman.gushchin@linux.dev>
> Sent: Wednesday, February 26, 2025 9:05 AM
> 
> Commit 467f432a521a ("RDMA/core: Split port and device counter sysfs
> attributes") accidentally exposed hw_counters to non-init net namespaces.
> 
> Fix this by hiding the IB_ATTR_GROUP_HW_STATS group when initializing a
> non-init rdma device.
> 
> Fixes: 467f432a521a ("RDMA/core: Split port and device counter sysfs
> attributes")
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Maher Sanalla <msanalla@nvidia.com>
> Cc: Parav Pandit <parav@mellanox.com>
> Cc: linux-rdma@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/infiniband/core/device.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 8dea307addf1..bf4a016ccb9d 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -528,6 +528,8 @@ static struct class ib_class = {  static void
> rdma_init_coredev(struct ib_core_device *coredev,
>  			      struct ib_device *dev, struct net *net)  {
> +	bool is_full_dev = net_eq(net, &init_net);
> +
Instead of it, below is more elegant check because 
a. its limited to do comparison on core dev area and other generic structure.
b. reuses the infra used in sysfs.c to detect coredev.
c. in upcoming future, I plan to expand device creation in non init ns too, where it still will be coredev.
And this conficts with that idea, hinting that comparing in below manner is still more elegant.

bool is_full_dev = &device->coredev == coredev; 

>  	/* This BUILD_BUG_ON is intended to catch layout change
>  	 * of union of ib_core_device and device.
>  	 * dev must be the first element as ib_core and providers @@ -539,6
> +541,10 @@ static void rdma_init_coredev(struct ib_core_device *coredev,
> 
>  	coredev->dev.class = &ib_class;
>  	coredev->dev.groups = dev->groups;
> +
> +	if (!is_full_dev)
> +		coredev->dev.groups[IB_ATTR_GROUP_HW_STATS] = NULL;
> +
Static hard coding to HW_STATS enum is not enough because when IB_ATTR_GROUP_DRIVER_ATTR group is not used, 
ib_setup_device_attrs() will initialize HW_STATS at index = DRIVER_ATTR.
So you need to store the index of hw_stats group in the ib_device struct, so you can make it null for non_core_dev.

And if that can be done without introducing the new enums, applying the fix on previous stable kernels will be easy.
Otherwise, this patch needs to do to -rc and other previous patch to -next and its difficult.

>  	device_initialize(&coredev->dev);
>  	coredev->owner = dev;
>  	INIT_LIST_HEAD(&coredev->port_list);
> --
> 2.48.1.658.g4767266eb4-goog

Re: [PATCH 1/2] net: RDMA: explicitly enumerate ib_device attribute groups

[Leon Romanovsky]

On Wed, Feb 26, 2025 at 03:35:25AM +0000, Roman Gushchin wrote:
> Explicitly enumerate ib_device's attribute groups.
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Maher Sanalla <msanalla@nvidia.com>
> Cc: Parav Pandit <parav@mellanox.com>
> Cc: linux-rdma@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/infiniband/core/device.c |  4 ++--
>  include/rdma/ib_verbs.h          | 14 ++++++++------
>  2 files changed, 10 insertions(+), 8 deletions(-)

Very minor comment as you will need to address Parav's comments anyway.
The title should be "RDMA/core: Explicitly ..." and not as it is written.

Thanks