[PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Wenjia Zhang]

Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
alternative to get_netdev") introduced an API ib_device_get_netdev.
The SMC-R variant of the SMC protocol continued to use the old API
ib_device_ops.get_netdev() to lookup netdev. As this commit 8d159eb2117b
("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
ib_device_ops.get_netdev didn't work any more at least by using a mlx5
device driver. Thus, using ib_device_set_netdev() now became mandatory.

Replace ib_device_ops.get_netdev() with ib_device_get_netdev().

Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
Reported-by: Aswin K <aswin@linux.ibm.com>
Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
---
 net/smc/smc_ib.c   | 8 ++------
 net/smc/smc_pnet.c | 4 +---
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
index 9297dc20bfe2..9c563cdbea90 100644
--- a/net/smc/smc_ib.c
+++ b/net/smc/smc_ib.c
@@ -899,9 +899,7 @@ static void smc_copy_netdev_ifindex(struct smc_ib_device *smcibdev, int port)
 	struct ib_device *ibdev = smcibdev->ibdev;
 	struct net_device *ndev;
 
-	if (!ibdev->ops.get_netdev)
-		return;
-	ndev = ibdev->ops.get_netdev(ibdev, port + 1);
+	ndev = ib_device_get_netdev(ibdev, port + 1);
 	if (ndev) {
 		smcibdev->ndev_ifidx[port] = ndev->ifindex;
 		dev_put(ndev);
@@ -921,9 +919,7 @@ void smc_ib_ndev_change(struct net_device *ndev, unsigned long event)
 		port_cnt = smcibdev->ibdev->phys_port_cnt;
 		for (i = 0; i < min_t(size_t, port_cnt, SMC_MAX_PORTS); i++) {
 			libdev = smcibdev->ibdev;
-			if (!libdev->ops.get_netdev)
-				continue;
-			lndev = libdev->ops.get_netdev(libdev, i + 1);
+			lndev = ib_device_get_netdev(libdev, i + 1);
 			dev_put(lndev);
 			if (lndev != ndev)
 				continue;
diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
index 1dd362326c0a..8566937c8903 100644
--- a/net/smc/smc_pnet.c
+++ b/net/smc/smc_pnet.c
@@ -1054,9 +1054,7 @@ static void smc_pnet_find_rdma_dev(struct net_device *netdev,
 		for (i = 1; i <= SMC_MAX_PORTS; i++) {
 			if (!rdma_is_port_valid(ibdev->ibdev, i))
 				continue;
-			if (!ibdev->ibdev->ops.get_netdev)
-				continue;
-			ndev = ibdev->ibdev->ops.get_netdev(ibdev->ibdev, i);
+			ndev = ib_device_get_netdev(ibdev->ibdev, i);
 			if (!ndev)
 				continue;
 			dev_put(ndev);
-- 
2.43.0

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Halil Pasic]

On Fri, 25 Oct 2024 09:23:55 +0200
Wenjia Zhang <wenjia@linux.ibm.com> wrote:

> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> alternative to get_netdev") introduced an API ib_device_get_netdev.
> The SMC-R variant of the SMC protocol continued to use the old API
> ib_device_ops.get_netdev() to lookup netdev. As this commit 8d159eb2117b
> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> device driver. Thus, using ib_device_set_netdev() now became mandatory.
> 
> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> 
> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> Reported-by: Aswin K <aswin@linux.ibm.com>
> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Simon Horman]

On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:
> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> alternative to get_netdev") introduced an API ib_device_get_netdev.
> The SMC-R variant of the SMC protocol continued to use the old API
> ib_device_ops.get_netdev() to lookup netdev. As this commit 8d159eb2117b
> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> device driver. Thus, using ib_device_set_netdev() now became mandatory.
> 
> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> 
> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> Reported-by: Aswin K <aswin@linux.ibm.com>
> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Dust Li]

On 2024-10-25 09:23:55, Wenjia Zhang wrote:
>Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
>alternative to get_netdev") introduced an API ib_device_get_netdev.
>The SMC-R variant of the SMC protocol continued to use the old API
>ib_device_ops.get_netdev() to lookup netdev. As this commit 8d159eb2117b
>("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
>get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
>ib_device_ops.get_netdev didn't work any more at least by using a mlx5
>device driver. Thus, using ib_device_set_netdev() now became mandatory.
>
>Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
>
>Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
>Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
>Reported-by: Aswin K <aswin@linux.ibm.com>
>Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
>Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>

Reviewed-by: Dust Li <dust.li@linux.alibaba.com>

>---
> net/smc/smc_ib.c   | 8 ++------
> net/smc/smc_pnet.c | 4 +---
> 2 files changed, 3 insertions(+), 9 deletions(-)
>
>diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
>index 9297dc20bfe2..9c563cdbea90 100644
>--- a/net/smc/smc_ib.c
>+++ b/net/smc/smc_ib.c
>@@ -899,9 +899,7 @@ static void smc_copy_netdev_ifindex(struct smc_ib_device *smcibdev, int port)
> 	struct ib_device *ibdev = smcibdev->ibdev;
> 	struct net_device *ndev;
> 
>-	if (!ibdev->ops.get_netdev)
>-		return;
>-	ndev = ibdev->ops.get_netdev(ibdev, port + 1);
>+	ndev = ib_device_get_netdev(ibdev, port + 1);
> 	if (ndev) {
> 		smcibdev->ndev_ifidx[port] = ndev->ifindex;
> 		dev_put(ndev);
>@@ -921,9 +919,7 @@ void smc_ib_ndev_change(struct net_device *ndev, unsigned long event)
> 		port_cnt = smcibdev->ibdev->phys_port_cnt;
> 		for (i = 0; i < min_t(size_t, port_cnt, SMC_MAX_PORTS); i++) {
> 			libdev = smcibdev->ibdev;
>-			if (!libdev->ops.get_netdev)
>-				continue;
>-			lndev = libdev->ops.get_netdev(libdev, i + 1);
>+			lndev = ib_device_get_netdev(libdev, i + 1);
> 			dev_put(lndev);
> 			if (lndev != ndev)
> 				continue;
>diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
>index 1dd362326c0a..8566937c8903 100644
>--- a/net/smc/smc_pnet.c
>+++ b/net/smc/smc_pnet.c
>@@ -1054,9 +1054,7 @@ static void smc_pnet_find_rdma_dev(struct net_device *netdev,
> 		for (i = 1; i <= SMC_MAX_PORTS; i++) {
> 			if (!rdma_is_port_valid(ibdev->ibdev, i))
> 				continue;
>-			if (!ibdev->ibdev->ops.get_netdev)
>-				continue;
>-			ndev = ibdev->ibdev->ops.get_netdev(ibdev->ibdev, i);
>+			ndev = ib_device_get_netdev(ibdev->ibdev, i);
> 			if (!ndev)
> 				continue;
> 			dev_put(ndev);
>-- 
>2.43.0
>

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Wen Gu]

On 2024/10/25 15:23, Wenjia Zhang wrote:
> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> alternative to get_netdev") introduced an API ib_device_get_netdev.
> The SMC-R variant of the SMC protocol continued to use the old API
> ib_device_ops.get_netdev() to lookup netdev. As this commit 8d159eb2117b
> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> device driver. Thus, using ib_device_set_netdev() now became mandatory.
> 
> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> 
> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> Reported-by: Aswin K <aswin@linux.ibm.com>
> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>

LGTM!

Reviewed-by: Wen Gu <guwen@linux.alibaba.com>

> ---
>   net/smc/smc_ib.c   | 8 ++------
>   net/smc/smc_pnet.c | 4 +---
>   2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> index 9297dc20bfe2..9c563cdbea90 100644
> --- a/net/smc/smc_ib.c
> +++ b/net/smc/smc_ib.c
> @@ -899,9 +899,7 @@ static void smc_copy_netdev_ifindex(struct smc_ib_device *smcibdev, int port)
>   	struct ib_device *ibdev = smcibdev->ibdev;
>   	struct net_device *ndev;
>   
> -	if (!ibdev->ops.get_netdev)
> -		return;
> -	ndev = ibdev->ops.get_netdev(ibdev, port + 1);
> +	ndev = ib_device_get_netdev(ibdev, port + 1);
>   	if (ndev) {
>   		smcibdev->ndev_ifidx[port] = ndev->ifindex;
>   		dev_put(ndev);
> @@ -921,9 +919,7 @@ void smc_ib_ndev_change(struct net_device *ndev, unsigned long event)
>   		port_cnt = smcibdev->ibdev->phys_port_cnt;
>   		for (i = 0; i < min_t(size_t, port_cnt, SMC_MAX_PORTS); i++) {
>   			libdev = smcibdev->ibdev;
> -			if (!libdev->ops.get_netdev)
> -				continue;
> -			lndev = libdev->ops.get_netdev(libdev, i + 1);
> +			lndev = ib_device_get_netdev(libdev, i + 1);
>   			dev_put(lndev);
>   			if (lndev != ndev)
>   				continue;
> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
> index 1dd362326c0a..8566937c8903 100644
> --- a/net/smc/smc_pnet.c
> +++ b/net/smc/smc_pnet.c
> @@ -1054,9 +1054,7 @@ static void smc_pnet_find_rdma_dev(struct net_device *netdev,
>   		for (i = 1; i <= SMC_MAX_PORTS; i++) {
>   			if (!rdma_is_port_valid(ibdev->ibdev, i))
>   				continue;
> -			if (!ibdev->ibdev->ops.get_netdev)
> -				continue;
> -			ndev = ibdev->ibdev->ops.get_netdev(ibdev->ibdev, i);
> +			ndev = ib_device_get_netdev(ibdev->ibdev, i);
>   			if (!ndev)
>   				continue;
>   			dev_put(ndev);

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Zhu Yanjun]

在 2024/10/25 9:23, Wenjia Zhang 写道:
> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> alternative to get_netdev") introduced an API ib_device_get_netdev.
> The SMC-R variant of the SMC protocol continued to use the old API
> ib_device_ops.get_netdev() to lookup netdev. As this commit 8d159eb2117b
> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling

Thanks a lot.

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Because the commit 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and 
get_netdev functions") removes the get_netdev callback from 
mlx5_ib_dev_common_roce_ops, in mlx4, get_netdev is still in 
mlx4_ib_dev_ops. So the following commit will follow mlx5 to remove 
get_netdev from mlx4 driver.

 From a59f2e01428640a332a51b8d910ec166704aa441 Mon Sep 17 00:00:00 2001
From: Zhu Yanjun <yanjun.zhu@linux.dev>
Date: Sun, 27 Oct 2024 20:21:27 +0100
Subject: [PATCH 1/1] RDMA/mlx4: Use IB get_netdev functions and remove
  get_netdev callback

In the commit 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev
functions") removed the get_netdev callback from
mlx5_ib_dev_common_roce_ops, in mlx4, get_netdev callback should also
be removed.

Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
compile successfully only
---
  drivers/infiniband/hw/mlx4/main.c | 35 -------------------------------
  1 file changed, 35 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c 
b/drivers/infiniband/hw/mlx4/main.c
index 529db874d67c..cf34d92de7b1 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -123,40 +123,6 @@ static int num_ib_ports(struct mlx4_dev *dev)
         return ib_ports;
  }

-static struct net_device *mlx4_ib_get_netdev(struct ib_device *device,
-                                            u32 port_num)
-{
-       struct mlx4_ib_dev *ibdev = to_mdev(device);
-       struct net_device *dev, *ret = NULL;
-
-       rcu_read_lock();
-       for_each_netdev_rcu(&init_net, dev) {
-               if (dev->dev.parent != ibdev->ib_dev.dev.parent ||
-                   dev->dev_port + 1 != port_num)
-                       continue;
-
-               if (mlx4_is_bonded(ibdev->dev)) {
-                       struct net_device *upper;
-
-                       upper = netdev_master_upper_dev_get_rcu(dev);
-                       if (upper) {
-                               struct net_device *active;
-
-                               active = 
bond_option_active_slave_get_rcu(netdev_priv(upper));
-                               if (active)
-                                       dev = active;
-                       }
-               }
-
-               dev_hold(dev);
-               ret = dev;
-               break;
-       }
-
-       rcu_read_unlock();
-       return ret;
-}
-
  static int mlx4_ib_update_gids_v1(struct gid_entry *gids,
                                   struct mlx4_ib_dev *ibdev,
                                   u32 port_num)
@@ -2544,7 +2510,6 @@ static const struct ib_device_ops mlx4_ib_dev_ops = {
         .get_dev_fw_str = get_fw_ver_str,
         .get_dma_mr = mlx4_ib_get_dma_mr,
         .get_link_layer = mlx4_ib_port_link_layer,
-       .get_netdev = mlx4_ib_get_netdev,
         .get_port_immutable = mlx4_port_immutable,
         .map_mr_sg = mlx4_ib_map_mr_sg,
         .mmap = mlx4_ib_mmap,
--
2.34.1

Zhu Yanjun

> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> device driver. Thus, using ib_device_set_netdev() now became mandatory.
> 
> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> 
> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> Reported-by: Aswin K <aswin@linux.ibm.com>
> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
> ---
>   net/smc/smc_ib.c   | 8 ++------
>   net/smc/smc_pnet.c | 4 +---
>   2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> index 9297dc20bfe2..9c563cdbea90 100644
> --- a/net/smc/smc_ib.c
> +++ b/net/smc/smc_ib.c
> @@ -899,9 +899,7 @@ static void smc_copy_netdev_ifindex(struct smc_ib_device *smcibdev, int port)
>   	struct ib_device *ibdev = smcibdev->ibdev;
>   	struct net_device *ndev;
>   
> -	if (!ibdev->ops.get_netdev)
> -		return;
> -	ndev = ibdev->ops.get_netdev(ibdev, port + 1);
> +	ndev = ib_device_get_netdev(ibdev, port + 1);
>   	if (ndev) {
>   		smcibdev->ndev_ifidx[port] = ndev->ifindex;
>   		dev_put(ndev);
> @@ -921,9 +919,7 @@ void smc_ib_ndev_change(struct net_device *ndev, unsigned long event)
>   		port_cnt = smcibdev->ibdev->phys_port_cnt;
>   		for (i = 0; i < min_t(size_t, port_cnt, SMC_MAX_PORTS); i++) {
>   			libdev = smcibdev->ibdev;
> -			if (!libdev->ops.get_netdev)
> -				continue;
> -			lndev = libdev->ops.get_netdev(libdev, i + 1);
> +			lndev = ib_device_get_netdev(libdev, i + 1);
>   			dev_put(lndev);
>   			if (lndev != ndev)
>   				continue;
> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
> index 1dd362326c0a..8566937c8903 100644
> --- a/net/smc/smc_pnet.c
> +++ b/net/smc/smc_pnet.c
> @@ -1054,9 +1054,7 @@ static void smc_pnet_find_rdma_dev(struct net_device *netdev,
>   		for (i = 1; i <= SMC_MAX_PORTS; i++) {
>   			if (!rdma_is_port_valid(ibdev->ibdev, i))
>   				continue;
> -			if (!ibdev->ibdev->ops.get_netdev)
> -				continue;
> -			ndev = ibdev->ibdev->ops.get_netdev(ibdev->ibdev, i);
> +			ndev = ib_device_get_netdev(ibdev->ibdev, i);
>   			if (!ndev)
>   				continue;
>   			dev_put(ndev);

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:
> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> alternative to get_netdev") introduced an API ib_device_get_netdev.
> The SMC-R variant of the SMC protocol continued to use the old API
> ib_device_ops.get_netdev() to lookup netdev. 

I would say that calls to ibdev ops from ULPs was never been right
thing to do. The ib_device_set_netdev() was introduced for the drivers.

So the whole commit message is not accurate and better to be rewritten.

> As this commit 8d159eb2117b
> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> device driver.

It is not a correct statement too. All modern drivers (for last 5 years)
don't have that .get_netdev() ops, so it is not mlx5 specific, but another
justification to say that SMC-R was doing it wrong.

> Thus, using ib_device_set_netdev() now became mandatory.

ib_device_set_netdev() is mandatory for the drivers, it is nothing to do
with ULPs.

> 
> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().

It is too late for me to do proper review for today, but I would say
that it is worth to pay attention to multiple dev_put() calls in the
functions around the ib_device_get_netdev().

> 
> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")

It is not related to this change Fixes line.

> Reported-by: Aswin K <aswin@linux.ibm.com>
> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
> ---
>  net/smc/smc_ib.c   | 8 ++------
>  net/smc/smc_pnet.c | 4 +---
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> index 9297dc20bfe2..9c563cdbea90 100644
> --- a/net/smc/smc_ib.c
> +++ b/net/smc/smc_ib.c
> @@ -899,9 +899,7 @@ static void smc_copy_netdev_ifindex(struct smc_ib_device *smcibdev, int port)
>  	struct ib_device *ibdev = smcibdev->ibdev;
>  	struct net_device *ndev;
>  
> -	if (!ibdev->ops.get_netdev)
> -		return;
> -	ndev = ibdev->ops.get_netdev(ibdev, port + 1);
> +	ndev = ib_device_get_netdev(ibdev, port + 1);
>  	if (ndev) {
>  		smcibdev->ndev_ifidx[port] = ndev->ifindex;
>  		dev_put(ndev);
> @@ -921,9 +919,7 @@ void smc_ib_ndev_change(struct net_device *ndev, unsigned long event)
>  		port_cnt = smcibdev->ibdev->phys_port_cnt;
>  		for (i = 0; i < min_t(size_t, port_cnt, SMC_MAX_PORTS); i++) {
>  			libdev = smcibdev->ibdev;
> -			if (!libdev->ops.get_netdev)
> -				continue;
> -			lndev = libdev->ops.get_netdev(libdev, i + 1);
> +			lndev = ib_device_get_netdev(libdev, i + 1);
>  			dev_put(lndev);
>  			if (lndev != ndev)
>  				continue;
> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
> index 1dd362326c0a..8566937c8903 100644
> --- a/net/smc/smc_pnet.c
> +++ b/net/smc/smc_pnet.c
> @@ -1054,9 +1054,7 @@ static void smc_pnet_find_rdma_dev(struct net_device *netdev,
>  		for (i = 1; i <= SMC_MAX_PORTS; i++) {
>  			if (!rdma_is_port_valid(ibdev->ibdev, i))
>  				continue;
> -			if (!ibdev->ibdev->ops.get_netdev)
> -				continue;
> -			ndev = ibdev->ibdev->ops.get_netdev(ibdev->ibdev, i);
> +			ndev = ib_device_get_netdev(ibdev->ibdev, i);
>  			if (!ndev)
>  				continue;
>  			dev_put(ndev);
> -- 
> 2.43.0
> 
> 

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Sun, Oct 27, 2024 at 10:18:57PM +0200, Leon Romanovsky wrote:
> On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:
> > Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> > alternative to get_netdev") introduced an API ib_device_get_netdev.
> > The SMC-R variant of the SMC protocol continued to use the old API
> > ib_device_ops.get_netdev() to lookup netdev. 
> 
> I would say that calls to ibdev ops from ULPs was never been right
> thing to do. The ib_device_set_netdev() was introduced for the drivers.
> 
> So the whole commit message is not accurate and better to be rewritten.
> 
> > As this commit 8d159eb2117b
> > ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> > get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
> > ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> > device driver.
> 
> It is not a correct statement too. All modern drivers (for last 5 years)
> don't have that .get_netdev() ops, so it is not mlx5 specific, but another
> justification to say that SMC-R was doing it wrong.
> 
> > Thus, using ib_device_set_netdev() now became mandatory.
> 
> ib_device_set_netdev() is mandatory for the drivers, it is nothing to do
> with ULPs.
> 
> > 
> > Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> 
> It is too late for me to do proper review for today, but I would say
> that it is worth to pay attention to multiple dev_put() calls in the
> functions around the ib_device_get_netdev().
> 
> > 
> > Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")

Honestly, this patch in Fixes line doesn't look right to me. It pokes inside
of ib_device to get netdev index. For example call to smc_ib_ndev_change()
will return completely unpredictable results, due to races.

It is bad that RDMA ML wasn't even CCed back then, we would say NAK to
this patch.
https://lore.kernel.org/netdev/20201201192049.53517-6-kgraul@linux.ibm.com/

Thanks

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[D. Wythe]

On 10/25/24 3:23 PM, Wenjia Zhang wrote:
> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> alternative to get_netdev") introduced an API ib_device_get_netdev.
> The SMC-R variant of the SMC protocol continued to use the old API
> ib_device_ops.get_netdev() to lookup netdev. As this commit 8d159eb2117b
> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> device driver. Thus, using ib_device_set_netdev() now became mandatory.
> 
> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> 
> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> Reported-by: Aswin K <aswin@linux.ibm.com>
> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
> ---
>   net/smc/smc_ib.c   | 8 ++------
>   net/smc/smc_pnet.c | 4 +---
>   2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> index 9297dc20bfe2..9c563cdbea90 100644
> --- a/net/smc/smc_ib.c
> +++ b/net/smc/smc_ib.c
> @@ -899,9 +899,7 @@ static void smc_copy_netdev_ifindex(struct smc_ib_device *smcibdev, int port)
>   	struct ib_device *ibdev = smcibdev->ibdev;
>   	struct net_device *ndev;
>   
> -	if (!ibdev->ops.get_netdev)
> -		return;
> -	ndev = ibdev->ops.get_netdev(ibdev, port + 1);
> +	ndev = ib_device_get_netdev(ibdev, port + 1);
>   	if (ndev) {
>   		smcibdev->ndev_ifidx[port] = ndev->ifindex;
>   		dev_put(ndev);
> @@ -921,9 +919,7 @@ void smc_ib_ndev_change(struct net_device *ndev, unsigned long event)
>   		port_cnt = smcibdev->ibdev->phys_port_cnt;
>   		for (i = 0; i < min_t(size_t, port_cnt, SMC_MAX_PORTS); i++) {
>   			libdev = smcibdev->ibdev;
> -			if (!libdev->ops.get_netdev)
> -				continue;
> -			lndev = libdev->ops.get_netdev(libdev, i + 1);
> +			lndev = ib_device_get_netdev(libdev, i + 1);
>   			dev_put(lndev);
>   			if (lndev != ndev)
>   				continue;
> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
> index 1dd362326c0a..8566937c8903 100644
> --- a/net/smc/smc_pnet.c
> +++ b/net/smc/smc_pnet.c
> @@ -1054,9 +1054,7 @@ static void smc_pnet_find_rdma_dev(struct net_device *netdev,
>   		for (i = 1; i <= SMC_MAX_PORTS; i++) {
>   			if (!rdma_is_port_valid(ibdev->ibdev, i))
>   				continue;
> -			if (!ibdev->ibdev->ops.get_netdev)
> -				continue;
> -			ndev = ibdev->ibdev->ops.get_netdev(ibdev->ibdev, i);
> +			ndev = ib_device_get_netdev(ibdev->ibdev, i);
>   			if (!ndev)
>   				continue;
>   			dev_put(ndev);


Reviewed-by: D. Wythe <alibuda@linux.alibaba.com>

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Paolo Abeni]

On 10/25/24 09:23, Wenjia Zhang wrote:
> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> alternative to get_netdev") introduced an API ib_device_get_netdev.
> The SMC-R variant of the SMC protocol continued to use the old API
> ib_device_ops.get_netdev() to lookup netdev. As this commit 8d159eb2117b
> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> device driver. Thus, using ib_device_set_netdev() now became mandatory.
> 
> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> 
> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> Reported-by: Aswin K <aswin@linux.ibm.com>
> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>

Please adjust the commit message as per Leon suggestion. You can retain
all the ack collected so far.

Thanks,

Paolo

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Wenjia Zhang]

On 27.10.24 21:18, Leon Romanovsky wrote:
> On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:
>> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
>> alternative to get_netdev") introduced an API ib_device_get_netdev.
>> The SMC-R variant of the SMC protocol continued to use the old API
>> ib_device_ops.get_netdev() to lookup netdev.
> 
> I would say that calls to ibdev ops from ULPs was never been right
> thing to do. The ib_device_set_netdev() was introduced for the drivers.
> 
> So the whole commit message is not accurate and better to be rewritten.
> 
>> As this commit 8d159eb2117b
>> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
>> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
>> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
>> device driver.
> 
> It is not a correct statement too. All modern drivers (for last 5 years)
> don't have that .get_netdev() ops, so it is not mlx5 specific, but another
> justification to say that SMC-R was doing it wrong.
> 
>> Thus, using ib_device_set_netdev() now became mandatory.
> 
> ib_device_set_netdev() is mandatory for the drivers, it is nothing to do
> with ULPs.
> 
>>
>> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> 
> It is too late for me to do proper review for today, but I would say
> that it is worth to pay attention to multiple dev_put() calls in the
> functions around the ib_device_get_netdev().
> 
>>
>> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
>> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> 
> It is not related to this change Fixes line.
> 

Hi Leon,

Thank you for the review! I agree that SMC could do better. However, we 
should fix it and give enough information and reference on the changes, 
since the code has already existed and didn't work with the old way. I 
can rewrite the commit message.

What about:
"
The SMC-R variant of the SMC protocol still called 
ib_device_ops.get_netdev() to lookup netdev. As we used mlx5 device 
driver to run SMC-R, it failed to find a device, because in mlx5_ib the 
internal net device management for retrieving net devices was replaced 
by a common interface ib_device_get_netdev() in commit 8d159eb2117b 
("RDMA/mlx5: Use IB set_netdev and get_netdev functions"). Thus, replace 
ib_device_ops.get_netdev() with ib_device_get_netdev() in SMC.
"

Thanks,
Wenjia

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Wenjia Zhang]

On 31.10.24 11:01, Paolo Abeni wrote:
> On 10/25/24 09:23, Wenjia Zhang wrote:
>> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
>> alternative to get_netdev") introduced an API ib_device_get_netdev.
>> The SMC-R variant of the SMC protocol continued to use the old API
>> ib_device_ops.get_netdev() to lookup netdev. As this commit 8d159eb2117b
>> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
>> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
>> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
>> device driver. Thus, using ib_device_set_netdev() now became mandatory.
>>
>> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
>>
>> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
>> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
>> Reported-by: Aswin K <aswin@linux.ibm.com>
>> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
>> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
> 
> Please adjust the commit message as per Leon suggestion. You can retain
> all the ack collected so far.
> 
> Thanks,
> 
> Paolo
> 
Hi Paolo,

thank you for the reminder!
Sure, I'll do it.

Thanks,
Wenjia

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Tue, Nov 05, 2024 at 10:50:45AM +0100, Wenjia Zhang wrote:
> 
> 
> On 27.10.24 21:18, Leon Romanovsky wrote:
> > On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:
> > > Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> > > alternative to get_netdev") introduced an API ib_device_get_netdev.
> > > The SMC-R variant of the SMC protocol continued to use the old API
> > > ib_device_ops.get_netdev() to lookup netdev.
> > 
> > I would say that calls to ibdev ops from ULPs was never been right
> > thing to do. The ib_device_set_netdev() was introduced for the drivers.
> > 
> > So the whole commit message is not accurate and better to be rewritten.
> > 
> > > As this commit 8d159eb2117b
> > > ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> > > get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
> > > ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> > > device driver.
> > 
> > It is not a correct statement too. All modern drivers (for last 5 years)
> > don't have that .get_netdev() ops, so it is not mlx5 specific, but another
> > justification to say that SMC-R was doing it wrong.
> > 
> > > Thus, using ib_device_set_netdev() now became mandatory.
> > 
> > ib_device_set_netdev() is mandatory for the drivers, it is nothing to do
> > with ULPs.
> > 
> > > 
> > > Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> > 
> > It is too late for me to do proper review for today, but I would say
> > that it is worth to pay attention to multiple dev_put() calls in the
> > functions around the ib_device_get_netdev().
> > 
> > > 
> > > Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> > > Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> > 
> > It is not related to this change Fixes line.
> > 
> 
> Hi Leon,
> 
> Thank you for the review! I agree that SMC could do better. However, we
> should fix it and give enough information and reference on the changes,
> since the code has already existed and didn't work with the old way. 

The code which you change worked by chance and was wrong from day one.

> I can rewrite the commit message.
> 
> What about:
> "
> The SMC-R variant of the SMC protocol still called
> ib_device_ops.get_netdev() to lookup netdev. As we used mlx5 device driver
> to run SMC-R, it failed to find a device, because in mlx5_ib the internal
> net device management for retrieving net devices was replaced by a common
> interface ib_device_get_netdev() in commit 8d159eb2117b ("RDMA/mlx5: Use IB
> set_netdev and get_netdev functions"). Thus, replace
> ib_device_ops.get_netdev() with ib_device_get_netdev() in SMC.
> "

 The SMC-R variant of the SMC protocol used direct call to ib_device_ops.get_netdev()
 function to lookup netdev. Such direct accesses are not correct for any
 usage outside of RDMA core code. 

 RDMA subsystem provides ib_device_get_netdev() function that works on
 all RDMA drivers returns valid netdev with proper locking an reference
 counting. The commit 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev
 functions") exposed that SMC-R didn't use that function.

 So update the SMC-R to use proper API,

Thanks

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Wenjia Zhang]

On 05.11.24 12:23, Leon Romanovsky wrote:
> On Tue, Nov 05, 2024 at 10:50:45AM +0100, Wenjia Zhang wrote:
>>
>>
>> On 27.10.24 21:18, Leon Romanovsky wrote:
>>> On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:
>>>> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
>>>> alternative to get_netdev") introduced an API ib_device_get_netdev.
>>>> The SMC-R variant of the SMC protocol continued to use the old API
>>>> ib_device_ops.get_netdev() to lookup netdev.
>>>
>>> I would say that calls to ibdev ops from ULPs was never been right
>>> thing to do. The ib_device_set_netdev() was introduced for the drivers.
>>>
>>> So the whole commit message is not accurate and better to be rewritten.
>>>
>>>> As this commit 8d159eb2117b
>>>> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
>>>> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
>>>> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
>>>> device driver.
>>>
>>> It is not a correct statement too. All modern drivers (for last 5 years)
>>> don't have that .get_netdev() ops, so it is not mlx5 specific, but another
>>> justification to say that SMC-R was doing it wrong.
>>>
>>>> Thus, using ib_device_set_netdev() now became mandatory.
>>>
>>> ib_device_set_netdev() is mandatory for the drivers, it is nothing to do
>>> with ULPs.
>>>
>>>>
>>>> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
>>>
>>> It is too late for me to do proper review for today, but I would say
>>> that it is worth to pay attention to multiple dev_put() calls in the
>>> functions around the ib_device_get_netdev().
>>>
>>>>
>>>> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
>>>> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
>>>
>>> It is not related to this change Fixes line.
>>>
>>
>> Hi Leon,
>>
>> Thank you for the review! I agree that SMC could do better. However, we
>> should fix it and give enough information and reference on the changes,
>> since the code has already existed and didn't work with the old way.
> 
> The code which you change worked by chance and was wrong from day one.
> 
>> I can rewrite the commit message.
>>
>> What about:
>> "
>> The SMC-R variant of the SMC protocol still called
>> ib_device_ops.get_netdev() to lookup netdev. As we used mlx5 device driver
>> to run SMC-R, it failed to find a device, because in mlx5_ib the internal
>> net device management for retrieving net devices was replaced by a common
>> interface ib_device_get_netdev() in commit 8d159eb2117b ("RDMA/mlx5: Use IB
>> set_netdev and get_netdev functions"). Thus, replace
>> ib_device_ops.get_netdev() with ib_device_get_netdev() in SMC.
>> "
> 
>   The SMC-R variant of the SMC protocol used direct call to ib_device_ops.get_netdev()
>   function to lookup netdev. Such direct accesses are not correct for any
>   usage outside of RDMA core code.
> 
Is such an absolute statement documented somewhere? If not, I don't 
think it's convenient that I use it. Maybe you guys as RDMA core 
maintainer can, not I.
>   RDMA subsystem provides ib_device_get_netdev() function that works on
>   all RDMA drivers returns valid netdev with proper locking an reference
>   counting. The commit 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev
>   functions") exposed that SMC-R didn't use that function.
> 
>   So update the SMC-R to use proper API,
> 
> Thanks
> 
mhhh, I'd like to stick to my version, which sounds more neutral IMO. I 
think the purpose is the same.

Thanks,
Wenjia

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Tue, Nov 05, 2024 at 01:30:24PM +0100, Wenjia Zhang wrote:
> 
> 
> On 05.11.24 12:23, Leon Romanovsky wrote:
> > On Tue, Nov 05, 2024 at 10:50:45AM +0100, Wenjia Zhang wrote:
> > > 
> > > 
> > > On 27.10.24 21:18, Leon Romanovsky wrote:
> > > > On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:
> > > > > Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
> > > > > alternative to get_netdev") introduced an API ib_device_get_netdev.
> > > > > The SMC-R variant of the SMC protocol continued to use the old API
> > > > > ib_device_ops.get_netdev() to lookup netdev.
> > > > 
> > > > I would say that calls to ibdev ops from ULPs was never been right
> > > > thing to do. The ib_device_set_netdev() was introduced for the drivers.
> > > > 
> > > > So the whole commit message is not accurate and better to be rewritten.
> > > > 
> > > > > As this commit 8d159eb2117b
> > > > > ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
> > > > > get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
> > > > > ib_device_ops.get_netdev didn't work any more at least by using a mlx5
> > > > > device driver.
> > > > 
> > > > It is not a correct statement too. All modern drivers (for last 5 years)
> > > > don't have that .get_netdev() ops, so it is not mlx5 specific, but another
> > > > justification to say that SMC-R was doing it wrong.
> > > > 
> > > > > Thus, using ib_device_set_netdev() now became mandatory.
> > > > 
> > > > ib_device_set_netdev() is mandatory for the drivers, it is nothing to do
> > > > with ULPs.
> > > > 
> > > > > 
> > > > > Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> > > > 
> > > > It is too late for me to do proper review for today, but I would say
> > > > that it is worth to pay attention to multiple dev_put() calls in the
> > > > functions around the ib_device_get_netdev().
> > > > 
> > > > > 
> > > > > Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> > > > > Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> > > > 
> > > > It is not related to this change Fixes line.
> > > > 
> > > 
> > > Hi Leon,
> > > 
> > > Thank you for the review! I agree that SMC could do better. However, we
> > > should fix it and give enough information and reference on the changes,
> > > since the code has already existed and didn't work with the old way.
> > 
> > The code which you change worked by chance and was wrong from day one.
> > 
> > > I can rewrite the commit message.
> > > 
> > > What about:
> > > "
> > > The SMC-R variant of the SMC protocol still called
> > > ib_device_ops.get_netdev() to lookup netdev. As we used mlx5 device driver
> > > to run SMC-R, it failed to find a device, because in mlx5_ib the internal
> > > net device management for retrieving net devices was replaced by a common
> > > interface ib_device_get_netdev() in commit 8d159eb2117b ("RDMA/mlx5: Use IB
> > > set_netdev and get_netdev functions"). Thus, replace
> > > ib_device_ops.get_netdev() with ib_device_get_netdev() in SMC.
> > > "
> > 
> >   The SMC-R variant of the SMC protocol used direct call to ib_device_ops.get_netdev()
> >   function to lookup netdev. Such direct accesses are not correct for any
> >   usage outside of RDMA core code.
> > 
> Is such an absolute statement documented somewhere? If not, I don't think
> it's convenient that I use it. Maybe you guys as RDMA core maintainer can,
> not I.

You can too as it is very clear. All functions which can be used have
EXPORT_SYMBOL near them, ops.get_netdev() has nothing like that.

> >   RDMA subsystem provides ib_device_get_netdev() function that works on
> >   all RDMA drivers returns valid netdev with proper locking an reference
> >   counting. The commit 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev
> >   functions") exposed that SMC-R didn't use that function.
> > 
> >   So update the SMC-R to use proper API,
> > 
> > Thanks
> > 
> mhhh, I'd like to stick to my version, which sounds more neutral IMO. I
> think the purpose is the same.

I don't want to argue about the words, my point is that get_netdev() was
never been the right interface.

Thanks

> 
> Thanks,
> Wenjia

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Wenjia Zhang]

On 05.11.24 14:39, Leon Romanovsky wrote:
> On Tue, Nov 05, 2024 at 01:30:24PM +0100, Wenjia Zhang wrote:
>>
>>
>> On 05.11.24 12:23, Leon Romanovsky wrote:
>>> On Tue, Nov 05, 2024 at 10:50:45AM +0100, Wenjia Zhang wrote:
>>>>
>>>>
>>>> On 27.10.24 21:18, Leon Romanovsky wrote:
>>>>> On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:
>>>>>> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
>>>>>> alternative to get_netdev") introduced an API ib_device_get_netdev.
>>>>>> The SMC-R variant of the SMC protocol continued to use the old API
>>>>>> ib_device_ops.get_netdev() to lookup netdev.
>>>>>
>>>>> I would say that calls to ibdev ops from ULPs was never been right
>>>>> thing to do. The ib_device_set_netdev() was introduced for the drivers.
>>>>>
>>>>> So the whole commit message is not accurate and better to be rewritten.
>>>>>
>>>>>> As this commit 8d159eb2117b
>>>>>> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
>>>>>> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
>>>>>> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
>>>>>> device driver.
>>>>>
>>>>> It is not a correct statement too. All modern drivers (for last 5 years)
>>>>> don't have that .get_netdev() ops, so it is not mlx5 specific, but another
>>>>> justification to say that SMC-R was doing it wrong.
>>>>>
>>>>>> Thus, using ib_device_set_netdev() now became mandatory.
>>>>>
>>>>> ib_device_set_netdev() is mandatory for the drivers, it is nothing to do
>>>>> with ULPs.
>>>>>
>>>>>>
>>>>>> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
>>>>>
>>>>> It is too late for me to do proper review for today, but I would say
>>>>> that it is worth to pay attention to multiple dev_put() calls in the
>>>>> functions around the ib_device_get_netdev().
>>>>>
>>>>>>
>>>>>> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
>>>>>> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
>>>>>
>>>>> It is not related to this change Fixes line.
>>>>>
>>>>
>>>> Hi Leon,
>>>>
>>>> Thank you for the review! I agree that SMC could do better. However, we
>>>> should fix it and give enough information and reference on the changes,
>>>> since the code has already existed and didn't work with the old way.
>>>
>>> The code which you change worked by chance and was wrong from day one.
>>>
>>>> I can rewrite the commit message.
>>>>
>>>> What about:
>>>> "
>>>> The SMC-R variant of the SMC protocol still called
>>>> ib_device_ops.get_netdev() to lookup netdev. As we used mlx5 device driver
>>>> to run SMC-R, it failed to find a device, because in mlx5_ib the internal
>>>> net device management for retrieving net devices was replaced by a common
>>>> interface ib_device_get_netdev() in commit 8d159eb2117b ("RDMA/mlx5: Use IB
>>>> set_netdev and get_netdev functions"). Thus, replace
>>>> ib_device_ops.get_netdev() with ib_device_get_netdev() in SMC.
>>>> "
>>>
>>>    The SMC-R variant of the SMC protocol used direct call to ib_device_ops.get_netdev()
>>>    function to lookup netdev. Such direct accesses are not correct for any
>>>    usage outside of RDMA core code.
>>>
>> Is such an absolute statement documented somewhere? If not, I don't think
>> it's convenient that I use it. Maybe you guys as RDMA core maintainer can,
>> not I.
> 
> You can too as it is very clear. All functions which can be used have
> EXPORT_SYMBOL near them, ops.get_netdev() has nothing like that.
> 
>>>    RDMA subsystem provides ib_device_get_netdev() function that works on
>>>    all RDMA drivers returns valid netdev with proper locking an reference
>>>    counting. The commit 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev
>>>    functions") exposed that SMC-R didn't use that function.
>>>
>>>    So update the SMC-R to use proper API,
>>>
>>> Thanks
>>>
>> mhhh, I'd like to stick to my version, which sounds more neutral IMO. I
>> think the purpose is the same.
> 
> I don't want to argue about the words, my point is that get_netdev() was
> never been the right interface.
> 
> Thanks
> 
Ok, I got you. I'll send v2 with a proper commit message.

Thanks,
Wenjia

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Halil Pasic]

On Tue, 5 Nov 2024 13:23:13 +0200
Leon Romanovsky <leon@kernel.org> wrote:

> On Tue, Nov 05, 2024 at 10:50:45AM +0100, Wenjia Zhang wrote:
> > 
> > 
> > On 27.10.24 21:18, Leon Romanovsky wrote:  
> > > On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:  
> > > > Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as
> > > > an alternative to get_netdev") introduced an API
> > > > ib_device_get_netdev. The SMC-R variant of the SMC protocol
> > > > continued to use the old API ib_device_ops.get_netdev() to
> > > > lookup netdev.  
> > > 
> > > I would say that calls to ibdev ops from ULPs was never been right
> > > thing to do. The ib_device_set_netdev() was introduced for the
> > > drivers.
> > > 
> > > So the whole commit message is not accurate and better to be
> > > rewritten. 
> > > > As this commit 8d159eb2117b
> > > > ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> > > > removed the get_netdev callback from
> > > > mlx5_ib_dev_common_roce_ops, calling ib_device_ops.get_netdev
> > > > didn't work any more at least by using a mlx5 device driver.  
> > > 
> > > It is not a correct statement too. All modern drivers (for last 5
> > > years) don't have that .get_netdev() ops, so it is not mlx5
> > > specific, but another justification to say that SMC-R was doing it
> > > wrong. 
> > > > Thus, using ib_device_set_netdev() now became mandatory.  
> > > 
> > > ib_device_set_netdev() is mandatory for the drivers, it is nothing
> > > to do with ULPs.
> > >   
> > > > 
> > > > Replace ib_device_ops.get_netdev() with ib_device_get_netdev().  
> > > 
> > > It is too late for me to do proper review for today, but I would
> > > say that it is worth to pay attention to multiple dev_put() calls
> > > in the functions around the ib_device_get_netdev().
> > >   
> > > > 
> > > > Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> > > > Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and
> > > > get_netdev functions")  
> > > 
> > > It is not related to this change Fixes line.
> > >   
> > 
> > Hi Leon,
> > 
> > Thank you for the review! I agree that SMC could do better. However,
> > we should fix it and give enough information and reference on the
> > changes, since the code has already existed and didn't work with the
> > old way.   
> 
> The code which you change worked by chance and was wrong from day one.

I absolutely agree with that statement. But please notice that the
commit date of commit c2261dd76b54 ("RDMA/device: Add
ib_device_set_netdev() as an alternative to get_netdev") predates the
commit date of commit 54903572c23c ("net/smc: allow pnetid-less
configuration") only by 9 days. And before commit c2261dd76b54
("RDMA/device: Add ib_device_set_netdev() as an alternative to
get_netdev") there was no 
ib_device_get_netdev() AFAICT.

Maybe the two patches crossed mid air so to say.

> 
> > I can rewrite the commit message.
> > 
> > What about:
> > "
> > The SMC-R variant of the SMC protocol still called
> > ib_device_ops.get_netdev() to lookup netdev. As we used mlx5 device
> > driver to run SMC-R, it failed to find a device, because in mlx5_ib
> > the internal net device management for retrieving net devices was
> > replaced by a common interface ib_device_get_netdev() in commit
> > 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev
> > functions"). Thus, replace ib_device_ops.get_netdev() with
> > ib_device_get_netdev() in SMC. "  
> 
>  The SMC-R variant of the SMC protocol used direct call to
> ib_device_ops.get_netdev() function to lookup netdev. Such direct
> accesses are not correct for any usage outside of RDMA core code. 
> 

I agree, it is not correct since c2261dd76b54 ("RDMA/device: Add
ib_device_set_netdev() as an alternative to get_netdev").

Does fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
I would guess it is not, and I would not actually mind sending a patch
but I have trouble figuring out the logic behind  commit ecce70cf17d9
("ksmbd: fix missing RDMA-capable flag for IPoIB device in
ksmbd_rdma_capable_netdev()").


>  RDMA subsystem provides ib_device_get_netdev() function that works on
>  all RDMA drivers returns valid netdev with proper locking an reference
>  counting. The commit 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and
> get_netdev functions") exposed that SMC-R didn't use that function.
> 

I believe the intention was this all along. I think the commit message
was written with the idea that 54903572c23c happened before c2261dd76b54
which is not the case.

>  So update the SMC-R to use proper API,
> 

I believe this is exactly what the patch does! And I agree we need to
improve on the commit message.

Regards,
Halil

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Wed, Nov 06, 2024 at 10:24:39AM +0100, Halil Pasic wrote:
> On Tue, 5 Nov 2024 13:23:13 +0200
> Leon Romanovsky <leon@kernel.org> wrote:
> 
> > On Tue, Nov 05, 2024 at 10:50:45AM +0100, Wenjia Zhang wrote:
> > > 
> > > 
> > > On 27.10.24 21:18, Leon Romanovsky wrote:  
> > > > On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:  
> > > > > Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as
> > > > > an alternative to get_netdev") introduced an API
> > > > > ib_device_get_netdev. The SMC-R variant of the SMC protocol
> > > > > continued to use the old API ib_device_ops.get_netdev() to
> > > > > lookup netdev.  
> > > > 
> > > > I would say that calls to ibdev ops from ULPs was never been right
> > > > thing to do. The ib_device_set_netdev() was introduced for the
> > > > drivers.
> > > > 
> > > > So the whole commit message is not accurate and better to be
> > > > rewritten. 
> > > > > As this commit 8d159eb2117b
> > > > > ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> > > > > removed the get_netdev callback from
> > > > > mlx5_ib_dev_common_roce_ops, calling ib_device_ops.get_netdev
> > > > > didn't work any more at least by using a mlx5 device driver.  
> > > > 
> > > > It is not a correct statement too. All modern drivers (for last 5
> > > > years) don't have that .get_netdev() ops, so it is not mlx5
> > > > specific, but another justification to say that SMC-R was doing it
> > > > wrong. 
> > > > > Thus, using ib_device_set_netdev() now became mandatory.  
> > > > 
> > > > ib_device_set_netdev() is mandatory for the drivers, it is nothing
> > > > to do with ULPs.
> > > >   
> > > > > 
> > > > > Replace ib_device_ops.get_netdev() with ib_device_get_netdev().  
> > > > 
> > > > It is too late for me to do proper review for today, but I would
> > > > say that it is worth to pay attention to multiple dev_put() calls
> > > > in the functions around the ib_device_get_netdev().
> > > >   
> > > > > 
> > > > > Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
> > > > > Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and
> > > > > get_netdev functions")  
> > > > 
> > > > It is not related to this change Fixes line.
> > > >   
> > > 
> > > Hi Leon,
> > > 
> > > Thank you for the review! I agree that SMC could do better. However,
> > > we should fix it and give enough information and reference on the
> > > changes, since the code has already existed and didn't work with the
> > > old way.   
> > 
> > The code which you change worked by chance and was wrong from day one.
> 
> I absolutely agree with that statement. But please notice that the
> commit date of commit c2261dd76b54 ("RDMA/device: Add
> ib_device_set_netdev() as an alternative to get_netdev") predates the
> commit date of commit 54903572c23c ("net/smc: allow pnetid-less
> configuration") only by 9 days. And before commit c2261dd76b54
> ("RDMA/device: Add ib_device_set_netdev() as an alternative to
> get_netdev") there was no 
> ib_device_get_netdev() AFAICT.

It doesn't make it right.

1. While commit c2261dd76b54 was submitted and discussed, RDMA was not CCed.
2. Author didn't try to add his version of ib_device_get_netdev() as it
is done for all APIs exposed by RDMA core.

> 
> Maybe the two patches crossed mid air so to say.
> 
> > 
> > > I can rewrite the commit message.
> > > 
> > > What about:
> > > "
> > > The SMC-R variant of the SMC protocol still called
> > > ib_device_ops.get_netdev() to lookup netdev. As we used mlx5 device
> > > driver to run SMC-R, it failed to find a device, because in mlx5_ib
> > > the internal net device management for retrieving net devices was
> > > replaced by a common interface ib_device_get_netdev() in commit
> > > 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev
> > > functions"). Thus, replace ib_device_ops.get_netdev() with
> > > ib_device_get_netdev() in SMC. "  
> > 
> >  The SMC-R variant of the SMC protocol used direct call to
> > ib_device_ops.get_netdev() function to lookup netdev. Such direct
> > accesses are not correct for any usage outside of RDMA core code. 
> > 
> 
> I agree, it is not correct since c2261dd76b54 ("RDMA/device: Add
> ib_device_set_netdev() as an alternative to get_netdev").
> 
> Does fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?

RDMA core code is drivers/infiniband/core/*.

> I would guess it is not, and I would not actually mind sending a patch
> but I have trouble figuring out the logic behind  commit ecce70cf17d9
> ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> ksmbd_rdma_capable_netdev()").

It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
GID, netdev and fabric complexity.

> 
> 
> >  RDMA subsystem provides ib_device_get_netdev() function that works on
> >  all RDMA drivers returns valid netdev with proper locking an reference
> >  counting. The commit 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and
> > get_netdev functions") exposed that SMC-R didn't use that function.
> > 
> 
> I believe the intention was this all along. I think the commit message
> was written with the idea that 54903572c23c happened before c2261dd76b54
> which is not the case.
> 
> >  So update the SMC-R to use proper API,
> > 
> 
> I believe this is exactly what the patch does! And I agree we need to
> improve on the commit message.
> 
> Regards,
> Halil

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Halil Pasic]

On Wed, 6 Nov 2024 15:59:10 +0200
Leon Romanovsky <leon@kernel.org> wrote:

> > I absolutely agree with that statement. But please notice that the
> > commit date of commit c2261dd76b54 ("RDMA/device: Add
> > ib_device_set_netdev() as an alternative to get_netdev") predates the
> > commit date of commit 54903572c23c ("net/smc: allow pnetid-less
> > configuration") only by 9 days. And before commit c2261dd76b54
> > ("RDMA/device: Add ib_device_set_netdev() as an alternative to
> > get_netdev") there was no 
> > ib_device_get_netdev() AFAICT.  
> 
> It doesn't make it right.

I agree!
> 
> 1. While commit c2261dd76b54 was submitted and discussed, RDMA was not
> CCed.

Would the RDMA community agree with adding 
L:	linux-rdma@vger.kernel.org
to the "SHARED MEMORY COMMUNICATIONS (SMC) SOCKETS" section of the
MAINTAINERS file, so that get_maintainer.pl tells contributors to cc
RDMA?

In my personal opinion SMC would have benefited greatly from review by
the RDMA community, and this is not the first time where the RDMA
community was not included where it should have been.
 
> 2. Author didn't try to add his version of ib_device_get_netdev() as it
> is done for all APIs exposed by RDMA core.

I understand now that direct access to ops callbacks is off limits for
ULPs. I'm not sure I understand all the details, but I hope I don't have
to.

Regards,
Halil

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Halil Pasic]

On Wed, 6 Nov 2024 15:59:10 +0200
Leon Romanovsky <leon@kernel.org> wrote:

> > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?  
> 
> RDMA core code is drivers/infiniband/core/*.

Understood. So this is a violation of the no direct access to the
callbacks rule.

> 
> > I would guess it is not, and I would not actually mind sending a patch
> > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > ksmbd_rdma_capable_netdev()").  
> 
> It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> GID, netdev and fabric complexity.

I'm not familiar enough with either of the subsystems. Based on your
answer my guess is that it ain't outright bugous but still a layering 
violation. Copying linux-cifs@vger.kernel.org so that 
the smb are aware.

Thank you very much for all the explanations!

Regards,
Halil 

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Thu, Nov 07, 2024 at 12:47:11PM +0100, Halil Pasic wrote:
> On Wed, 6 Nov 2024 15:59:10 +0200
> Leon Romanovsky <leon@kernel.org> wrote:
> 
> > > I absolutely agree with that statement. But please notice that the
> > > commit date of commit c2261dd76b54 ("RDMA/device: Add
> > > ib_device_set_netdev() as an alternative to get_netdev") predates the
> > > commit date of commit 54903572c23c ("net/smc: allow pnetid-less
> > > configuration") only by 9 days. And before commit c2261dd76b54
> > > ("RDMA/device: Add ib_device_set_netdev() as an alternative to
> > > get_netdev") there was no 
> > > ib_device_get_netdev() AFAICT.  
> > 
> > It doesn't make it right.
> 
> I agree!
> > 
> > 1. While commit c2261dd76b54 was submitted and discussed, RDMA was not
> > CCed.
> 
> Would the RDMA community agree with adding 
> L:	linux-rdma@vger.kernel.org
> to the "SHARED MEMORY COMMUNICATIONS (SMC) SOCKETS" section of the
> MAINTAINERS file, so that get_maintainer.pl tells contributors to cc
> RDMA?

Yes, of course. We always curious to see how our in-kernel API works and
if it needs adjustments rather than ULP hacks to overcome its limitations.

> 
> In my personal opinion SMC would have benefited greatly from review by
> the RDMA community, and this is not the first time where the RDMA
> community was not included where it should have been.

Jakub pushes SMC authors to CC RDMA, which is great, but it wasn't in
the past and your idea of adding new entry to MAINTAINERS file will
help.

>  
> > 2. Author didn't try to add his version of ib_device_get_netdev() as it
> > is done for all APIs exposed by RDMA core.
> 
> I understand now that direct access to ops callbacks is off limits for
> ULPs. I'm not sure I understand all the details, but I hope I don't have
> to.
> 
> Regards,
> Halil

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Thu, Nov 07, 2024 at 12:56:43PM +0100, Halil Pasic wrote:
> On Wed, 6 Nov 2024 15:59:10 +0200
> Leon Romanovsky <leon@kernel.org> wrote:
> 
> > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?  
> > 
> > RDMA core code is drivers/infiniband/core/*.
> 
> Understood. So this is a violation of the no direct access to the
> callbacks rule.

It is not rule, but more common sense. Callbacks don't provide any
module reference counting, module autoload e.t.c

It is very rare situation where you call device callbacks from one subsystem
in another. I'm not familiar with such situations.

> 
> > 
> > > I would guess it is not, and I would not actually mind sending a patch
> > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > ksmbd_rdma_capable_netdev()").  
> > 
> > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > GID, netdev and fabric complexity.
> 
> I'm not familiar enough with either of the subsystems. Based on your
> answer my guess is that it ain't outright bugous but still a layering 
> violation. Copying linux-cifs@vger.kernel.org so that 
> the smb are aware.
> 
> Thank you very much for all the explanations!
> 
> Regards,
> Halil 

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Namjae Jeon]

On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
>
> On Wed, 6 Nov 2024 15:59:10 +0200
> Leon Romanovsky <leon@kernel.org> wrote:
>
> > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> >
> > RDMA core code is drivers/infiniband/core/*.
>
> Understood. So this is a violation of the no direct access to the
> callbacks rule.
>
> >
> > > I would guess it is not, and I would not actually mind sending a patch
> > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > ksmbd_rdma_capable_netdev()").
> >
> > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > GID, netdev and fabric complexity.
>
> I'm not familiar enough with either of the subsystems. Based on your
> answer my guess is that it ain't outright bugous but still a layering
> violation. Copying linux-cifs@vger.kernel.org so that
> the smb are aware.
Could you please elaborate what the violation is ?
I would also appreciate it if you could suggest to me how to fix this.

Thanks.
>
> Thank you very much for all the explanations!
>
> Regards,
> Halil
>

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> >
> > On Wed, 6 Nov 2024 15:59:10 +0200
> > Leon Romanovsky <leon@kernel.org> wrote:
> >
> > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > >
> > > RDMA core code is drivers/infiniband/core/*.
> >
> > Understood. So this is a violation of the no direct access to the
> > callbacks rule.
> >
> > >
> > > > I would guess it is not, and I would not actually mind sending a patch
> > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > ksmbd_rdma_capable_netdev()").
> > >
> > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > GID, netdev and fabric complexity.
> >
> > I'm not familiar enough with either of the subsystems. Based on your
> > answer my guess is that it ain't outright bugous but still a layering
> > violation. Copying linux-cifs@vger.kernel.org so that
> > the smb are aware.
> Could you please elaborate what the violation is ?

There are many, but the most screaming is that ksmbd has logic to
differentiate IPoIB devices. These devices are pure netdev devices
and should be treated like that. ULPs should treat them exactly
as they treat netdev devices.

> I would also appreciate it if you could suggest to me how to fix this.
> 
> Thanks.
> >
> > Thank you very much for all the explanations!
> >
> > Regards,
> > Halil
> >

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Namjae Jeon]

On Sat, Nov 9, 2024 at 2:59 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > >
> > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > >
> > > > RDMA core code is drivers/infiniband/core/*.
> > >
> > > Understood. So this is a violation of the no direct access to the
> > > callbacks rule.
> > >
> > > >
> > > > > I would guess it is not, and I would not actually mind sending a patch
> > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > > ksmbd_rdma_capable_netdev()").
> > > >
> > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > > GID, netdev and fabric complexity.
> > >
> > > I'm not familiar enough with either of the subsystems. Based on your
> > > answer my guess is that it ain't outright bugous but still a layering
> > > violation. Copying linux-cifs@vger.kernel.org so that
> > > the smb are aware.
> > Could you please elaborate what the violation is ?
>
> There are many, but the most screaming is that ksmbd has logic to
> differentiate IPoIB devices. These devices are pure netdev devices
> and should be treated like that. ULPs should treat them exactly
> as they treat netdev devices.
Okay, I'll discuss with Kangjing if there's another way to avoid this issue.
If not, I'll revert the patch.

Thanks.
>
> > I would also appreciate it if you could suggest to me how to fix this.
> >
> > Thanks.
> > >
> > > Thank you very much for all the explanations!
> > >
> > > Regards,
> > > Halil
> > >

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

Hi there,

I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
as mentioned in the thread.

I am working on modifying the patch to take care of the layering
violation. The original patch was meant to fix an issue with ksmbd,
where an IPoIB netdev was not recognized as RDMA-capable. The original
version of the capability evaluation tries to match each netdev to
ib_device by calling get_netdev in ib verbs. However this only works
in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
and since with IPoIB it is the other way around (netdev is the upper
layer of ib_device), get_netdev won't work anymore.

I tried to replicate the behavior of device matching reversely in the
original version of my patch using GID, which ended up as the layering
violation. However I am unaware of any exported functions from the
IPoIB driver that could do the reverse lookup from netdev to the lower
layer ib_device. Actually it seems that the IPoIB driver does not have
any exported symbols at all.

It might be that the device matching in reverse just does not make any
sense and does not need to be done at all. As long as it is an IPoIB
device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
automatically assume it is RDMA-capable. I am not 100% sure about this
though.

I am uncertain about how to proceed at this point and would like to
know your thoughts and opinions on this.

Thanks,
Kangjing

On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > >
> > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > >
> > > > RDMA core code is drivers/infiniband/core/*.
> > >
> > > Understood. So this is a violation of the no direct access to the
> > > callbacks rule.
> > >
> > > >
> > > > > I would guess it is not, and I would not actually mind sending a patch
> > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > > ksmbd_rdma_capable_netdev()").
> > > >
> > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > > GID, netdev and fabric complexity.
> > >
> > > I'm not familiar enough with either of the subsystems. Based on your
> > > answer my guess is that it ain't outright bugous but still a layering
> > > violation. Copying linux-cifs@vger.kernel.org so that
> > > the smb are aware.
> > Could you please elaborate what the violation is ?
>
> There are many, but the most screaming is that ksmbd has logic to
> differentiate IPoIB devices. These devices are pure netdev devices
> and should be treated like that. ULPs should treat them exactly
> as they treat netdev devices.
>
> > I would also appreciate it if you could suggest to me how to fix this.
> >
> > Thanks.
> > >
> > > Thank you very much for all the explanations!
> > >
> > > Regards,
> > > Halil
> > >



-- 
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Fri, Dec 13, 2024, at 13:07, Kangjing Huang wrote:
> Hi there,
>
> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> as mentioned in the thread.
>
> I am working on modifying the patch to take care of the layering
> violation. The original patch was meant to fix an issue with ksmbd,
> where an IPoIB netdev was not recognized as RDMA-capable.

This is exactly the purpose and design of IPoIB, to present regular netdev to the users and hide IB layer from them.

> The original
> version of the capability evaluation tries to match each netdev to
> ib_device by calling get_netdev in ib verbs. However this only works
> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> and since with IPoIB it is the other way around (netdev is the upper
> layer of ib_device), get_netdev won't work anymore.
>
> I tried to replicate the behavior of device matching reversely in the
> original version of my patch using GID, which ended up as the layering
> violation. However I am unaware of any exported functions from the
> IPoIB driver that could do the reverse lookup from netdev to the lower
> layer ib_device. Actually it seems that the IPoIB driver does not have
> any exported symbols at all.
>
> It might be that the device matching in reverse just does not make any
> sense and does not need to be done at all. As long as it is an IPoIB
> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> automatically assume it is RDMA-capable. I am not 100% sure about this
> though.
>
> I am uncertain about how to proceed at this point and would like to
> know your thoughts and opinions on this.

Delete this code completely and make sure that ksmbd has two paths only. One for netdevs and one for ib_devices. It is upto users to decide on which interface to run.

Thanks 

>
> Thanks,
> Kangjing
>
> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
>>
>> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
>> > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
>> > >
>> > > On Wed, 6 Nov 2024 15:59:10 +0200
>> > > Leon Romanovsky <leon@kernel.org> wrote:
>> > >
>> > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
>> > > >
>> > > > RDMA core code is drivers/infiniband/core/*.
>> > >
>> > > Understood. So this is a violation of the no direct access to the
>> > > callbacks rule.
>> > >
>> > > >
>> > > > > I would guess it is not, and I would not actually mind sending a patch
>> > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
>> > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
>> > > > > ksmbd_rdma_capable_netdev()").
>> > > >
>> > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
>> > > > GID, netdev and fabric complexity.
>> > >
>> > > I'm not familiar enough with either of the subsystems. Based on your
>> > > answer my guess is that it ain't outright bugous but still a layering
>> > > violation. Copying linux-cifs@vger.kernel.org so that
>> > > the smb are aware.
>> > Could you please elaborate what the violation is ?
>>
>> There are many, but the most screaming is that ksmbd has logic to
>> differentiate IPoIB devices. These devices are pure netdev devices
>> and should be treated like that. ULPs should treat them exactly
>> as they treat netdev devices.
>>
>> > I would also appreciate it if you could suggest to me how to fix this.
>> >
>> > Thanks.
>> > >
>> > > Thank you very much for all the explanations!
>> > >
>> > > Regards,
>> > > Halil
>> > >
>
>
>
> -- 
> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Namjae Jeon]

On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
>
> Hi there,
>
> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> as mentioned in the thread.
>
> I am working on modifying the patch to take care of the layering
> violation. The original patch was meant to fix an issue with ksmbd,
> where an IPoIB netdev was not recognized as RDMA-capable. The original
> version of the capability evaluation tries to match each netdev to
> ib_device by calling get_netdev in ib verbs. However this only works
> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> and since with IPoIB it is the other way around (netdev is the upper
> layer of ib_device), get_netdev won't work anymore.
>
> I tried to replicate the behavior of device matching reversely in the
> original version of my patch using GID, which ended up as the layering
> violation. However I am unaware of any exported functions from the
> IPoIB driver that could do the reverse lookup from netdev to the lower
> layer ib_device. Actually it seems that the IPoIB driver does not have
> any exported symbols at all.
>
> It might be that the device matching in reverse just does not make any
> sense and does not need to be done at all. As long as it is an IPoIB
> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> automatically assume it is RDMA-capable. I am not 100% sure about this
> though.
Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
How about assuming it's RDMA-capable and allowing users to turn
RDMA-capable on/off via sysfs?

Thanks!
>
> I am uncertain about how to proceed at this point and would like to
> know your thoughts and opinions on this.
>
> Thanks,
> Kangjing
>
> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > > >
> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > > >
> > > > > RDMA core code is drivers/infiniband/core/*.
> > > >
> > > > Understood. So this is a violation of the no direct access to the
> > > > callbacks rule.
> > > >
> > > > >
> > > > > > I would guess it is not, and I would not actually mind sending a patch
> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > > > ksmbd_rdma_capable_netdev()").
> > > > >
> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > > > GID, netdev and fabric complexity.
> > > >
> > > > I'm not familiar enough with either of the subsystems. Based on your
> > > > answer my guess is that it ain't outright bugous but still a layering
> > > > violation. Copying linux-cifs@vger.kernel.org so that
> > > > the smb are aware.
> > > Could you please elaborate what the violation is ?
> >
> > There are many, but the most screaming is that ksmbd has logic to
> > differentiate IPoIB devices. These devices are pure netdev devices
> > and should be treated like that. ULPs should treat them exactly
> > as they treat netdev devices.
> >
> > > I would also appreciate it if you could suggest to me how to fix this.
> > >
> > > Thanks.
> > > >
> > > > Thank you very much for all the explanations!
> > > >
> > > > Regards,
> > > > Halil
> > > >
>
>
>
> --
> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
>>
>> Hi there,
>>
>> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
>> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
>> as mentioned in the thread.
>>
>> I am working on modifying the patch to take care of the layering
>> violation. The original patch was meant to fix an issue with ksmbd,
>> where an IPoIB netdev was not recognized as RDMA-capable. The original
>> version of the capability evaluation tries to match each netdev to
>> ib_device by calling get_netdev in ib verbs. However this only works
>> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
>> and since with IPoIB it is the other way around (netdev is the upper
>> layer of ib_device), get_netdev won't work anymore.
>>
>> I tried to replicate the behavior of device matching reversely in the
>> original version of my patch using GID, which ended up as the layering
>> violation. However I am unaware of any exported functions from the
>> IPoIB driver that could do the reverse lookup from netdev to the lower
>> layer ib_device. Actually it seems that the IPoIB driver does not have
>> any exported symbols at all.
>>
>> It might be that the device matching in reverse just does not make any
>> sense and does not need to be done at all. As long as it is an IPoIB
>> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
>> automatically assume it is RDMA-capable. I am not 100% sure about this
>> though.
> Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> How about assuming it's RDMA-capable and allowing users to turn
> RDMA-capable on/off via sysfs?

Any attempt to treat ipoib differently from regular netdevice is wrong by definition.

>
> Thanks!
>>
>> I am uncertain about how to proceed at this point and would like to
>> know your thoughts and opinions on this.
>>
>> Thanks,
>> Kangjing
>>
>> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
>> >
>> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
>> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
>> > > >
>> > > > On Wed, 6 Nov 2024 15:59:10 +0200
>> > > > Leon Romanovsky <leon@kernel.org> wrote:
>> > > >
>> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
>> > > > >
>> > > > > RDMA core code is drivers/infiniband/core/*.
>> > > >
>> > > > Understood. So this is a violation of the no direct access to the
>> > > > callbacks rule.
>> > > >
>> > > > >
>> > > > > > I would guess it is not, and I would not actually mind sending a patch
>> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
>> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
>> > > > > > ksmbd_rdma_capable_netdev()").
>> > > > >
>> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
>> > > > > GID, netdev and fabric complexity.
>> > > >
>> > > > I'm not familiar enough with either of the subsystems. Based on your
>> > > > answer my guess is that it ain't outright bugous but still a layering
>> > > > violation. Copying linux-cifs@vger.kernel.org so that
>> > > > the smb are aware.
>> > > Could you please elaborate what the violation is ?
>> >
>> > There are many, but the most screaming is that ksmbd has logic to
>> > differentiate IPoIB devices. These devices are pure netdev devices
>> > and should be treated like that. ULPs should treat them exactly
>> > as they treat netdev devices.
>> >
>> > > I would also appreciate it if you could suggest to me how to fix this.
>> > >
>> > > Thanks.
>> > > >
>> > > > Thank you very much for all the explanations!
>> > > >
>> > > > Regards,
>> > > > Halil
>> > > >
>>
>>
>>
>> --
>> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
>
>
>
> On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> >>
> >> Hi there,
> >>
> >> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> >> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> >> as mentioned in the thread.
> >>
> >> I am working on modifying the patch to take care of the layering
> >> violation. The original patch was meant to fix an issue with ksmbd,
> >> where an IPoIB netdev was not recognized as RDMA-capable. The original
> >> version of the capability evaluation tries to match each netdev to
> >> ib_device by calling get_netdev in ib verbs. However this only works
> >> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> >> and since with IPoIB it is the other way around (netdev is the upper
> >> layer of ib_device), get_netdev won't work anymore.
> >>
> >> I tried to replicate the behavior of device matching reversely in the
> >> original version of my patch using GID, which ended up as the layering
> >> violation. However I am unaware of any exported functions from the
> >> IPoIB driver that could do the reverse lookup from netdev to the lower
> >> layer ib_device. Actually it seems that the IPoIB driver does not have
> >> any exported symbols at all.
> >>
> >> It might be that the device matching in reverse just does not make any
> >> sense and does not need to be done at all. As long as it is an IPoIB
> >> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> >> automatically assume it is RDMA-capable. I am not 100% sure about this
> >> though.
> > Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > How about assuming it's RDMA-capable and allowing users to turn
> > RDMA-capable on/off via sysfs?
It does make more sense to me at this point to just broadly assume all
ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
this assumption indeed holds and figure out to what extent this could
involve the same layering violation.

>
> Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
>
I would agree that the design direction to treat ipoib as a pure
regular net_device is the good way to go. But the problem with ksmbd
and ipoib devices stems from the SMB protocol itself.

In contrast to protocols that focus on certain functionalities like
nfs, SMB actually tries to manage network interfaces actively in the
protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
sub-feature of SMB Multichannel. Multichannel is designed to let
client and server find multiple data paths automatically (imagine a
pair of hosts with multiple adapters connected by multiple cables) to
increase bandwidth. So client can initiate a
FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
respond with NETWORK_INTERFACE_INFO containing _all_ local network
interface informations, including their capabilities such as
RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
seeing the capability flag would a client attempt to initiate a RDMA
connection.

Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)

TLDR is that the SMB protocol requires the server to enumerate all
net_devices and indicate their RDMA capability, and
ksmbd_rdma_capable_netdev() is only used in that process. Given such
context, I wonder what should be the best way to approach this? Is
using ARPHRD_INFINIBAND good enough and acceptable in terms of
layering?

> >
> > Thanks!
> >>
> >> I am uncertain about how to proceed at this point and would like to
> >> know your thoughts and opinions on this.
> >>
> >> Thanks,
> >> Kangjing
> >>
> >> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> >> >
> >> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> >> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> >> > > >
> >> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> >> > > > Leon Romanovsky <leon@kernel.org> wrote:
> >> > > >
> >> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> >> > > > >
> >> > > > > RDMA core code is drivers/infiniband/core/*.
> >> > > >
> >> > > > Understood. So this is a violation of the no direct access to the
> >> > > > callbacks rule.
> >> > > >
> >> > > > >
> >> > > > > > I would guess it is not, and I would not actually mind sending a patch
> >> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> >> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> >> > > > > > ksmbd_rdma_capable_netdev()").
> >> > > > >
> >> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> >> > > > > GID, netdev and fabric complexity.
> >> > > >
> >> > > > I'm not familiar enough with either of the subsystems. Based on your
> >> > > > answer my guess is that it ain't outright bugous but still a layering
> >> > > > violation. Copying linux-cifs@vger.kernel.org so that
> >> > > > the smb are aware.
> >> > > Could you please elaborate what the violation is ?
> >> >
> >> > There are many, but the most screaming is that ksmbd has logic to
> >> > differentiate IPoIB devices. These devices are pure netdev devices
> >> > and should be treated like that. ULPs should treat them exactly
> >> > as they treat netdev devices.
> >> >
> >> > > I would also appreciate it if you could suggest to me how to fix this.
> >> > >
> >> > > Thanks.
> >> > > >
> >> > > > Thank you very much for all the explanations!
> >> > > >
> >> > > > Regards,
> >> > > > Halil
> >> > > >
> >>
> >>
> >>
> >> --
> >> Kangjing "Chaser" Huang



--
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> >
> >
> > On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > > On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> > >>
> > >> Hi there,
> > >>
> > >> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> > >> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> > >> as mentioned in the thread.
> > >>
> > >> I am working on modifying the patch to take care of the layering
> > >> violation. The original patch was meant to fix an issue with ksmbd,
> > >> where an IPoIB netdev was not recognized as RDMA-capable. The original
> > >> version of the capability evaluation tries to match each netdev to
> > >> ib_device by calling get_netdev in ib verbs. However this only works
> > >> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> > >> and since with IPoIB it is the other way around (netdev is the upper
> > >> layer of ib_device), get_netdev won't work anymore.
> > >>
> > >> I tried to replicate the behavior of device matching reversely in the
> > >> original version of my patch using GID, which ended up as the layering
> > >> violation. However I am unaware of any exported functions from the
> > >> IPoIB driver that could do the reverse lookup from netdev to the lower
> > >> layer ib_device. Actually it seems that the IPoIB driver does not have
> > >> any exported symbols at all.
> > >>
> > >> It might be that the device matching in reverse just does not make any
> > >> sense and does not need to be done at all. As long as it is an IPoIB
> > >> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> > >> automatically assume it is RDMA-capable. I am not 100% sure about this
> > >> though.
> > > Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > > How about assuming it's RDMA-capable and allowing users to turn
> > > RDMA-capable on/off via sysfs?
> It does make more sense to me at this point to just broadly assume all
> ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> this assumption indeed holds and figure out to what extent this could
> involve the same layering violation.
> 
> >
> > Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> >
> I would agree that the design direction to treat ipoib as a pure
> regular net_device is the good way to go. But the problem with ksmbd
> and ipoib devices stems from the SMB protocol itself.
> 
> In contrast to protocols that focus on certain functionalities like
> nfs, SMB actually tries to manage network interfaces actively in the
> protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> sub-feature of SMB Multichannel. Multichannel is designed to let
> client and server find multiple data paths automatically (imagine a
> pair of hosts with multiple adapters connected by multiple cables) to
> increase bandwidth. So client can initiate a
> FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> respond with NETWORK_INTERFACE_INFO containing _all_ local network
> interface informations, including their capabilities such as
> RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> seeing the capability flag would a client attempt to initiate a RDMA
> connection.
> 
> Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> 
> TLDR is that the SMB protocol requires the server to enumerate all
> net_devices and indicate their RDMA capability, and
> ksmbd_rdma_capable_netdev() is only used in that process. Given such
> context, I wonder what should be the best way to approach this? Is
> using ARPHRD_INFINIBAND good enough and acceptable in terms of
> layering?

The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
right check if netdev is IPoIB or not. The layering problem is that
upper layers (ULPs) should use it as regular netdevice.

Thanks

> 
> > >
> > > Thanks!
> > >>
> > >> I am uncertain about how to proceed at this point and would like to
> > >> know your thoughts and opinions on this.
> > >>
> > >> Thanks,
> > >> Kangjing
> > >>
> > >> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >> >
> > >> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > >> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > >> > > >
> > >> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> > >> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > >> > > >
> > >> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > >> > > > >
> > >> > > > > RDMA core code is drivers/infiniband/core/*.
> > >> > > >
> > >> > > > Understood. So this is a violation of the no direct access to the
> > >> > > > callbacks rule.
> > >> > > >
> > >> > > > >
> > >> > > > > > I would guess it is not, and I would not actually mind sending a patch
> > >> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > >> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > >> > > > > > ksmbd_rdma_capable_netdev()").
> > >> > > > >
> > >> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > >> > > > > GID, netdev and fabric complexity.
> > >> > > >
> > >> > > > I'm not familiar enough with either of the subsystems. Based on your
> > >> > > > answer my guess is that it ain't outright bugous but still a layering
> > >> > > > violation. Copying linux-cifs@vger.kernel.org so that
> > >> > > > the smb are aware.
> > >> > > Could you please elaborate what the violation is ?
> > >> >
> > >> > There are many, but the most screaming is that ksmbd has logic to
> > >> > differentiate IPoIB devices. These devices are pure netdev devices
> > >> > and should be treated like that. ULPs should treat them exactly
> > >> > as they treat netdev devices.
> > >> >
> > >> > > I would also appreciate it if you could suggest to me how to fix this.
> > >> > >
> > >> > > Thanks.
> > >> > > >
> > >> > > > Thank you very much for all the explanations!
> > >> > > >
> > >> > > > Regards,
> > >> > > > Halil
> > >> > > >
> > >>
> > >>
> > >>
> > >> --
> > >> Kangjing "Chaser" Huang
> 
> 
> 
> --
> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> > On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > >
> > >
> > > On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > > > On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> > > >>
> > > >> Hi there,
> > > >>
> > > >> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> > > >> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> > > >> as mentioned in the thread.
> > > >>
> > > >> I am working on modifying the patch to take care of the layering
> > > >> violation. The original patch was meant to fix an issue with ksmbd,
> > > >> where an IPoIB netdev was not recognized as RDMA-capable. The original
> > > >> version of the capability evaluation tries to match each netdev to
> > > >> ib_device by calling get_netdev in ib verbs. However this only works
> > > >> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> > > >> and since with IPoIB it is the other way around (netdev is the upper
> > > >> layer of ib_device), get_netdev won't work anymore.
> > > >>
> > > >> I tried to replicate the behavior of device matching reversely in the
> > > >> original version of my patch using GID, which ended up as the layering
> > > >> violation. However I am unaware of any exported functions from the
> > > >> IPoIB driver that could do the reverse lookup from netdev to the lower
> > > >> layer ib_device. Actually it seems that the IPoIB driver does not have
> > > >> any exported symbols at all.
> > > >>
> > > >> It might be that the device matching in reverse just does not make any
> > > >> sense and does not need to be done at all. As long as it is an IPoIB
> > > >> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> > > >> automatically assume it is RDMA-capable. I am not 100% sure about this
> > > >> though.
> > > > Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > > > How about assuming it's RDMA-capable and allowing users to turn
> > > > RDMA-capable on/off via sysfs?
> > It does make more sense to me at this point to just broadly assume all
> > ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> > this assumption indeed holds and figure out to what extent this could
> > involve the same layering violation.
> >
> > >
> > > Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> > >
> > I would agree that the design direction to treat ipoib as a pure
> > regular net_device is the good way to go. But the problem with ksmbd
> > and ipoib devices stems from the SMB protocol itself.
> >
> > In contrast to protocols that focus on certain functionalities like
> > nfs, SMB actually tries to manage network interfaces actively in the
> > protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> > sub-feature of SMB Multichannel. Multichannel is designed to let
> > client and server find multiple data paths automatically (imagine a
> > pair of hosts with multiple adapters connected by multiple cables) to
> > increase bandwidth. So client can initiate a
> > FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> > respond with NETWORK_INTERFACE_INFO containing _all_ local network
> > interface informations, including their capabilities such as
> > RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> > seeing the capability flag would a client attempt to initiate a RDMA
> > connection.
> >
> > Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> >
> > TLDR is that the SMB protocol requires the server to enumerate all
> > net_devices and indicate their RDMA capability, and
> > ksmbd_rdma_capable_netdev() is only used in that process. Given such
> > context, I wonder what should be the best way to approach this? Is
> > using ARPHRD_INFINIBAND good enough and acceptable in terms of
> > layering?
>

> The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
> right check if netdev is IPoIB or not. The layering problem is that
> upper layers (ULPs) should use it as regular netdevice.

This is good to know. However, since the SMB protocol explicitly calls
for enumeration of all network interfaces on the server host,
including their RDMA capabilities, I believe this is a sensible
exception to the layering rule. Or is there anyway else to do this
enumeration from the kernel space?

Or we can give up implementing the full spec of the SMB protocol and
call for explicit configuration from user space on how to respond to
the IOCTL requests in question. Which one looks more sensible to you?

Thanks

>
> Thanks
>
> >
> > > >
> > > > Thanks!
> > > >>
> > > >> I am uncertain about how to proceed at this point and would like to
> > > >> know your thoughts and opinions on this.
> > > >>
> > > >> Thanks,
> > > >> Kangjing
> > > >>
> > > >> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >> >
> > > >> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > > >> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > > >> > > >
> > > >> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > >> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > >> > > >
> > > >> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > >> > > > >
> > > >> > > > > RDMA core code is drivers/infiniband/core/*.
> > > >> > > >
> > > >> > > > Understood. So this is a violation of the no direct access to the
> > > >> > > > callbacks rule.
> > > >> > > >
> > > >> > > > >
> > > >> > > > > > I would guess it is not, and I would not actually mind sending a patch
> > > >> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > >> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > >> > > > > > ksmbd_rdma_capable_netdev()").
> > > >> > > > >
> > > >> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > >> > > > > GID, netdev and fabric complexity.
> > > >> > > >
> > > >> > > > I'm not familiar enough with either of the subsystems. Based on your
> > > >> > > > answer my guess is that it ain't outright bugous but still a layering
> > > >> > > > violation. Copying linux-cifs@vger.kernel.org so that
> > > >> > > > the smb are aware.
> > > >> > > Could you please elaborate what the violation is ?
> > > >> >
> > > >> > There are many, but the most screaming is that ksmbd has logic to
> > > >> > differentiate IPoIB devices. These devices are pure netdev devices
> > > >> > and should be treated like that. ULPs should treat them exactly
> > > >> > as they treat netdev devices.
> > > >> >
> > > >> > > I would also appreciate it if you could suggest to me how to fix this.
> > > >> > >
> > > >> > > Thanks.
> > > >> > > >
> > > >> > > > Thank you very much for all the explanations!
> > > >> > > >
> > > >> > > > Regards,
> > > >> > > > Halil
> > > >> > > >
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> Kangjing "Chaser" Huang
> >
> >
> >
> > --
> > Kangjing "Chaser" Huang



-- 
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> > > On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > >
> > > >
> > > > On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > > > > On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> > > > >>
> > > > >> Hi there,
> > > > >>
> > > > >> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> > > > >> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> > > > >> as mentioned in the thread.
> > > > >>
> > > > >> I am working on modifying the patch to take care of the layering
> > > > >> violation. The original patch was meant to fix an issue with ksmbd,
> > > > >> where an IPoIB netdev was not recognized as RDMA-capable. The original
> > > > >> version of the capability evaluation tries to match each netdev to
> > > > >> ib_device by calling get_netdev in ib verbs. However this only works
> > > > >> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> > > > >> and since with IPoIB it is the other way around (netdev is the upper
> > > > >> layer of ib_device), get_netdev won't work anymore.
> > > > >>
> > > > >> I tried to replicate the behavior of device matching reversely in the
> > > > >> original version of my patch using GID, which ended up as the layering
> > > > >> violation. However I am unaware of any exported functions from the
> > > > >> IPoIB driver that could do the reverse lookup from netdev to the lower
> > > > >> layer ib_device. Actually it seems that the IPoIB driver does not have
> > > > >> any exported symbols at all.
> > > > >>
> > > > >> It might be that the device matching in reverse just does not make any
> > > > >> sense and does not need to be done at all. As long as it is an IPoIB
> > > > >> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> > > > >> automatically assume it is RDMA-capable. I am not 100% sure about this
> > > > >> though.
> > > > > Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > > > > How about assuming it's RDMA-capable and allowing users to turn
> > > > > RDMA-capable on/off via sysfs?
> > > It does make more sense to me at this point to just broadly assume all
> > > ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> > > this assumption indeed holds and figure out to what extent this could
> > > involve the same layering violation.
> > >
> > > >
> > > > Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> > > >
> > > I would agree that the design direction to treat ipoib as a pure
> > > regular net_device is the good way to go. But the problem with ksmbd
> > > and ipoib devices stems from the SMB protocol itself.
> > >
> > > In contrast to protocols that focus on certain functionalities like
> > > nfs, SMB actually tries to manage network interfaces actively in the
> > > protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> > > sub-feature of SMB Multichannel. Multichannel is designed to let
> > > client and server find multiple data paths automatically (imagine a
> > > pair of hosts with multiple adapters connected by multiple cables) to
> > > increase bandwidth. So client can initiate a
> > > FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> > > respond with NETWORK_INTERFACE_INFO containing _all_ local network
> > > interface informations, including their capabilities such as
> > > RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> > > seeing the capability flag would a client attempt to initiate a RDMA
> > > connection.
> > >
> > > Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> > >
> > > TLDR is that the SMB protocol requires the server to enumerate all
> > > net_devices and indicate their RDMA capability, and
> > > ksmbd_rdma_capable_netdev() is only used in that process. Given such
> > > context, I wonder what should be the best way to approach this? Is
> > > using ARPHRD_INFINIBAND good enough and acceptable in terms of
> > > layering?
> >
> 
> > The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
> > right check if netdev is IPoIB or not. The layering problem is that
> > upper layers (ULPs) should use it as regular netdevice.
> 
> This is good to know. However, since the SMB protocol explicitly calls
> for enumeration of all network interfaces on the server host,
> including their RDMA capabilities, I believe this is a sensible
> exception to the layering rule. Or is there anyway else to do this
> enumeration from the kernel space?
> 
> Or we can give up implementing the full spec of the SMB protocol and
> call for explicit configuration from user space on how to respond to
> the IOCTL requests in question. Which one looks more sensible to you?

My preference is to have same IPoIB treatment for all ULPs, including SMB.

My GUESS is that SMB specification authors didn't take into account HW and
Linux SW development around IPoIB and weren't aware of IPoIB offload which
is implemented and enabled by default in all modern IB NICs and Linux OSes.

That offload allows line-rate for IPoIB, something that is not possible
for SW IPoIB.

Thanks

> 
> Thanks
> 
> >
> > Thanks
> >
> > >
> > > > >
> > > > > Thanks!
> > > > >>
> > > > >> I am uncertain about how to proceed at this point and would like to
> > > > >> know your thoughts and opinions on this.
> > > > >>
> > > > >> Thanks,
> > > > >> Kangjing
> > > > >>
> > > > >> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >> >
> > > > >> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > > > >> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > > > >> > > >
> > > > >> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > > >> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > >> > > >
> > > > >> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > > >> > > > >
> > > > >> > > > > RDMA core code is drivers/infiniband/core/*.
> > > > >> > > >
> > > > >> > > > Understood. So this is a violation of the no direct access to the
> > > > >> > > > callbacks rule.
> > > > >> > > >
> > > > >> > > > >
> > > > >> > > > > > I would guess it is not, and I would not actually mind sending a patch
> > > > >> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > >> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > >> > > > > > ksmbd_rdma_capable_netdev()").
> > > > >> > > > >
> > > > >> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > > >> > > > > GID, netdev and fabric complexity.
> > > > >> > > >
> > > > >> > > > I'm not familiar enough with either of the subsystems. Based on your
> > > > >> > > > answer my guess is that it ain't outright bugous but still a layering
> > > > >> > > > violation. Copying linux-cifs@vger.kernel.org so that
> > > > >> > > > the smb are aware.
> > > > >> > > Could you please elaborate what the violation is ?
> > > > >> >
> > > > >> > There are many, but the most screaming is that ksmbd has logic to
> > > > >> > differentiate IPoIB devices. These devices are pure netdev devices
> > > > >> > and should be treated like that. ULPs should treat them exactly
> > > > >> > as they treat netdev devices.
> > > > >> >
> > > > >> > > I would also appreciate it if you could suggest to me how to fix this.
> > > > >> > >
> > > > >> > > Thanks.
> > > > >> > > >
> > > > >> > > > Thank you very much for all the explanations!
> > > > >> > > >
> > > > >> > > > Regards,
> > > > >> > > > Halil
> > > > >> > > >
> > > > >>
> > > > >>
> > > > >>
> > > > >> --
> > > > >> Kangjing "Chaser" Huang
> > >
> > >
> > >
> > > --
> > > Kangjing "Chaser" Huang
> 
> 
> 
> -- 
> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Tom Talpey]

On 1/8/2025 4:31 AM, Leon Romanovsky wrote:
> On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
>> On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
>>>
>>> On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
>>>> On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
>>>>>> On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
>>>>>>>
>>>>>>> Hi there,
>>>>>>>
>>>>>>> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
>>>>>>> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
>>>>>>> as mentioned in the thread.
>>>>>>>
>>>>>>> I am working on modifying the patch to take care of the layering
>>>>>>> violation. The original patch was meant to fix an issue with ksmbd,
>>>>>>> where an IPoIB netdev was not recognized as RDMA-capable. The original
>>>>>>> version of the capability evaluation tries to match each netdev to
>>>>>>> ib_device by calling get_netdev in ib verbs. However this only works
>>>>>>> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
>>>>>>> and since with IPoIB it is the other way around (netdev is the upper
>>>>>>> layer of ib_device), get_netdev won't work anymore.
>>>>>>>
>>>>>>> I tried to replicate the behavior of device matching reversely in the
>>>>>>> original version of my patch using GID, which ended up as the layering
>>>>>>> violation. However I am unaware of any exported functions from the
>>>>>>> IPoIB driver that could do the reverse lookup from netdev to the lower
>>>>>>> layer ib_device. Actually it seems that the IPoIB driver does not have
>>>>>>> any exported symbols at all.
>>>>>>>
>>>>>>> It might be that the device matching in reverse just does not make any
>>>>>>> sense and does not need to be done at all. As long as it is an IPoIB
>>>>>>> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
>>>>>>> automatically assume it is RDMA-capable. I am not 100% sure about this
>>>>>>> though.
>>>>>> Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
>>>>>> How about assuming it's RDMA-capable and allowing users to turn
>>>>>> RDMA-capable on/off via sysfs?
>>>> It does make more sense to me at this point to just broadly assume all
>>>> ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
>>>> this assumption indeed holds and figure out to what extent this could
>>>> involve the same layering violation.
>>>>
>>>>>
>>>>> Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
>>>>>
>>>> I would agree that the design direction to treat ipoib as a pure
>>>> regular net_device is the good way to go. But the problem with ksmbd
>>>> and ipoib devices stems from the SMB protocol itself.
>>>>
>>>> In contrast to protocols that focus on certain functionalities like
>>>> nfs, SMB actually tries to manage network interfaces actively in the
>>>> protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
>>>> sub-feature of SMB Multichannel. Multichannel is designed to let
>>>> client and server find multiple data paths automatically (imagine a
>>>> pair of hosts with multiple adapters connected by multiple cables) to
>>>> increase bandwidth. So client can initiate a
>>>> FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
>>>> respond with NETWORK_INTERFACE_INFO containing _all_ local network
>>>> interface informations, including their capabilities such as
>>>> RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
>>>> seeing the capability flag would a client attempt to initiate a RDMA
>>>> connection.
>>>>
>>>> Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
>>>>
>>>> TLDR is that the SMB protocol requires the server to enumerate all
>>>> net_devices and indicate their RDMA capability, and
>>>> ksmbd_rdma_capable_netdev() is only used in that process. Given such
>>>> context, I wonder what should be the best way to approach this? Is
>>>> using ARPHRD_INFINIBAND good enough and acceptable in terms of
>>>> layering?
>>>
>>
>>> The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
>>> right check if netdev is IPoIB or not. The layering problem is that
>>> upper layers (ULPs) should use it as regular netdevice.
>>
>> This is good to know. However, since the SMB protocol explicitly calls
>> for enumeration of all network interfaces on the server host,
>> including their RDMA capabilities, I believe this is a sensible
>> exception to the layering rule. Or is there anyway else to do this
>> enumeration from the kernel space?
>>
>> Or we can give up implementing the full spec of the SMB protocol and
>> call for explicit configuration from user space on how to respond to
>> the IOCTL requests in question. Which one looks more sensible to you?
> 
> My preference is to have same IPoIB treatment for all ULPs, including SMB.
> 
> My GUESS is that SMB specification authors didn't take into account HW and
> Linux SW development around IPoIB and weren't aware of IPoIB offload which
> is implemented and enabled by default in all modern IB NICs and Linux OSes.

The SMB3 specification is completely unconcerned with IPoIB and any
other layer-2 or layer-3 implementation details. It merely discusses
an exchange of network interface capabilities such as link speed and
RDMA support. The SMB3 client uses this list to implement multichannel.

I totally agree that inspecting ARPHRD_INFINIBAND is an incorrect method
of building this list. Just because an interface supports IPoIB does not
mean it also exposes RDMA, especially in-kernel. And that ignores any
non-IB transport too of course.

Kangjing, please educate me if I'm confused here, but doesn't the
code in ksmbd_rdma_capable_netdev() look up the ib_device anyway, at
the end of the function?

> 	if (rdma_capable == false) {
> 		struct ib_device *ibdev;
> 
> 		ibdev = ib_device_get_by_netdev(netdev, RDMA_DRIVER_UNKNOWN);
> 		if (ibdev) {
> 			if (rdma_frwr_is_supported(&ibdev->attrs))
> 				rdma_capable = true;
> 			ib_device_put(ibdev);
> 		}
> 	}
> 
> 	return rdma_capable;

So, why is the code concerned at all with ARPHRD_INFINIBAND just a few
lines above? And why does it look in the smb_direct_device_list first?

Tom.

> 
> That offload allows line-rate for IPoIB, something that is not possible
> for SW IPoIB.
> 
> Thanks
> 
>>
>> Thanks
>>
>>>
>>> Thanks
>>>
>>>>
>>>>>>
>>>>>> Thanks!
>>>>>>>
>>>>>>> I am uncertain about how to proceed at this point and would like to
>>>>>>> know your thoughts and opinions on this.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Kangjing
>>>>>>>
>>>>>>> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
>>>>>>>>
>>>>>>>> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
>>>>>>>>> On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Wed, 6 Nov 2024 15:59:10 +0200
>>>>>>>>>> Leon Romanovsky <leon@kernel.org> wrote:
>>>>>>>>>>
>>>>>>>>>>>> Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
>>>>>>>>>>>
>>>>>>>>>>> RDMA core code is drivers/infiniband/core/*.
>>>>>>>>>>
>>>>>>>>>> Understood. So this is a violation of the no direct access to the
>>>>>>>>>> callbacks rule.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> I would guess it is not, and I would not actually mind sending a patch
>>>>>>>>>>>> but I have trouble figuring out the logic behind  commit ecce70cf17d9
>>>>>>>>>>>> ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
>>>>>>>>>>>> ksmbd_rdma_capable_netdev()").
>>>>>>>>>>>
>>>>>>>>>>> It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
>>>>>>>>>>> GID, netdev and fabric complexity.
>>>>>>>>>>
>>>>>>>>>> I'm not familiar enough with either of the subsystems. Based on your
>>>>>>>>>> answer my guess is that it ain't outright bugous but still a layering
>>>>>>>>>> violation. Copying linux-cifs@vger.kernel.org so that
>>>>>>>>>> the smb are aware.
>>>>>>>>> Could you please elaborate what the violation is ?
>>>>>>>>
>>>>>>>> There are many, but the most screaming is that ksmbd has logic to
>>>>>>>> differentiate IPoIB devices. These devices are pure netdev devices
>>>>>>>> and should be treated like that. ULPs should treat them exactly
>>>>>>>> as they treat netdev devices.
>>>>>>>>
>>>>>>>>> I would also appreciate it if you could suggest to me how to fix this.
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>> Thank you very much for all the explanations!
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Halil
>>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Kangjing "Chaser" Huang
>>>>
>>>>
>>>>
>>>> --
>>>> Kangjing "Chaser" Huang
>>
>>
>>
>> -- 
>> Kangjing "Chaser" Huang
> 
> 

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

On Wed, Jan 8, 2025 at 5:27 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 1/8/2025 4:31 AM, Leon Romanovsky wrote:
> > On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> >> On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
> >>>
> >>> On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> >>>> On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> >>>>>> On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> >>>>>>>
> >>>>>>> Hi there,
> >>>>>>>
> >>>>>>> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> >>>>>>> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> >>>>>>> as mentioned in the thread.
> >>>>>>>
> >>>>>>> I am working on modifying the patch to take care of the layering
> >>>>>>> violation. The original patch was meant to fix an issue with ksmbd,
> >>>>>>> where an IPoIB netdev was not recognized as RDMA-capable. The original
> >>>>>>> version of the capability evaluation tries to match each netdev to
> >>>>>>> ib_device by calling get_netdev in ib verbs. However this only works
> >>>>>>> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> >>>>>>> and since with IPoIB it is the other way around (netdev is the upper
> >>>>>>> layer of ib_device), get_netdev won't work anymore.
> >>>>>>>
> >>>>>>> I tried to replicate the behavior of device matching reversely in the
> >>>>>>> original version of my patch using GID, which ended up as the layering
> >>>>>>> violation. However I am unaware of any exported functions from the
> >>>>>>> IPoIB driver that could do the reverse lookup from netdev to the lower
> >>>>>>> layer ib_device. Actually it seems that the IPoIB driver does not have
> >>>>>>> any exported symbols at all.
> >>>>>>>
> >>>>>>> It might be that the device matching in reverse just does not make any
> >>>>>>> sense and does not need to be done at all. As long as it is an IPoIB
> >>>>>>> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> >>>>>>> automatically assume it is RDMA-capable. I am not 100% sure about this
> >>>>>>> though.
> >>>>>> Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> >>>>>> How about assuming it's RDMA-capable and allowing users to turn
> >>>>>> RDMA-capable on/off via sysfs?
> >>>> It does make more sense to me at this point to just broadly assume all
> >>>> ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> >>>> this assumption indeed holds and figure out to what extent this could
> >>>> involve the same layering violation.
> >>>>
> >>>>>
> >>>>> Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> >>>>>
> >>>> I would agree that the design direction to treat ipoib as a pure
> >>>> regular net_device is the good way to go. But the problem with ksmbd
> >>>> and ipoib devices stems from the SMB protocol itself.
> >>>>
> >>>> In contrast to protocols that focus on certain functionalities like
> >>>> nfs, SMB actually tries to manage network interfaces actively in the
> >>>> protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> >>>> sub-feature of SMB Multichannel. Multichannel is designed to let
> >>>> client and server find multiple data paths automatically (imagine a
> >>>> pair of hosts with multiple adapters connected by multiple cables) to
> >>>> increase bandwidth. So client can initiate a
> >>>> FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> >>>> respond with NETWORK_INTERFACE_INFO containing _all_ local network
> >>>> interface informations, including their capabilities such as
> >>>> RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> >>>> seeing the capability flag would a client attempt to initiate a RDMA
> >>>> connection.
> >>>>
> >>>> Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> >>>>
> >>>> TLDR is that the SMB protocol requires the server to enumerate all
> >>>> net_devices and indicate their RDMA capability, and
> >>>> ksmbd_rdma_capable_netdev() is only used in that process. Given such
> >>>> context, I wonder what should be the best way to approach this? Is
> >>>> using ARPHRD_INFINIBAND good enough and acceptable in terms of
> >>>> layering?
> >>>
> >>
> >>> The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
> >>> right check if netdev is IPoIB or not. The layering problem is that
> >>> upper layers (ULPs) should use it as regular netdevice.
> >>
> >> This is good to know. However, since the SMB protocol explicitly calls
> >> for enumeration of all network interfaces on the server host,
> >> including their RDMA capabilities, I believe this is a sensible
> >> exception to the layering rule. Or is there anyway else to do this
> >> enumeration from the kernel space?
> >>
> >> Or we can give up implementing the full spec of the SMB protocol and
> >> call for explicit configuration from user space on how to respond to
> >> the IOCTL requests in question. Which one looks more sensible to you?
> >
> > My preference is to have same IPoIB treatment for all ULPs, including SMB.

If that is the case, then we don't really have many options left but
to expose the advertisement of rdma-capable flags as a configurable
option from the user space (maybe via sysfs). I am not sure if this
will allow the user to mark an interface as RDMA-capable despite its
incapability, or would the capability check be allowed to be made from
the configuration validation site?

> >
> > My GUESS is that SMB specification authors didn't take into account HW and
> > Linux SW development around IPoIB and weren't aware of IPoIB offload which
> > is implemented and enabled by default in all modern IB NICs and Linux OSes.
>
> The SMB3 specification is completely unconcerned with IPoIB and any
> other layer-2 or layer-3 implementation details. It merely discusses
> an exchange of network interface capabilities such as link speed and
> RDMA support. The SMB3 client uses this list to implement multichannel.
>
> I totally agree that inspecting ARPHRD_INFINIBAND is an incorrect method
> of building this list. Just because an interface supports IPoIB does not
> mean it also exposes RDMA, especially in-kernel. And that ignores any
> non-IB transport too of course.

I see, so if I understand this argument correctly, the point is that
even if an IPoIB device *is* guaranteed to have some RDMA support in
the stack (since IPoIB is ULP of an IB device, which always supports
RDMA), it does not necessarily want to expose that capability to its
own ULP in the stacks above, right? However if this is the case, then
what would be the correct way to determine the capability flag from a
ULP?

>
> Kangjing, please educate me if I'm confused here, but doesn't the
> code in ksmbd_rdma_capable_netdev() look up the ib_device anyway, at
> the end of the function?
>
> >       if (rdma_capable == false) {
> >               struct ib_device *ibdev;
> >
> >               ibdev = ib_device_get_by_netdev(netdev, RDMA_DRIVER_UNKNOWN);
> >               if (ibdev) {
> >                       if (rdma_frwr_is_supported(&ibdev->attrs))
> >                               rdma_capable = true;
> >                       ib_device_put(ibdev);
> >               }
> >       }
> >
> >       return rdma_capable;
>
> So, why is the code concerned at all with ARPHRD_INFINIBAND just a few
> lines above?

This check does not work with IPoIB devices. For IPoIB devices
ib_device_get_by_netdev would just return null.

Correct me if I am wrong but I think this API is intended to work with
RDMA/IB transport as ULP of a normal netdev transport (e.g. RoCE,
iWARP). It essentially only performs a lookup of the devices up the
stack, not down the stack, thus not working for IPoIB devices. I feel
this is somehow intended as well since it works inline with the
principle that IPoIB devices should just be treated like a normal
netdev.

This is probably true for the get_netdev verb on ib_device, and the
ib_device_get_netdev API, they are just performing the lookup in
reverse direction.

> And why does it look in the smb_direct_device_list first?

I don't know - this section of code is already here before my patch,
and it confuses me a lot too. It is actually doing the lookup in one
direction and then doing it again in the other direction, probably on
the same mapping. I just left it there during initial patch
submission, just in fear of breaking any strange corner cases.

Thanks,
Kangjing


>
> Tom.
>
> >
> > That offload allows line-rate for IPoIB, something that is not possible
> > for SW IPoIB.
> >
> > Thanks
> >
> >>
> >> Thanks
> >>
> >>>
> >>> Thanks
> >>>
> >>>>
> >>>>>>
> >>>>>> Thanks!
> >>>>>>>
> >>>>>>> I am uncertain about how to proceed at this point and would like to
> >>>>>>> know your thoughts and opinions on this.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Kangjing
> >>>>>>>
> >>>>>>> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> >>>>>>>>
> >>>>>>>> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> >>>>>>>>> On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On Wed, 6 Nov 2024 15:59:10 +0200
> >>>>>>>>>> Leon Romanovsky <leon@kernel.org> wrote:
> >>>>>>>>>>
> >>>>>>>>>>>> Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> >>>>>>>>>>>
> >>>>>>>>>>> RDMA core code is drivers/infiniband/core/*.
> >>>>>>>>>>
> >>>>>>>>>> Understood. So this is a violation of the no direct access to the
> >>>>>>>>>> callbacks rule.
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>> I would guess it is not, and I would not actually mind sending a patch
> >>>>>>>>>>>> but I have trouble figuring out the logic behind  commit ecce70cf17d9
> >>>>>>>>>>>> ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> >>>>>>>>>>>> ksmbd_rdma_capable_netdev()").
> >>>>>>>>>>>
> >>>>>>>>>>> It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> >>>>>>>>>>> GID, netdev and fabric complexity.
> >>>>>>>>>>
> >>>>>>>>>> I'm not familiar enough with either of the subsystems. Based on your
> >>>>>>>>>> answer my guess is that it ain't outright bugous but still a layering
> >>>>>>>>>> violation. Copying linux-cifs@vger.kernel.org so that
> >>>>>>>>>> the smb are aware.
> >>>>>>>>> Could you please elaborate what the violation is ?
> >>>>>>>>
> >>>>>>>> There are many, but the most screaming is that ksmbd has logic to
> >>>>>>>> differentiate IPoIB devices. These devices are pure netdev devices
> >>>>>>>> and should be treated like that. ULPs should treat them exactly
> >>>>>>>> as they treat netdev devices.
> >>>>>>>>
> >>>>>>>>> I would also appreciate it if you could suggest to me how to fix this.
> >>>>>>>>>
> >>>>>>>>> Thanks.
> >>>>>>>>>>
> >>>>>>>>>> Thank you very much for all the explanations!
> >>>>>>>>>>
> >>>>>>>>>> Regards,
> >>>>>>>>>> Halil
> >>>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> --
> >>>>>>> Kangjing "Chaser" Huang
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> Kangjing "Chaser" Huang
> >>
> >>
> >>
> >> --
> >> Kangjing "Chaser" Huang
> >
> >
>


--
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Wed, Jan 08, 2025 at 10:40:22PM +0000, Kangjing Huang wrote:
> On Wed, Jan 8, 2025 at 5:27 PM Tom Talpey <tom@talpey.com> wrote:
> >
> > On 1/8/2025 4:31 AM, Leon Romanovsky wrote:
> > > On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> > >> On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >>>
> > >>> On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> > >>>> On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > >>>>>> On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> > >>>>>>>
> > >>>>>>> Hi there,
> > >>>>>>>
> > >>>>>>> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> > >>>>>>> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> > >>>>>>> as mentioned in the thread.
> > >>>>>>>
> > >>>>>>> I am working on modifying the patch to take care of the layering
> > >>>>>>> violation. The original patch was meant to fix an issue with ksmbd,
> > >>>>>>> where an IPoIB netdev was not recognized as RDMA-capable. The original
> > >>>>>>> version of the capability evaluation tries to match each netdev to
> > >>>>>>> ib_device by calling get_netdev in ib verbs. However this only works
> > >>>>>>> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> > >>>>>>> and since with IPoIB it is the other way around (netdev is the upper
> > >>>>>>> layer of ib_device), get_netdev won't work anymore.
> > >>>>>>>
> > >>>>>>> I tried to replicate the behavior of device matching reversely in the
> > >>>>>>> original version of my patch using GID, which ended up as the layering
> > >>>>>>> violation. However I am unaware of any exported functions from the
> > >>>>>>> IPoIB driver that could do the reverse lookup from netdev to the lower
> > >>>>>>> layer ib_device. Actually it seems that the IPoIB driver does not have
> > >>>>>>> any exported symbols at all.
> > >>>>>>>
> > >>>>>>> It might be that the device matching in reverse just does not make any
> > >>>>>>> sense and does not need to be done at all. As long as it is an IPoIB
> > >>>>>>> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> > >>>>>>> automatically assume it is RDMA-capable. I am not 100% sure about this
> > >>>>>>> though.
> > >>>>>> Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > >>>>>> How about assuming it's RDMA-capable and allowing users to turn
> > >>>>>> RDMA-capable on/off via sysfs?
> > >>>> It does make more sense to me at this point to just broadly assume all
> > >>>> ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> > >>>> this assumption indeed holds and figure out to what extent this could
> > >>>> involve the same layering violation.
> > >>>>
> > >>>>>
> > >>>>> Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> > >>>>>
> > >>>> I would agree that the design direction to treat ipoib as a pure
> > >>>> regular net_device is the good way to go. But the problem with ksmbd
> > >>>> and ipoib devices stems from the SMB protocol itself.
> > >>>>
> > >>>> In contrast to protocols that focus on certain functionalities like
> > >>>> nfs, SMB actually tries to manage network interfaces actively in the
> > >>>> protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> > >>>> sub-feature of SMB Multichannel. Multichannel is designed to let
> > >>>> client and server find multiple data paths automatically (imagine a
> > >>>> pair of hosts with multiple adapters connected by multiple cables) to
> > >>>> increase bandwidth. So client can initiate a
> > >>>> FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> > >>>> respond with NETWORK_INTERFACE_INFO containing _all_ local network
> > >>>> interface informations, including their capabilities such as
> > >>>> RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> > >>>> seeing the capability flag would a client attempt to initiate a RDMA
> > >>>> connection.
> > >>>>
> > >>>> Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> > >>>>
> > >>>> TLDR is that the SMB protocol requires the server to enumerate all
> > >>>> net_devices and indicate their RDMA capability, and
> > >>>> ksmbd_rdma_capable_netdev() is only used in that process. Given such
> > >>>> context, I wonder what should be the best way to approach this? Is
> > >>>> using ARPHRD_INFINIBAND good enough and acceptable in terms of
> > >>>> layering?
> > >>>
> > >>
> > >>> The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
> > >>> right check if netdev is IPoIB or not. The layering problem is that
> > >>> upper layers (ULPs) should use it as regular netdevice.
> > >>
> > >> This is good to know. However, since the SMB protocol explicitly calls
> > >> for enumeration of all network interfaces on the server host,
> > >> including their RDMA capabilities, I believe this is a sensible
> > >> exception to the layering rule. Or is there anyway else to do this
> > >> enumeration from the kernel space?
> > >>
> > >> Or we can give up implementing the full spec of the SMB protocol and
> > >> call for explicit configuration from user space on how to respond to
> > >> the IOCTL requests in question. Which one looks more sensible to you?
> > >
> > > My preference is to have same IPoIB treatment for all ULPs, including SMB.
> 
> If that is the case, then we don't really have many options left but
> to expose the advertisement of rdma-capable flags as a configurable
> option from the user space (maybe via sysfs). I am not sure if this
> will allow the user to mark an interface as RDMA-capable despite its
> incapability, or would the capability check be allowed to be made from
> the configuration validation site?

Except SMB spec, why do you need to provide "RDMA-capable" information?
Is it must? What will it give to the users? Why can't you treat IPoIB
like any other netdevice?

> 
> > >
> > > My GUESS is that SMB specification authors didn't take into account HW and
> > > Linux SW development around IPoIB and weren't aware of IPoIB offload which
> > > is implemented and enabled by default in all modern IB NICs and Linux OSes.
> >
> > The SMB3 specification is completely unconcerned with IPoIB and any
> > other layer-2 or layer-3 implementation details. It merely discusses
> > an exchange of network interface capabilities such as link speed and
> > RDMA support. The SMB3 client uses this list to implement multichannel.
> >
> > I totally agree that inspecting ARPHRD_INFINIBAND is an incorrect method
> > of building this list. Just because an interface supports IPoIB does not
> > mean it also exposes RDMA, especially in-kernel. And that ignores any
> > non-IB transport too of course.
> 
> I see, so if I understand this argument correctly, the point is that
> even if an IPoIB device *is* guaranteed to have some RDMA support in
> the stack (since IPoIB is ULP of an IB device, which always supports
> RDMA), it does not necessarily want to expose that capability to its
> own ULP in the stacks above, right? However if this is the case, then
> what would be the correct way to determine the capability flag from a
> ULP?

Let's start with an answer to more simple question: "why do you need
this capability flag?"

> 
> >
> > Kangjing, please educate me if I'm confused here, but doesn't the
> > code in ksmbd_rdma_capable_netdev() look up the ib_device anyway, at
> > the end of the function?
> >
> > >       if (rdma_capable == false) {
> > >               struct ib_device *ibdev;
> > >
> > >               ibdev = ib_device_get_by_netdev(netdev, RDMA_DRIVER_UNKNOWN);
> > >               if (ibdev) {
> > >                       if (rdma_frwr_is_supported(&ibdev->attrs))
> > >                               rdma_capable = true;
> > >                       ib_device_put(ibdev);
> > >               }
> > >       }
> > >
> > >       return rdma_capable;
> >
> > So, why is the code concerned at all with ARPHRD_INFINIBAND just a few
> > lines above?
> 
> This check does not work with IPoIB devices. For IPoIB devices
> ib_device_get_by_netdev would just return null.
> 
> Correct me if I am wrong but I think this API is intended to work with
> RDMA/IB transport as ULP of a normal netdev transport (e.g. RoCE,
> iWARP). It essentially only performs a lookup of the devices up the
> stack, not down the stack, thus not working for IPoIB devices. I feel
> this is somehow intended as well since it works inline with the
> principle that IPoIB devices should just be treated like a normal
> netdev.
> 
> This is probably true for the get_netdev verb on ib_device, and the
> ib_device_get_netdev API, they are just performing the lookup in
> reverse direction.

Yes, ib_device_get_by_netdev and get_netdev are intended to perform
lookup of ib device based on underlying netdev, but in IPoIB case you
are interested in ib device based on upper netdev.

So this leads to another question, if user didn't ask to connect SMB to
IB device and chose netdevice (IPoIB) instead, why are you still forcing
him to take IB path? If it is not user's decision and you are choosing
devices from the list, you already have in your list the IB device which
is connected to IPoIB.

Thanks

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Christoph Hellwig]

On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> This is good to know. However, since the SMB protocol explicitly calls
> for enumeration of all network interfaces on the server host,
> including their RDMA capabilities, I believe this is a sensible
> exception to the layering rule. Or is there anyway else to do this
> enumeration from the kernel space?

No, it's not a sensible exception.  It's a massive data leak and a
completely idiotic protocol feasture Linux should not support.  If the
protocol requires a lsit of network interfaces, a Linux server should
require explicitly require that list to be configured and not expose
private information to the untrusted network.

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

On Thu, Jan 9, 2025 at 8:02 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> > This is good to know. However, since the SMB protocol explicitly calls
> > for enumeration of all network interfaces on the server host,
> > including their RDMA capabilities, I believe this is a sensible
> > exception to the layering rule. Or is there anyway else to do this
> > enumeration from the kernel space?
>
> No, it's not a sensible exception.  It's a massive data leak and a
> completely idiotic protocol feasture Linux should not support.  If the
> protocol requires a lsit of network interfaces, a Linux server should
> require explicitly require that list to be configured and not expose
> private information to the untrusted network.
>

I see the point. I wasn't thinking about it from the security
perspective too much and would agree with you on this argument about
data leaks.

The protocol itself is not necessarily unsecured, since in the spec it
calls for enumeration in an "implementation-specific manner"[1]. I
would interpret that as the implementation could enumerate interfaces
as it sees fit to its functional and security perspectives. The spec
only requires the server to always return a list of client-usable
interfaces, that does not need to always be the full interface list.

That being said, from ksmbd's implementation perspective, this
definitely needs to be something to be explicitly configured.

ref: [1](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d-240729.pdf)
sec 3.3.5.15.11

On Thu, Jan 9, 2025 at 7:59 AM Leon Romanovsky <leon@kernel.org> wrote:
...

> Except SMB spec, why do you need to provide "RDMA-capable" information?
> Is it must? What will it give to the users? Why can't you treat IPoIB
> like any other netdevice?

...

> Let's start with an answer to more simple question: "why do you need
> this capability flag?"

The short answer is: a Windows client requires this flag to be present
to actually utilize RDMA.

Long answer:

Although SMB-Direct (Microsoft speak for SMB over RDMA) protocol
technically is just wrapping SMB packets in RDMA transport, allowing
for a SMB connection to be completely handshaked and established in
RDMA context, without any need for transferring ip packets. This kind
of connection seems to be never happening for the actual client
implementation in Windows.

Under Windows, SMB Direct is only enabled when SMB multichannel is
enabled. And there is no way for the user to specify if they want to
connect to the IPoIB endpoint or RDMA endpoint - the client simply
connects to the ip interface first, uses the aforementioned IOCTL
request to query the RDMA capabilities for all interfaces, and upon
finding usable RDMA-capable interfaces, opens preferred data channels
on RDMA transport to carry the actual data.

As far as I know there is no way from a Windows user's perspective to
modify this behavior and upon disabling SMB multichannel, SMB will
just disable RDMA support and transfer everything over IP. The RDMA
transport is only "automatically upgraded to", no explicit
configurations are available whatsoever.

So if ksmbd does not send out capability flags to indicate the Windows
client to initiate RDMA transport as a side channel, the client will
just keep working with connections on the ip stack and the user can
never utilize RDMA.

To be clear: I would strongly agree that the Windows client's behavior
here is pretty stupid and is just outright dumb design. In configuring
my two-workstation setup utilizing this protocol, the no-config design
of the Windows side has created far more problems than it has solved.
It also somehow brought me here submitting my first kernel patch.
However if we want any interoperability of ksmbd with Windows clients
on the RDMA front, we would need to implement the IOCTL response to
some degree and send out the RDMA capability flags.

My apologies for not making this context earlier, I was too
hyper-focused on the code itself to fall into the classic XY problem
trap.

Disclaimer: all my knowledge and research about windows client is
limited to the edition I have access to (Windows 10 Pro Workstation),
I have no idea if Windows server edition's client is different.

...

>
> Yes, ib_device_get_by_netdev and get_netdev are intended to perform
> lookup of ib device based on underlying netdev, but in IPoIB case you
> are interested in ib device based on upper netdev.
>
> So this leads to another question, if user didn't ask to connect SMB to
> IB device and chose netdevice (IPoIB) instead, why are you still forcing
> him to take IB path? If it is not user's decision and you are choosing
> devices from the list, you already have in your list the IB device which
> is connected to IPoIB.

This is related to the long answer to the first question. Basically
using a Windows client the user cannot make decisions to use RDMA or
not, it is not the decision of the server, either - the client will
enumerate the interfaces and make decisions for the user. And due to
the presence of SMB multichannel, the ip interface receiving the
interface enumeration request is not even guaranteed to be the
connected IPoIB interface - the multichannel protocol is designed to
work with service discovery, where it is totally possible that the
initial connection will be established on another data path, before
the client discovers another better path potentially with RDMA
capability. The only way for ksmbd to be fully compatible with these
is to implement the interface enumeration to some degree. Although I
am indeed convinced that this needs to be explicitly configured to
avoid any security risks.

Thanks


>
> Thanks




--
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Tom Talpey]

On 1/9/2025 5:43 AM, Kangjing Huang wrote:
> On Thu, Jan 9, 2025 at 8:02 AM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
>>> This is good to know. However, since the SMB protocol explicitly calls
>>> for enumeration of all network interfaces on the server host,
>>> including their RDMA capabilities, I believe this is a sensible
>>> exception to the layering rule. Or is there anyway else to do this
>>> enumeration from the kernel space?
>>
>> No, it's not a sensible exception.  It's a massive data leak and a
>> completely idiotic protocol feasture Linux should not support.  If the
>> protocol requires a lsit of network interfaces, a Linux server should
>> require explicitly require that list to be configured and not expose
>> private information to the untrusted network.
>>
> 
> I see the point. I wasn't thinking about it from the security
> perspective too much and would agree with you on this argument about
> data leaks.
> 
> The protocol itself is not necessarily unsecured, since in the spec it
> calls for enumeration in an "implementation-specific manner"[1]. I
> would interpret that as the implementation could enumerate interfaces
> as it sees fit to its functional and security perspectives. The spec
> only requires the server to always return a list of client-usable
> interfaces, that does not need to always be the full interface list.

Absolutely. Windows has a number of filters on both sides to decide
which interfaces to advertise (server) and which to actually connect
to (client). For example, back-end cluster interfaces are never
exposed via QUERY_NETWORK_INTERFACES.

The ksmbd.conf file already has the "interfaces=" stanza, perhaps
you/we should simply consider using that list as the base. This
effectively would force the "bind interfaces only" flag to true
for RDMA adapters however. Making system-specific lists like these
explicitly opt-in is rarely popular.

Another alternative is to use the rdma_cm, which is the actual thing
controlling the listeners. When it begins listening on an adapter,
perhaps there's a callout that ksmbd can monitor to enumerate the
endpoints that will respond to 445/5445.

Tom.

> 
> That being said, from ksmbd's implementation perspective, this
> definitely needs to be something to be explicitly configured.
> 
> ref: [1](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d-240729.pdf)
> sec 3.3.5.15.11
> 
> On Thu, Jan 9, 2025 at 7:59 AM Leon Romanovsky <leon@kernel.org> wrote:
> ...
> 
>> Except SMB spec, why do you need to provide "RDMA-capable" information?
>> Is it must? What will it give to the users? Why can't you treat IPoIB
>> like any other netdevice?
> 
> ...
> 
>> Let's start with an answer to more simple question: "why do you need
>> this capability flag?"
> 
> The short answer is: a Windows client requires this flag to be present
> to actually utilize RDMA.
> 
> Long answer:
> 
> Although SMB-Direct (Microsoft speak for SMB over RDMA) protocol
> technically is just wrapping SMB packets in RDMA transport, allowing
> for a SMB connection to be completely handshaked and established in
> RDMA context, without any need for transferring ip packets. This kind
> of connection seems to be never happening for the actual client
> implementation in Windows.
> 
> Under Windows, SMB Direct is only enabled when SMB multichannel is
> enabled. And there is no way for the user to specify if they want to
> connect to the IPoIB endpoint or RDMA endpoint - the client simply
> connects to the ip interface first, uses the aforementioned IOCTL
> request to query the RDMA capabilities for all interfaces, and upon
> finding usable RDMA-capable interfaces, opens preferred data channels
> on RDMA transport to carry the actual data.
> 
> As far as I know there is no way from a Windows user's perspective to
> modify this behavior and upon disabling SMB multichannel, SMB will
> just disable RDMA support and transfer everything over IP. The RDMA
> transport is only "automatically upgraded to", no explicit
> configurations are available whatsoever.
> 
> So if ksmbd does not send out capability flags to indicate the Windows
> client to initiate RDMA transport as a side channel, the client will
> just keep working with connections on the ip stack and the user can
> never utilize RDMA.
> 
> To be clear: I would strongly agree that the Windows client's behavior
> here is pretty stupid and is just outright dumb design. In configuring
> my two-workstation setup utilizing this protocol, the no-config design
> of the Windows side has created far more problems than it has solved.
> It also somehow brought me here submitting my first kernel patch.
> However if we want any interoperability of ksmbd with Windows clients
> on the RDMA front, we would need to implement the IOCTL response to
> some degree and send out the RDMA capability flags.
> 
> My apologies for not making this context earlier, I was too
> hyper-focused on the code itself to fall into the classic XY problem
> trap.
> 
> Disclaimer: all my knowledge and research about windows client is
> limited to the edition I have access to (Windows 10 Pro Workstation),
> I have no idea if Windows server edition's client is different.
> 
> ...
> 
>>
>> Yes, ib_device_get_by_netdev and get_netdev are intended to perform
>> lookup of ib device based on underlying netdev, but in IPoIB case you
>> are interested in ib device based on upper netdev.
>>
>> So this leads to another question, if user didn't ask to connect SMB to
>> IB device and chose netdevice (IPoIB) instead, why are you still forcing
>> him to take IB path? If it is not user's decision and you are choosing
>> devices from the list, you already have in your list the IB device which
>> is connected to IPoIB.
> 
> This is related to the long answer to the first question. Basically
> using a Windows client the user cannot make decisions to use RDMA or
> not, it is not the decision of the server, either - the client will
> enumerate the interfaces and make decisions for the user. And due to
> the presence of SMB multichannel, the ip interface receiving the
> interface enumeration request is not even guaranteed to be the
> connected IPoIB interface - the multichannel protocol is designed to
> work with service discovery, where it is totally possible that the
> initial connection will be established on another data path, before
> the client discovers another better path potentially with RDMA
> capability. The only way for ksmbd to be fully compatible with these
> is to implement the interface enumeration to some degree. Although I
> am indeed convinced that this needs to be explicitly configured to
> avoid any security risks.
> 
> Thanks
> 
> 
>>
>> Thanks
> 
> 
> 
> 
> --
> Kangjing "Chaser" Huang
> 
> 

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Christoph Hellwig]

On Thu, Jan 09, 2025 at 12:49:16PM -0500, Tom Talpey wrote:
> Absolutely. Windows has a number of filters on both sides to decide
> which interfaces to advertise (server) and which to actually connect
> to (client). For example, back-end cluster interfaces are never
> exposed via QUERY_NETWORK_INTERFACES.
> 
> The ksmbd.conf file already has the "interfaces=" stanza, perhaps
> you/we should simply consider using that list as the base. This
> effectively would force the "bind interfaces only" flag to true
> for RDMA adapters however. Making system-specific lists like these
> explicitly opt-in is rarely popular.

Filter still seems like the wrong approach vs an explicitly configured
list.  That's what for example nvme over fabrics does for discovery.