Re: [infiniband-core] question about arguments position

[Gustavo A. R. Silva]

Hi Parav and Doug,

Quoting Doug Ledford <dledford@xxxxxxxxxx>:

> On Thu, 2017-05-04 at 21:52 +0000, Parav Pandit wrote:
> > Hi,
> >
> > >
> > > -----Original Message-----
> > > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma-
> > > owner@xxxxxxxxxxxxxxx] On Behalf Of Gustavo A. R. Silva
> > > Sent: Thursday, May 4, 2017 12:42 PM
> > > To: Doug Ledford <dledford@xxxxxxxxxx>; Sean Hefty
> > > <sean.hefty@xxxxxxxxx>; Hal Rosenstock <hal.rosenstock@xxxxxxxxx>;
> > > Sagi
> > > Grimberg <sagi@xxxxxxxxxx>; Bart Van Assche
> > > <bart.vanassche@xxxxxxxxxxx>; Steve Wise <swise@xxxxxxxxxxxxxxxxxxx
> > > om>;
> > > Leon Romanovsky <leonro@xxxxxxxxxxxx>; Yishai Hadas
> > > <yishaih@xxxxxxxxxxxx>; Moni Shoua <monis@xxxxxxxxxxxx>
> > > Cc: linux-rdma@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > Subject: [infiniband-core] question about arguments position
> > >
> > >
> > > Hello everybody,
> > >
> > > While looking into Coverity ID 1351047 I ran into the following
> > > piece of code at
> > > drivers/infiniband/core/verbs.c:496:
> > >
> > > ret = rdma_addr_find_l2_eth_by_grh(&dgid, &sgid,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂah_attr->dmac,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂwc->wc_flags & IB_WC_WITH_VLAN
> > > ?
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂNULL : &vlan_id,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&if_index, &hoplimit);
> > >
> > >
> > > The issue here is that the position of arguments in the call to
> > > rdma_addr_find_l2_eth_by_grh() function do not match the order of
> > > the
> > > parameters:
> > >
> > > &dgid is passed to sgid
> > > &sgid is passed to dgid
> > >
> > > This is the function prototype:
> > >
> > > int rdma_addr_find_l2_eth_by_grh(const union ib_gid *sgid,
> > > 				Âconst union ib_gid *dgid,
> > > 				Âu8 *dmac, u16 *vlan_id, int *if_index,
> > > 				Âint *hoplimit)
> > >
> > > My question here is if this is intentional?
> > >
> > Yes. ib_init_ah_from_wc() creates ah from the incoming packet.
> > Incoming packet has dgid of the receiver node on which this code is
> > getting executed
> > And sgid contains the GID of the sender.
> >
> > When resolving mac address of destination, you use arrived dgid as
> > sgid.
> > And use sgid as dgid because sgid contains destinations GID whom to
> > respond to.
>
> A patch to add a comment and forestall future questions here might be a
> good addition.

In the case of Coverity, I already triaged and documented this issue.  
So people can ignore it in the future.

Regarding the code comments, what about the following patch based on  
Parav's comments:

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 85ed505..38864ea 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -450,6 +450,19 @@ int ib_get_gids_from_rdma_hdr(const union  
rdma_network_hdr *hdr,
 }
 EXPORT_SYMBOL(ib_get_gids_from_rdma_hdr);

+/*
+ * This function creates ah from the incoming packet.
+ * Incoming packet has dgid of the receiver node on which this code is
+ * getting executed and, sgid contains the GID of the sender.
+ *
+ * When resolving mac address of destination, the arrived dgid is used
+ * as sgid and, sgid is used as dgid because sgid contains destinations
+ * GID whom to respond to.
+ *
+ * This is why when calling rdma_addr_find_l2_eth_by_grh() function, the
+ * position of arguments dgid and sgid do not match the order of the
+ * parameters.
+ */
 int ib_init_ah_from_wc(struct ib_device *device, u8 port_num,
                       const struct ib_wc *wc, const struct ib_grh *grh,
                       struct ib_ah_attr *ah_attr)

Thanks for clarifying
--
Gustavo A. R. Silva