From mboxrd@z Thu Jan 1 00:00:00 1970 From: Or Gerlitz Subject: Re: [PATCH V1 for-next 3/4] IB/core: Make sure that the PSN does not overflow Date: Sun, 1 Feb 2015 13:33:30 +0200 Message-ID: <54CE0F0A.7080704@mellanox.com> References: <1422522871-11216-1-git-send-email-ogerlitz@mellanox.com> <1422522871-11216-4-git-send-email-ogerlitz@mellanox.com> <20150129175437.GF11842@obsidianresearch.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150129175437.GF11842-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jason Gunthorpe Cc: Roland Dreier , linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Amir Vadai , Tal Alon , Sean Hefty , Majd Dibbiny List-Id: linux-rdma@vger.kernel.org On 1/29/2015 7:54 PM, Jason Gunthorpe wrote: > On Thu, Jan 29, 2015 at 11:14:30AM +0200, Or Gerlitz wrote: >> From: Majd Dibbiny >> >> The rq/sq->psn is 24 bits as defined in the IB spec, therefore ULPs and User >> space applications shouldn't use the 8 most significant bits in the 32 bits >> variables to avoid overflow in modify_qp. >> >> Fixed the PSN usage for kernel ULPs and masked out the 8 most significant bits >> for User-space applications. >> >> From now on, we check in ib_modify_qp_is_ok that the PSN doesn't overflow, and >> we fail if so. > So, I guess the actual problem here is this: > > get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num); > > ? > > So what we need is > > get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num); > + id_priv->seq_num &= 0xffffff; > >> if (qp_attr->qp_state == IB_QPS_RTR) >> - qp_attr->rq_psn = id_priv->seq_num; >> + qp_attr->rq_psn = id_priv->seq_num & 0xffffff; > All this duplicitive masking should be centralized at the seq_num assignment, not sprinkled sure, point taken, will fix. >> int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state, >> - enum ib_qp_type type, enum ib_qp_attr_mask mask, >> - enum rdma_link_layer ll) >> + enum ib_qp_type type, struct ib_qp_attr *attr, >> + enum ib_qp_attr_mask mask, enum rdma_link_layer ll) >> { >> enum ib_qp_attr_mask req_param, opt_param; >> >> @@ -860,6 +860,12 @@ int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state, >> if (mask & ~(req_param | opt_param | IB_QP_STATE)) >> return 0; >> >> + if ((mask & IB_QP_SQ_PSN) && (attr->sq_psn & 0xff000000)) >> + return 0; >> + >> + if ((mask & IB_QP_RQ_PSN) && (attr->rq_psn & 0xff000000)) >> + return 0; >> + > And since rdmacm has had this longstanding bug of generating > 24 bit PSNs, this change seems really scary - very likely to break working systems. By IBTA the HW can only use 24 bits, also the IB CM also makes sure to only encode/decode 24 PSN bits to/from the wire (see the PSN related helpers in drivers/infiniband/core/cm_msgs.h), so in that respect, I don't see what other bits which are not 24 bits out of the 32 generated ones could be of some use to existing applications, please clarify. > > I think we'd be better off keeping the current behavior of masking the PSN, not enforcing the limit. > >> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c >> index 56959ad..bdf35b0 100644 >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c >> @@ -294,7 +294,7 @@ static int ipoib_cm_modify_rx_qp(struct net_device *dev, >> ipoib_warn(priv, "failed to init QP attr for RTR: %d\n", ret); >> return ret; >> } >> - qp_attr.rq_psn = psn; >> + qp_attr.rq_psn = psn & 0xffffff; >> ret = ib_modify_qp(qp, &qp_attr, qp_attr_mask); > Please don't just sed/grep stuff without review. It was trivial for me > to find that this is unneeded: > > psn = prandom_u32() & 0xffffff; > ret = ipoib_cm_modify_rx_qp(dev, cm_id, p->qp, psn); right, will skip ipoib in this patch since it works just fine. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html