[PATCH rdma-rc] RDMA/mlx4: Read pkey table length instead of hardcoded value

[PATCH rdma-rc] RDMA/mlx4: Read pkey table length instead of hardcoded value

[Leon Romanovsky]

From: Mark Bloch <markb@mellanox.com>

The driver shouldn't assume that a pkey table is available, this
can happen if RoCE isn't supported by the device.

Use the pkey table length reported by the device. This together with the
cited commit from Jack caused a regression where mlx4 devices without
RoCE aren't created.

Cc: <stable@vger.kernel.org>
Cc: Long Li <longli@microsoft.com>
Fixes: 1901b91f9982 ("IB/core: Fix potential NULL pointer dereference in pkey cache")
Fixes: fa417f7b520e ("IB/mlx4: Add support for IBoE")
Signed-off-by: Mark Bloch <markb@mellanox.com>
Reviewed-by: Maor Gottlieb <maorg@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx4/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 5e7910a517da..bd4f975e7f9a 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -784,7 +784,8 @@ static int eth_link_query_port(struct ib_device *ibdev, u8 port,
 	props->ip_gids = true;
 	props->gid_tbl_len	= mdev->dev->caps.gid_table_len[port];
 	props->max_msg_sz	= mdev->dev->caps.max_msg_sz;
-	props->pkey_tbl_len	= 1;
+	if (mdev->dev->caps.pkey_table_len[port])
+		props->pkey_tbl_len = 1;
 	props->max_mtu		= IB_MTU_4096;
 	props->max_vl_num	= 2;
 	props->state		= IB_PORT_DOWN;
-- 
2.26.2

Re: [PATCH rdma-rc] RDMA/mlx4: Read pkey table length instead of hardcoded value

[jackm]

On Sun, 23 Aug 2020 09:17:54 +0300
Leon Romanovsky <leon@kernel.org> wrote:

> From: Mark Bloch <markb@mellanox.com>
> 
> The driver shouldn't assume that a pkey table is available, this
> can happen if RoCE isn't supported by the device.
> 
> Use the pkey table length reported by the device. This together with
> the cited commit from Jack caused a regression where mlx4 devices
> without RoCE aren't created.

I don't understand. Do you mean that WITH this patch there is a
regression, or do you mean that this patch FIXES the regression?

If this patch fixes the regression, I suggest the following replacement
text for the last paragraph:

If the pkey_table is not available (which is the case when RoCE is not
supported), the cited commit caused a regression where mlx4_devices
without RoCE are not created.

Fix this by returning a pkey table length of zero in procedure
eth_link_query_port() if the pkey-table length reported by the device
is zero.

> 
> Cc: <stable@vger.kernel.org>
> Cc: Long Li <longli@microsoft.com>
> Fixes: 1901b91f9982 ("IB/core: Fix potential NULL pointer dereference
> in pkey cache") Fixes: fa417f7b520e ("IB/mlx4: Add support for IBoE")
> Signed-off-by: Mark Bloch <markb@mellanox.com>
> Reviewed-by: Maor Gottlieb <maorg@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/hw/mlx4/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/main.c
> b/drivers/infiniband/hw/mlx4/main.c index 5e7910a517da..bd4f975e7f9a
> 100644 --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -784,7 +784,8 @@ static int eth_link_query_port(struct ib_device
> *ibdev, u8 port, props->ip_gids = true;
>  	props->gid_tbl_len	=
> mdev->dev->caps.gid_table_len[port]; props->max_msg_sz	=
> mdev->dev->caps.max_msg_sz;
> -	props->pkey_tbl_len	= 1;

I don't like depending on the caller to provide a zeroed-out props
structure.
I think it is better to do:
   props->pkey_tbl_len = mdev->dev->caps.pkey_table_len[port] ? 1 : 0 ;
so that the pkey_table_len value is set no matter what.

> +	if (mdev->dev->caps.pkey_table_len[port])
> +		props->pkey_tbl_len = 1;
>  	props->max_mtu		= IB_MTU_4096;
>  	props->max_vl_num	= 2;
>  	props->state		= IB_PORT_DOWN;

-Jack

Re: [PATCH rdma-rc] RDMA/mlx4: Read pkey table length instead of hardcoded value

[Leon Romanovsky]

On Sun, Aug 23, 2020 at 02:27:39PM +0300, jackm wrote:
> On Sun, 23 Aug 2020 09:17:54 +0300
> Leon Romanovsky <leon@kernel.org> wrote:
>
> > From: Mark Bloch <markb@mellanox.com>
> >
> > The driver shouldn't assume that a pkey table is available, this
> > can happen if RoCE isn't supported by the device.
> >
> > Use the pkey table length reported by the device. This together with
> > the cited commit from Jack caused a regression where mlx4 devices
> > without RoCE aren't created.
>
> I don't understand. Do you mean that WITH this patch there is a
> regression, or do you mean that this patch FIXES the regression?

This specific patch fixes regression.

>
> If this patch fixes the regression, I suggest the following replacement
> text for the last paragraph:
>
> If the pkey_table is not available (which is the case when RoCE is not
> supported), the cited commit caused a regression where mlx4_devices
> without RoCE are not created.
>
> Fix this by returning a pkey table length of zero in procedure
> eth_link_query_port() if the pkey-table length reported by the device
> is zero.

I'll change, thanks.

>
> >
> > Cc: <stable@vger.kernel.org>
> > Cc: Long Li <longli@microsoft.com>
> > Fixes: 1901b91f9982 ("IB/core: Fix potential NULL pointer dereference
> > in pkey cache") Fixes: fa417f7b520e ("IB/mlx4: Add support for IBoE")
> > Signed-off-by: Mark Bloch <markb@mellanox.com>
> > Reviewed-by: Maor Gottlieb <maorg@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  drivers/infiniband/hw/mlx4/main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx4/main.c
> > b/drivers/infiniband/hw/mlx4/main.c index 5e7910a517da..bd4f975e7f9a
> > 100644 --- a/drivers/infiniband/hw/mlx4/main.c
> > +++ b/drivers/infiniband/hw/mlx4/main.c
> > @@ -784,7 +784,8 @@ static int eth_link_query_port(struct ib_device
> > *ibdev, u8 port, props->ip_gids = true;
> >  	props->gid_tbl_len	=
> > mdev->dev->caps.gid_table_len[port]; props->max_msg_sz	=
> > mdev->dev->caps.max_msg_sz;
> > -	props->pkey_tbl_len	= 1;
>
> I don't like depending on the caller to provide a zeroed-out props
> structure.
> I think it is better to do:
>    props->pkey_tbl_len = mdev->dev->caps.pkey_table_len[port] ? 1 : 0 ;
> so that the pkey_table_len value is set no matter what.

"props" are cleared by definition of IB/core to make sure that drivers
doesn't return junk in ->query_port() for the fields that are not assigned.
This is why I removed redundant assignment to 0.

Thanks

>
> > +	if (mdev->dev->caps.pkey_table_len[port])
> > +		props->pkey_tbl_len = 1;
> >  	props->max_mtu		= IB_MTU_4096;
> >  	props->max_vl_num	= 2;
> >  	props->state		= IB_PORT_DOWN;
>
> -Jack
>

Re: [PATCH rdma-rc] RDMA/mlx4: Read pkey table length instead of hardcoded value

[jackm]

On Sun, 23 Aug 2020 15:33:42 +0300
Leon Romanovsky <leon@kernel.org> wrote:

> "props" are cleared by definition of IB/core to make sure that drivers
> doesn't return junk in ->query_port() for the fields that are not
> assigned. This is why I removed redundant assignment to 0.
> 
> Thanks

OK, got it. I remove my objection. No RoCE support basically means that
the pkey_tbl_len field should be treated as an unassigned field (which
will remain zero, like all the other unassigned fields).

-Jack