[openib-general] [PATCH] RDMA/iwcm: Bugs in cm_conn_req_handler()

[Tom Tucker]

Roland:

This looks bad. Lemme noodle...

On Thu, 2007-02-08 at 20:23 -0800, Roland Dreier wrote:
> BTW, while looking at iwcm.c, I noticed the following highly dubious
> code for the first time:
> 
> 	static int iwcm_deref_id(struct iwcm_id_private *cm_id_priv)
> 	{
> 		int ret = 0;
> 	
> 		BUG_ON(atomic_read(&cm_id_priv->refcount)==0);
> 		if (atomic_dec_and_test(&cm_id_priv->refcount)) {
> 			BUG_ON(!list_empty(&cm_id_priv->work_list));
> 			if (waitqueue_active(&cm_id_priv->destroy_comp.wait)) {
> 				BUG_ON(cm_id_priv->state != IW_CM_STATE_DESTROYING);
> 				BUG_ON(test_bit(IWCM_F_CALLBACK_DESTROY,
> 						&cm_id_priv->flags));
> 				ret = 1;
> 			}
> 			complete(&cm_id_priv->destroy_comp);
> 		}
> 	
> 		return ret;
> 	}
> 
> The test of waitqueue_active on destroy_comp.wait looks really bad for
> two reasons: first, it is relying on an internal implementation detail
> of struct completion that really shouldn't be used by generic code.
> And second, it seems to me that this doesn't even work right, since
> there is a race something like the following:
> 
> iw_destroy_cm_id():
> destroy_cm_id(cm_id); // still 1 ref left
> 
> 				cm_work_handler():
> 					if (iwcm_deref_id()) // drop last ref
> 						return;
> 					// no one waiting yet, doesn't
> 					// return, but destroy_comp is
> 					// signaled
> 
> wait_for_completion(&cm_id_priv->destroy_comp);
> // destroy_comp is signaled, proceed
> kfree(cm_id_priv);
> 
> 					// continue using cm_id_priv
> 					// OOPS
> 
> I don't understand this code well enough for the fix to be obvious.
> 
>  - R.