Re: [PATCH V1 for-next 3/4] IB/core: Make sure that the PSN does not overflow

[Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.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 <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>
>> 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