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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 8 Dec 2021 20:13:07 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Doug Ledford <dledford@...hat.com>,
        Avihai Horon <avihaih@...dia.com>,
        linux-kernel@...r.kernel.org, linux-rdma@...r.kernel.org,
        Mark Zhang <markzhang@...dia.com>,
        "Michael S. Tsirkin" <mst@....mellanox.co.il>
Subject: Re: [PATCH rdma-next 2/3] RDMA/core: Let ib_find_gid() continue
 search even after empty entry

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@...dia.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@...dia.com>
> > > Reviewed-by: Mark Zhang <markzhang@...dia.com>
> > > Signed-off-by: Leon Romanovsky <leonro@...dia.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