> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@xxxxxxxx] > Sent: Tuesday, May 01, 2018 1:19 PM > To: Doug Ledford <dledford@xxxxxxxxxx> > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Parav Pandit > <parav@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Raju > Rangoju <rajur@xxxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx> > Subject: Re: [PATCH rdma-rc] RDMA/cma: Zero out qp and ah attribute > > On Tue, May 01, 2018 at 02:05:13PM -0400, Doug Ledford wrote: > > On Mon, 2018-04-30 at 15:56 -0600, Jason Gunthorpe wrote: > > > On Sun, Apr 29, 2018 at 10:46:46AM +0300, Leon Romanovsky wrote: > > > > From: Parav Pandit <parav@xxxxxxxxxxxx> > > > > > > > > Commit given in fixes tag introduced an accurate check to validate > > > > device, port, index by referring to the cache layer for querying > > > > GIDs for all link layers (IB, RoCE and iWarp). > > > > > > > > When rdmacm tries to modify the QP to RTR state for kernel > > > > consumers, qp and ah attributes are uninitialized. Each transport > > > > layer (IB/iWarp) initializes them depending on transport type. > > > > However qp ah_attr are not used for iWarp and remained > > > > uninitialized, which is further used in ib_query_gid() call. This > > > > results into a failure to query the GID due to an invalid GID > > > > index coming from the uninitialized stack memory. > > > > This is reported and discussed in thread [1]. > > > > > > What is ib_query_gid supposed to do for iWarp? > > > > Return the sole entry in the GID table. But, you pass the index, so > > you have to know to pass index 0. In this case, because of > > uninitialized memory in the ah_attr, it's being told to try and return > > another entry that is invalid. > > Ok, but what is in the 0 index of the GID table for iWarp? It's a GID derived based on mac address of netdev. > > > However, I see no benefit to doing this bandaid fix while leaving the > > real bug: that we are calling ib_query_gid() for no purpose in this > > function now a days. Just clear out the bad usage and move on. To > > that end, I've written a patch for this: > > > > [dledford@haswell-e linus (k.o/wip/dl-for-rc *)]$ git diff diff --git > > a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index > > 8364223422d0..a693fcd4c513 100644 > > +++ b/drivers/infiniband/core/cma.c > > @@ -868,7 +868,6 @@ static int cma_modify_qp_rtr(struct > > rdma_id_private *id_priv, { > > struct ib_qp_attr qp_attr; > > int qp_attr_mask, ret; > > - union ib_gid sgid; > > > > mutex_lock(&id_priv->qp_mutex); > > if (!id_priv->id.qp) { > > @@ -891,12 +890,6 @@ static int cma_modify_qp_rtr(struct rdma_id_private > *id_priv, > > if (ret) > > goto out; > > > > - ret = ib_query_gid(id_priv->id.device, id_priv->id.port_num, > > - rdma_ah_read_grh(&qp_attr.ah_attr)->sgid_index, > > - &sgid, NULL); > > - if (ret) > > - goto out; > > - > > Well, this was serving to ensure that the sgid index is present in the table for > protocols that use gids. > > Presumably modify_qp will check this too though? Yes. Ah_attr creation from wc and path records now have good checks for kernel QPs. User QP checks will be done anyway in modify_qp(). A patch to remove ib_query_gid() is already in Leon's queue with right commit ids. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
