| Message ID | aab136be84ad03185a1084cb2e1ca9cad322ab23.1637581778.git.leonro@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Skip holes in GID tables | expand |
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
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
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
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)