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

Thu Oct 14 11:32:57 PDT 2004

On Thu, 2004-10-14 at 13:55, Sean Hefty wrote:
> 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?

What if they didn't fill in any methods in their registration request ?
Should a recv_handler be required ? Note that the mthca appears to be a
send only MAD client (for locally generated traps). That's what all this
stemmed from. Maybe I took it too far.

> If I understand this change, a client sent a MAD, expecting a response, got one, 
> but didn't register with a receive handler.  

Maybe he sent something and wasn't expecting a response (so didn't
register a recv handler) but got one anyway. Should the MAD layer crash
because of this ?

> 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.

I've been wondering this myself too. In the previous case, it caused the
MAD layer to crash on a NULL pointer reference and the Linux locked up
sometime thereafter. 

> The only real problem with this change is that the receive buffer needs to be released 
> if it is not given to a client.  

I think it's there at the end of the ib_mad_recv_done_handler where
!mad_agent->agent.recv_handler is checked and kmem_cache_free is called
if so. I see the problem you have with doing it this way later in your
email so I will fix it.

> We should probably change the send status as well, since no response was delivered.

OK. What status would you propose ? Do you want to generate a patch for
this or should I ?

> > @@ -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.

This was to be handled by the code at the end of
ib_mad_recv_done_handler. I will fix it.

> > -  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.

OK. I will move the buffer releases to where the references are held.

> 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.

That's a much better solution. As to the checks for the recv_handler, it
depends on whether a recv_handler is required. Is it is, a simple check
during registration and removal of the checks for whether than recv
handler was supplied (the way it was). I thought it more flexible to
make this optional but only got part way there with the implementation.

> >    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.

Agreed. So are send handlers always required ? I just continued with
thinking that if receive handlers are optional, might send ones be too
in certain cases ? 

> 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.

Yes, that's a better solution.

> >    /* 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.

-- Hal