[openib-general] [PATCH] ib_mad: Fix send only registrations

[Sean Hefty]

On Thu, 14 Oct 2004 13:33:08 -0400
Hal Rosenstock <halr at voltaire.com> wrote:

>  	/* Validate MAD registration request if supplied */
>  	if (mad_reg_req) {
> -		if (!recv_handler ||
> -		    mad_reg_req->mgmt_class_version >= MAX_MGMT_VERSION) {
> +		if (mad_reg_req->mgmt_class_version >= MAX_MGMT_VERSION) {
>  			ret = ERR_PTR(-EINVAL);
>  			goto error1;
>  		}
> +		if (!bitmap_empty(mad_reg_req->method_mask,
> +				  IB_MGMT_MAX_METHODS)) {
> +			if (!recv_handler) {
> +				ret = ERR_PTR(-EINVAL);
> +				goto error1;
> +			}
> +		} else {
> +			if (!send_handler) {
> +				ret = ERR_PTR(-EINVAL);
> +				goto error1;
> +			}
> +		}

I'm not quite understanding this change.  If the user has provided a mad_reg_req, they are indicating that they want to receive unsolicited MADs.  A recv_handler should be required.  Am I missing something?

>  	/* Validate device and port */
> @@ -842,7 +854,9 @@
>  		spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
>  
>  		/* Defined behavior is to complete response before request */
> -		mad_agent_priv->agent.recv_handler(&mad_agent_priv->agent,
> +		if (mad_agent_priv->agent.recv_handler)
> +			mad_agent_priv->agent.recv_handler(
> +						&mad_agent_priv->agent,
>  						&recv->header.recv_wc);
>  		atomic_dec(&mad_agent_priv->refcount);

If I understand this change, a client sent a MAD, expecting a response, got one, but didn't register with a receive handler.  As a side thought, I'm wondering how much protection we need to build into the code to handle kernel clients that don't provide all of the necessary parameters, but we can discuss this.

The only real problem with this change is that the receive buffer needs to be released if it is not given to a client.  We should probably change the send status as well, since no response was delivered.

> @@ -851,7 +865,9 @@
>  		mad_send_wc.wr_id = mad_send_wr->wr_id;
>  		ib_mad_complete_send_wr(mad_send_wr, &mad_send_wc);
>  	} else {
> -		mad_agent_priv->agent.recv_handler(&mad_agent_priv->agent,
> +		if (mad_agent_priv->agent.recv_handler)
> +			mad_agent_priv->agent.recv_handler(
> +						&mad_agent_priv->agent,
>  						&recv->header.recv_wc);

Need to free the receive buffer here as well if not delivered.

> -	if (!mad_agent) { 
> +	if (!mad_agent || !mad_agent->agent.recv_handler) { 

This appears to be where the receive buffer would have been freed, but...
We can't safely walk into the mad_agent structure after calling ib_mad_complete_recv().  Immediately above this code a reference is taken on the mad_agent.  That reference is released in ib_mad_complete_recv(), which would allow the user to destroy the mad_agent before returning back to this call and the if statement above.

I'm more in favor of removing checks for a recv_handler completely, but if we want to keep it, we can move it into find_mad_agent(), and just not report a mad_agent if it doesn't have a recv_handler.

>  	if (mad_send_wr->status != IB_WC_SUCCESS )
>  		mad_send_wc->status = mad_send_wr->status;
> -	mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> mad_send_wc);
> +	if (mad_agent_priv->agent.send_handler)
> +		mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> +						    mad_send_wc);

This has similar problems to the receive handling.  If a client issued a send, but doesn't have a send_handler, there's nothing we can do with the send buffer, which needs to be freed.  I think that a client who does this is causing more problems then we can deal with in the access layer.

A possible fix for this is to check that mad_agent has a send_handler when the send is posted, rather than waiting until it completes.

>  	/* Release reference on agent taken when sending */
>  	if (atomic_dec_and_test(&mad_agent_priv->refcount))
> @@ -1135,7 +1153,9 @@
>  	list_for_each_entry_safe(mad_send_wr, temp_mad_send_wr,
>  				 &cancel_list, agent_list) {
>  		mad_send_wc.wr_id = mad_send_wr->wr_id;
> -		mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> +		if (mad_agent_priv->agent.send_handler)
> +			mad_agent_priv->agent.send_handler(
> +						   &mad_agent_priv->agent,
>  						   &mad_send_wc);

Same issue as above.
  
>  		list_del(&mad_send_wr->agent_list);
> @@ -1196,8 +1216,9 @@
>  	mad_send_wc.status = IB_WC_WR_FLUSH_ERR;
>  	mad_send_wc.vendor_err = 0;
>  	mad_send_wc.wr_id = mad_send_wr->wr_id;
> -	mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> -					   &mad_send_wc);
> +	if (mad_agent_priv->agent.send_handler)
> +		mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> +						   &mad_send_wc);

Ditto.