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.
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
