[PATCH rdma-next 0/3] Skip holes in GID tables

[PATCH rdma-next 0/3] Skip holes in GID tables

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Hi,

This short series extends rdma_query_gid() callers to skip holes
in GID tables.

Avihai Horon (3):
  RDMA/core: Modify rdma_query_gid() to return accurate error codes
  RDMA/core: Let ib_find_gid() continue search even after empty entry
  RDMA/cma: Let cma_resolve_ib_dev() continue search even after empty
    entry

 drivers/infiniband/core/cache.c  |  8 ++++++--
 drivers/infiniband/core/cma.c    | 14 +++++++++++---
 drivers/infiniband/core/device.c |  3 +++
 3 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.33.1

[PATCH rdma-next 1/3] RDMA/core: Modify rdma_query_gid() to return accurate error codes

[Leon Romanovsky]

From: Avihai Horon <avihaih@nvidia.com>

Modify rdma_query_gid() to return -ENOENT for empty entries. This will
make error reporting more accurate and will be used in next patches.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
Reviewed-by: Mark Zhang <markzhang@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/cache.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 0c98dd3dee67..dd66f1a6e792 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -963,9 +963,13 @@ int rdma_query_gid(struct ib_device *device, u32 port_num,
 	table = rdma_gid_table(device, port_num);
 	read_lock_irqsave(&table->rwlock, flags);
 
-	if (index < 0 || index >= table->sz ||
-	    !is_gid_entry_valid(table->data_vec[index]))
+	if (index < 0 || index >= table->sz)
+		goto done;
+
+	if (!is_gid_entry_valid(table->data_vec[index])) {
+		res = -ENOENT;
 		goto done;
+	}
 
 	memcpy(gid, &table->data_vec[index]->attr.gid, sizeof(*gid));
 	res = 0;
-- 
2.33.1

[PATCH rdma-next 2/3] RDMA/core: Let ib_find_gid() continue search even after empty entry

[Leon Romanovsky]

From: Avihai Horon <avihaih@nvidia.com>

Currently, ib_find_gid() will stop searching after encountering the
first empty GID table entry. This behavior is wrong since neither IB
nor RoCE spec enforce tightly packed GID tables.

For example, when a valid GID entry exists at index N, and if a GID
entry is empty at index N-1, ib_find_gid() will fail to find the valid
entry.

Fix it by making ib_find_gid() continue searching even after
encountering missing entries.

Fixes: 5eb620c81ce3 ("IB/core: Add helpers for uncached GID and P_Key searches")
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
Reviewed-by: Mark Zhang <markzhang@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 22a4adda7981..b5d8443030d4 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2460,8 +2460,11 @@ int ib_find_gid(struct ib_device *device, union ib_gid *gid,
 		for (i = 0; i < device->port_data[port].immutable.gid_tbl_len;
 		     ++i) {
 			ret = rdma_query_gid(device, port, i, &tmp_gid);
+			if (ret == -ENOENT)
+				continue;
 			if (ret)
 				return ret;
+
 			if (!memcmp(&tmp_gid, gid, sizeof *gid)) {
 				*port_num = port;
 				if (index)
-- 
2.33.1

[PATCH rdma-next 3/3] RDMA/cma: Let cma_resolve_ib_dev() continue search even after empty entry

[Leon Romanovsky]

From: Avihai Horon <avihaih@nvidia.com>

Currently, when cma_resolve_ib_dev() searches for a matching GID it will
stop searching after encountering the first empty GID table entry. This
behavior is wrong since neither IB nor RoCE spec enforce tightly packed
GID tables.

For example, when the matching valid GID entry exists at index N, and if
a GID entry is empty at index N-1, cma_resolve_ib_dev() will fail to
find the matching valid entry.

Fix it by making cma_resolve_ib_dev() continue searching even after
encountering missing entries.

Fixes: f17df3b0dede ("RDMA/cma: Add support for AF_IB to rdma_resolve_addr()")
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
Reviewed-by: Mark Zhang <markzhang@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/cma.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 835ac54d4a24..b669002c9255 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -766,6 +766,7 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
 	unsigned int p;
 	u16 pkey, index;
 	enum ib_port_state port_state;
+	int ret;
 	int i;
 
 	cma_dev = NULL;
@@ -784,9 +785,16 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
 
 			if (ib_get_cached_port_state(cur_dev->device, p, &port_state))
 				continue;
-			for (i = 0; !rdma_query_gid(cur_dev->device,
-						    p, i, &gid);
-			     i++) {
+
+			for (i = 0; i < cur_dev->device->port_data[p].immutable.gid_tbl_len;
+			     ++i) {
+				ret = rdma_query_gid(cur_dev->device, p, i,
+						     &gid);
+				if (ret == -ENOENT)
+					continue;
+				if (ret)
+					break;
+
 				if (!memcmp(&gid, dgid, sizeof(gid))) {
 					cma_dev = cur_dev;
 					sgid = gid;
-- 
2.33.1

Re: [PATCH rdma-next 1/3] RDMA/core: Modify rdma_query_gid() to return accurate error codes

[Jason Gunthorpe]

On Mon, Nov 22, 2021 at 01:53:56PM +0200, Leon Romanovsky wrote:
> From: Avihai Horon <avihaih@nvidia.com>
> 
> Modify rdma_query_gid() to return -ENOENT for empty entries. This will
> make error reporting more accurate and will be used in next patches.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Reviewed-by: Mark Zhang <markzhang@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>  drivers/infiniband/core/cache.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> index 0c98dd3dee67..dd66f1a6e792 100644
> +++ b/drivers/infiniband/core/cache.c
> @@ -963,9 +963,13 @@ int rdma_query_gid(struct ib_device *device, u32 port_num,
>  	table = rdma_gid_table(device, port_num);
>  	read_lock_irqsave(&table->rwlock, flags);
>  
> -	if (index < 0 || index >= table->sz ||
> -	    !is_gid_entry_valid(table->data_vec[index]))
> +	if (index < 0 || index >= table->sz)
> +		goto done;
> +
> +	if (!is_gid_entry_valid(table->data_vec[index])) {
> +		res = -ENOENT;
>  		goto done;
> +	}

Please get rid of the default assignment to res so this code is all
consistent

@@ -955,7 +955,7 @@ int rdma_query_gid(struct ib_device *device, u32 port_num,
 {
        struct ib_gid_table *table;
        unsigned long flags;
-       int res = -EINVAL;
+       int res;
 
        if (!rdma_is_port_valid(device, port_num))
                return -EINVAL;
@@ -963,9 +963,15 @@ int rdma_query_gid(struct ib_device *device, u32 port_num,
        table = rdma_gid_table(device, port_num);
        read_lock_irqsave(&table->rwlock, flags);
 
-       if (index < 0 || index >= table->sz ||
-           !is_gid_entry_valid(table->data_vec[index]))
+       if (index < 0 || index >= table->sz) {
+               res = -EINVAL;
                goto done;
+       }
+
+       if (!is_gid_entry_valid(table->data_vec[index])) {
+               res = -ENOENT;
+               goto done;
+       }

Jason

Re: [PATCH rdma-next 2/3] RDMA/core: Let ib_find_gid() continue search even after empty entry

[Jason Gunthorpe]

On Mon, Nov 22, 2021 at 01:53:57PM +0200, Leon Romanovsky wrote:
> From: Avihai Horon <avihaih@nvidia.com>
> 
> Currently, ib_find_gid() will stop searching after encountering the
> first empty GID table entry. This behavior is wrong since neither IB
> nor RoCE spec enforce tightly packed GID tables.
> 
> For example, when a valid GID entry exists at index N, and if a GID
> entry is empty at index N-1, ib_find_gid() will fail to find the valid
> entry.
> 
> Fix it by making ib_find_gid() continue searching even after
> encountering missing entries.
> 
> Fixes: 5eb620c81ce3 ("IB/core: Add helpers for uncached GID and P_Key searches")
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Reviewed-by: Mark Zhang <markzhang@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>  drivers/infiniband/core/device.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 22a4adda7981..b5d8443030d4 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -2460,8 +2460,11 @@ int ib_find_gid(struct ib_device *device, union ib_gid *gid,
>  		for (i = 0; i < device->port_data[port].immutable.gid_tbl_len;
>  		     ++i) {
>  			ret = rdma_query_gid(device, port, i, &tmp_gid);
> +			if (ret == -ENOENT)
> +				continue;
>  			if (ret)
>  				return ret;

There is no return code from rdma_query_gid that means stop searching,
so just write

if (ret)
   continue

Jason

Re: [PATCH rdma-next 3/3] RDMA/cma: Let cma_resolve_ib_dev() continue search even after empty entry

[Jason Gunthorpe]

On Mon, Nov 22, 2021 at 01:53:58PM +0200, Leon Romanovsky wrote:
> From: Avihai Horon <avihaih@nvidia.com>
> 
> Currently, when cma_resolve_ib_dev() searches for a matching GID it will
> stop searching after encountering the first empty GID table entry. This
> behavior is wrong since neither IB nor RoCE spec enforce tightly packed
> GID tables.
> 
> For example, when the matching valid GID entry exists at index N, and if
> a GID entry is empty at index N-1, cma_resolve_ib_dev() will fail to
> find the matching valid entry.
> 
> Fix it by making cma_resolve_ib_dev() continue searching even after
> encountering missing entries.
> 
> Fixes: f17df3b0dede ("RDMA/cma: Add support for AF_IB to rdma_resolve_addr()")
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Reviewed-by: Mark Zhang <markzhang@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>  drivers/infiniband/core/cma.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 835ac54d4a24..b669002c9255 100644
> +++ b/drivers/infiniband/core/cma.c
> @@ -766,6 +766,7 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
>  	unsigned int p;
>  	u16 pkey, index;
>  	enum ib_port_state port_state;
> +	int ret;
>  	int i;
>  
>  	cma_dev = NULL;
> @@ -784,9 +785,16 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
>  
>  			if (ib_get_cached_port_state(cur_dev->device, p, &port_state))
>  				continue;
> -			for (i = 0; !rdma_query_gid(cur_dev->device,
> -						    p, i, &gid);
> -			     i++) {
> +
> +			for (i = 0; i < cur_dev->device->port_data[p].immutable.gid_tbl_len;
> +			     ++i) {
> +				ret = rdma_query_gid(cur_dev->device, p, i,
> +						     &gid);
> +				if (ret == -ENOENT)
> +					continue;
> +				if (ret)
> +					break;

Same here, just always continue

Jason

Re: [PATCH rdma-next 2/3] RDMA/core: Let ib_find_gid() continue search even after empty entry

[Leon Romanovsky]

On Tue, Dec 07, 2021 at 02:43:04PM -0400, Jason Gunthorpe wrote:
> On Mon, Nov 22, 2021 at 01:53:57PM +0200, Leon Romanovsky wrote:
> > From: Avihai Horon <avihaih@nvidia.com>
> > 
> > Currently, ib_find_gid() will stop searching after encountering the
> > first empty GID table entry. This behavior is wrong since neither IB
> > nor RoCE spec enforce tightly packed GID tables.
> > 
> > For example, when a valid GID entry exists at index N, and if a GID
> > entry is empty at index N-1, ib_find_gid() will fail to find the valid
> > entry.
> > 
> > Fix it by making ib_find_gid() continue searching even after
> > encountering missing entries.
> > 
> > Fixes: 5eb620c81ce3 ("IB/core: Add helpers for uncached GID and P_Key searches")
> > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > Reviewed-by: Mark Zhang <markzhang@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> >  drivers/infiniband/core/device.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > index 22a4adda7981..b5d8443030d4 100644
> > +++ b/drivers/infiniband/core/device.c
> > @@ -2460,8 +2460,11 @@ int ib_find_gid(struct ib_device *device, union ib_gid *gid,
> >  		for (i = 0; i < device->port_data[port].immutable.gid_tbl_len;
> >  		     ++i) {
> >  			ret = rdma_query_gid(device, port, i, &tmp_gid);
> > +			if (ret == -ENOENT)
> > +				continue;
> >  			if (ret)
> >  				return ret;
> 
> There is no return code from rdma_query_gid that means stop searching,

In rdma_query_gid() any error stopped searching, and here we continue
same behaviour as before. You can argue that this function can't really
get illegal parameters and it never returns -EINVAL, but someone needs
to check all callers that this is true.

> so just write
> 
> if (ret)
>    continue

As long as we don't delete input validity checks, it is not correct.

Thanks

> 
> Jason

Re: [PATCH rdma-next 2/3] RDMA/core: Let ib_find_gid() continue search even after empty entry

[Jason Gunthorpe]

On Wed, Dec 08, 2021 at 08:51:59AM +0200, Leon Romanovsky wrote:
> On Tue, Dec 07, 2021 at 02:43:04PM -0400, Jason Gunthorpe wrote:
> > On Mon, Nov 22, 2021 at 01:53:57PM +0200, Leon Romanovsky wrote:
> > > From: Avihai Horon <avihaih@nvidia.com>
> > > 
> > > Currently, ib_find_gid() will stop searching after encountering the
> > > first empty GID table entry. This behavior is wrong since neither IB
> > > nor RoCE spec enforce tightly packed GID tables.
> > > 
> > > For example, when a valid GID entry exists at index N, and if a GID
> > > entry is empty at index N-1, ib_find_gid() will fail to find the valid
> > > entry.
> > > 
> > > Fix it by making ib_find_gid() continue searching even after
> > > encountering missing entries.
> > > 
> > > Fixes: 5eb620c81ce3 ("IB/core: Add helpers for uncached GID and P_Key searches")
> > > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > > Reviewed-by: Mark Zhang <markzhang@nvidia.com>
> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > >  drivers/infiniband/core/device.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > index 22a4adda7981..b5d8443030d4 100644
> > > +++ b/drivers/infiniband/core/device.c
> > > @@ -2460,8 +2460,11 @@ int ib_find_gid(struct ib_device *device, union ib_gid *gid,
> > >  		for (i = 0; i < device->port_data[port].immutable.gid_tbl_len;
> > >  		     ++i) {
> > >  			ret = rdma_query_gid(device, port, i, &tmp_gid);
> > > +			if (ret == -ENOENT)
> > > +				continue;
> > >  			if (ret)
> > >  				return ret;
> > 
> > There is no return code from rdma_query_gid that means stop searching,
> 
> In rdma_query_gid() any error stopped searching, and here we continue
> same behaviour as before. You can argue that this function can't really
> get illegal parameters and it never returns -EINVAL, but someone needs
> to check all callers that this is true.
> 
> > so just write
> > 
> > if (ret)
> >    continue
> 
> As long as we don't delete input validity checks, it is not correct.

It is fine, there is no return condition that means stop searching,
and even if we fail the validity checks that is a WARN_ON and keep
going, not a stop searching event here.

So just do continue, no need for complications.

Jason