* [infiniband-core] question about arguments position
@ 2017-05-04 17:42 Gustavo A. R. Silva
2017-05-04 21:52 ` Parav Pandit
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-04 17:42 UTC (permalink / raw)
To: Doug Ledford, Sean Hefty, Hal Rosenstock, Sagi Grimberg,
Bart Van Assche, Steve Wise, Leon Romanovsky, Yishai Hadas,
Moni Shoua
Cc: linux-rdma, linux-kernel
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?
In case it is not, I will send a patch to fix it. But first it would
be great to hear any comment about it.
Thank you
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [infiniband-core] question about arguments position
2017-05-04 17:42 [infiniband-core] question about arguments position Gustavo A. R. Silva
@ 2017-05-04 21:52 ` Parav Pandit
2017-05-04 23:49 ` Doug Ledford
0 siblings, 1 reply; 4+ messages in thread
From: Parav Pandit @ 2017-05-04 21:52 UTC (permalink / raw)
To: Gustavo A. R. Silva, Doug Ledford, Sean Hefty, Hal Rosenstock,
Sagi Grimberg, Bart Van Assche, Steve Wise, Leon Romanovsky,
Yishai Hadas, Moni Shoua
Cc: linux-rdma, linux-kernel
Hi,
> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Gustavo A. R. Silva
> Sent: Thursday, May 4, 2017 12:42 PM
> To: Doug Ledford <dledford@redhat.com>; Sean Hefty
> <sean.hefty@intel.com>; Hal Rosenstock <hal.rosenstock@gmail.com>; Sagi
> Grimberg <sagi@rimberg.me>; Bart Van Assche
> <bart.vanassche@sandisk.com>; Steve Wise <swise@opengridcomputing.com>;
> Leon Romanovsky <leonro@mellanox.com>; Yishai Hadas
> <yishaih@mellanox.com>; Moni Shoua <monis@mellanox.com>
> Cc: linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org
> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [infiniband-core] question about arguments position
2017-05-04 21:52 ` Parav Pandit
@ 2017-05-04 23:49 ` Doug Ledford
2017-05-05 1:38 ` Gustavo A. R. Silva
0 siblings, 1 reply; 4+ messages in thread
From: Doug Ledford @ 2017-05-04 23:49 UTC (permalink / raw)
To: Parav Pandit, Gustavo A. R. Silva, Sean Hefty, Hal Rosenstock,
Sagi Grimberg, Bart Van Assche, Steve Wise, Leon Romanovsky,
Yishai Hadas, Moni Shoua
Cc: linux-rdma, linux-kernel
On Thu, 2017-05-04 at 21:52 +0000, Parav Pandit wrote:
> Hi,
>
> >
> > -----Original Message-----
> > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> > owner@vger.kernel.org] On Behalf Of Gustavo A. R. Silva
> > Sent: Thursday, May 4, 2017 12:42 PM
> > To: Doug Ledford <dledford@redhat.com>; Sean Hefty
> > <sean.hefty@intel.com>; Hal Rosenstock <hal.rosenstock@gmail.com>;
> > Sagi
> > Grimberg <sagi@rimberg.me>; Bart Van Assche
> > <bart.vanassche@sandisk.com>; Steve Wise <swise@opengridcomputing.c
> > om>;
> > Leon Romanovsky <leonro@mellanox.com>; Yishai Hadas
> > <yishaih@mellanox.com>; Moni Shoua <monis@mellanox.com>
> > Cc: linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org
> > 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.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [infiniband-core] question about arguments position
2017-05-04 23:49 ` Doug Ledford
@ 2017-05-05 1:38 ` Gustavo A. R. Silva
0 siblings, 0 replies; 4+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-05 1:38 UTC (permalink / raw)
To: Doug Ledford
Cc: Parav Pandit, Sean Hefty, Hal Rosenstock, Sagi Grimberg,
Bart Van Assche, Steve Wise, Leon Romanovsky, Yishai Hadas,
Moni Shoua, linux-rdma, linux-kernel
Hi Parav and Doug,
Quoting Doug Ledford <dledford@redhat.com>:
> On Thu, 2017-05-04 at 21:52 +0000, Parav Pandit wrote:
>> Hi,
>>
>> >
>> > -----Original Message-----
>> > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
>> > owner@vger.kernel.org] On Behalf Of Gustavo A. R. Silva
>> > Sent: Thursday, May 4, 2017 12:42 PM
>> > To: Doug Ledford <dledford@redhat.com>; Sean Hefty
>> > <sean.hefty@intel.com>; Hal Rosenstock <hal.rosenstock@gmail.com>;
>> > Sagi
>> > Grimberg <sagi@rimberg.me>; Bart Van Assche
>> > <bart.vanassche@sandisk.com>; Steve Wise <swise@opengridcomputing.c
>> > om>;
>> > Leon Romanovsky <leonro@mellanox.com>; Yishai Hadas
>> > <yishaih@mellanox.com>; Moni Shoua <monis@mellanox.com>
>> > Cc: linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org
>> > 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-05-05 1:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-04 17:42 [infiniband-core] question about arguments position Gustavo A. R. Silva
2017-05-04 21:52 ` Parav Pandit
2017-05-04 23:49 ` Doug Ledford
2017-05-05 1:38 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).