linux-kernel - Re: [PATCH 1/4] RDMA/core: Squash a single user static function

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]
Message-ID: <20250910122152.GR341237@unreal>
Date: Wed, 10 Sep 2025 15:21:52 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Parav Pandit <parav@...dia.com>
Cc: Edward Srouji <edwards@...dia.com>, "jgg@...pe.ca" <jgg@...pe.ca>,
	"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Cosmin Ratiu <cratiu@...dia.com>,
	Vlad Dumitrescu <vdumitrescu@...dia.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	Tariq Toukan <tariqt@...dia.com>, Mark Bloch <mbloch@...dia.com>,
	Gal Pressman <gal@...dia.com>
Subject: Re: [PATCH 1/4] RDMA/core: Squash a single user static function

On Wed, Sep 10, 2025 at 10:51:08AM +0000, Parav Pandit wrote:
> 
> 
> > From: Leon Romanovsky <leon@...nel.org>
> > Sent: 10 September 2025 01:48 PM
> > 
> > On Sun, Sep 07, 2025 at 07:08:30PM +0300, Edward Srouji wrote:
> > > From: Parav Pandit <parav@...dia.com>
> > >
> > > In order to reduce dependencies in IFF_LOOPBACK in route and neighbour
> > > resolution steps, squash the static function to its single caller and
> > > simplify the code. No functional change.
> > 
> > It needs more explanation why it is true. 
> After second look at the code, it is not true.
> It is not true for the case when dev->flags has IFF_LOOPBACK and translate_ip() failed.
> In existing code, when translate_ip() fails, code still sets dev_addr->network.
> dev_addr->network is not referred if the process_one_req() has error.
> 
> > Before this change, we set dev_addr-
> > >network to some value and returned error.
> > That error propagated upto process_one_req(), which handles only some
> > errors and ignores rest.
> > 
> > So now, we continue to handle REQ without proper req->addr->network.
> >
> Not exactly. When error is returned by the code addr->network is not filled up, which is correct thing to do.
> 
> So at best, commit message should be updated to remove "No functional change".
> 
> Will send v1.

Parav,

Please setup your e-mail client, your reply is mixed with mine and
repeats it.

Thanks

>  
> > Thanks
> > 
> > >
> > > Signed-off-by: Parav Pandit <parav@...dia.com>
> > > Reviewed-by: Vlad Dumitrescu <vdumitrescu@...dia.com>
> > > Signed-off-by: Edward Srouji <edwards@...dia.com>
> > > ---
> > >  drivers/infiniband/core/addr.c | 49
> > > ++++++++++++++--------------------
> > >  1 file changed, 20 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/core/addr.c
> > > b/drivers/infiniband/core/addr.c index be0743dac3ff..594e7ee335f7
> > > 100644
> > > --- a/drivers/infiniband/core/addr.c
> > > +++ b/drivers/infiniband/core/addr.c
> > > @@ -465,34 +465,6 @@ static int addr_resolve_neigh(const struct dst_entry
> > *dst,
> > >  	return ret;
> > >  }
> > >
> > > -static int copy_src_l2_addr(struct rdma_dev_addr *dev_addr,
> > > -			    const struct sockaddr *dst_in,
> > > -			    const struct dst_entry *dst,
> > > -			    const struct net_device *ndev)
> > > -{
> > > -	int ret = 0;
> > > -
> > > -	if (dst->dev->flags & IFF_LOOPBACK)
> > > -		ret = rdma_translate_ip(dst_in, dev_addr);
> > > -	else
> > > -		rdma_copy_src_l2_addr(dev_addr, dst->dev);
> > > -
> > > -	/*
> > > -	 * If there's a gateway and type of device not ARPHRD_INFINIBAND,
> > > -	 * we're definitely in RoCE v2 (as RoCE v1 isn't routable) set the
> > > -	 * network type accordingly.
> > > -	 */
> > > -	if (has_gateway(dst, dst_in->sa_family) &&
> > > -	    ndev->type != ARPHRD_INFINIBAND)
> > > -		dev_addr->network = dst_in->sa_family == AF_INET ?
> > > -						RDMA_NETWORK_IPV4 :
> > > -						RDMA_NETWORK_IPV6;
> > > -	else
> > > -		dev_addr->network = RDMA_NETWORK_IB;
> > > -
> > > -	return ret;
> > > -}
> > > -
> > >  static int rdma_set_src_addr_rcu(struct rdma_dev_addr *dev_addr,
> > >  				 unsigned int *ndev_flags,
> > >  				 const struct sockaddr *dst_in,
> > > @@ -503,6 +475,7 @@ static int rdma_set_src_addr_rcu(struct
> > rdma_dev_addr *dev_addr,
> > >  	*ndev_flags = ndev->flags;
> > >  	/* A physical device must be the RDMA device to use */
> > >  	if (ndev->flags & IFF_LOOPBACK) {
> > > +		int ret;
> > >  		/*
> > >  		 * RDMA (IB/RoCE, iWarp) doesn't run on lo interface or
> > >  		 * loopback IP address. So if route is resolved to loopback
> > @@
> > > -512,9 +485,27 @@ static int rdma_set_src_addr_rcu(struct rdma_dev_addr
> > *dev_addr,
> > >  		ndev = rdma_find_ndev_for_src_ip_rcu(dev_net(ndev),
> > dst_in);
> > >  		if (IS_ERR(ndev))
> > >  			return -ENODEV;
> > > +		ret = rdma_translate_ip(dst_in, dev_addr);
> > > +		if (ret)
> > > +			return ret;
> > > +	} else {
> > > +		rdma_copy_src_l2_addr(dev_addr, dst->dev);
> > >  	}
> > >
> > > -	return copy_src_l2_addr(dev_addr, dst_in, dst, ndev);
> > > +	/*
> > > +	 * If there's a gateway and type of device not ARPHRD_INFINIBAND,
> > > +	 * we're definitely in RoCE v2 (as RoCE v1 isn't routable) set the
> > > +	 * network type accordingly.
> > > +	 */
> > > +	if (has_gateway(dst, dst_in->sa_family) &&
> > > +	    ndev->type != ARPHRD_INFINIBAND)
> > > +		dev_addr->network = dst_in->sa_family == AF_INET ?
> > > +						RDMA_NETWORK_IPV4 :
> > > +						RDMA_NETWORK_IPV6;
> > > +	else
> > > +		dev_addr->network = RDMA_NETWORK_IB;
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  static int set_addr_netns_by_gid_rcu(struct rdma_dev_addr *addr)
> > > --
> > > 2.21.3
> > >
> > >