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 <majd@xxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
