Re: [PATCH] ksmbd: fix missing RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()

[Tom Talpey <tom@talpey.com>]

On 10/15/2023 10:45 AM, Namjae Jeon wrote:
> From: Kangjing Huang <huangkangjing@gmail.com>
> 
> Physical ib_device does not have an underlying net_device, thus its
> association with IPoIB net_device cannot be retrieved via
> ops.get_netdev() or ib_device_get_by_netdev(). ksmbd reads physical
> ib_device port GUID from the lower 16 bytes of the hardware addresses on
> IPoIB net_device and match its underlying ib_device using ib_find_gid()
> 
> Signed-off-by: Kangjing Huang <huangkangjing@gmail.com>
> Acked-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/smb/server/transport_rdma.c | 39 +++++++++++++++++++++++++---------
>   1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/smb/server/transport_rdma.c b/fs/smb/server/transport_rdma.c
> index 3b269e1f523a..a82131f7dd83 100644
> --- a/fs/smb/server/transport_rdma.c
> +++ b/fs/smb/server/transport_rdma.c
> @@ -2140,8 +2140,7 @@ static int smb_direct_ib_client_add(struct ib_device *ib_dev)
>   	if (ib_dev->node_type != RDMA_NODE_IB_CA)
>   		smb_direct_port = SMB_DIRECT_PORT_IWARP;
>   
> -	if (!ib_dev->ops.get_netdev ||
> -	    !rdma_frwr_is_supported(&ib_dev->attrs))
> +	if (!rdma_frwr_is_supported(&ib_dev->attrs))
>   		return 0;
>   
>   	smb_dev = kzalloc(sizeof(*smb_dev), GFP_KERNEL);
> @@ -2241,17 +2240,37 @@ bool ksmbd_rdma_capable_netdev(struct net_device *netdev)
>   		for (i = 0; i < smb_dev->ib_dev->phys_port_cnt; i++) {
>   			struct net_device *ndev;
>   
> -			ndev = smb_dev->ib_dev->ops.get_netdev(smb_dev->ib_dev,
> -							       i + 1);
> -			if (!ndev)
> -				continue;
> +			/* RoCE and iWRAP ib_dev is backed by a netdev */
> +			if (smb_dev->ib_dev->ops.get_netdev) {

The "IWRAP" is a typo, but IMO the comment is misleading. This is simply
looking up the target netdev, it's not limited to these two rdma types.
I suggest deleting the comment.

> +				ndev = smb_dev->ib_dev->ops.get_netdev(
> +					smb_dev->ib_dev, i + 1);
> +				if (!ndev)
> +					continue;
>   
> -			if (ndev == netdev) {
> +				if (ndev == netdev) {
> +					dev_put(ndev);
> +					rdma_capable = true;
> +					goto out;
> +				}
>   				dev_put(ndev);

Why not move this dev_put up above the if (ndev == netdev) test? It's
needed in both cases, so it's confusing to have two copies.

> -				rdma_capable = true;
> -				goto out;
> +			/* match physical ib_dev with IPoIB netdev by GUID */

Add more information to this comment, perhaps:

   /* if no exact netdev match, check for matching Infiniband GUID */

> +			} else if (netdev->type == ARPHRD_INFINIBAND) {
> +				struct netdev_hw_addr *ha;
> +				union ib_gid gid;
> +				u32 port_num;
> +				int ret;
> +
> +				netdev_hw_addr_list_for_each(
> +					ha, &netdev->dev_addrs) {
> +					memcpy(&gid, ha->addr + 4, sizeof(gid));
> +					ret = ib_find_gid(smb_dev->ib_dev, &gid,
> +							  &port_num, NULL);
> +					if (!ret) {
> +						rdma_capable = true;
> +						goto out;

Won't this leak the ndev? It needs a dev_put(ndev) before breaking
the loop, too, right?

> +					}
> +				}
>   			}
> -			dev_put(ndev);
>   		}
>   	}
>   out: