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

From: Jason Gunthorpe <jgg@ziepe.ca>
To: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
	Avihai Horon <avihaih@nvidia.com>,
	linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
	Mark Zhang <markzhang@nvidia.com>,
	"Michael S. Tsirkin" <mst@dev.mellanox.co.il>
Subject: Re: [PATCH rdma-next 2/3] RDMA/core: Let ib_find_gid() continue search even after empty entry
Date: Wed, 8 Dec 2021 20:13:07 -0400	[thread overview]
Message-ID: <20211209001307.GD6467@ziepe.ca> (raw)
In-Reply-To: <YbBWD5rDm+wBxVjU@unreal>

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