[Openib-windows] Wrong allocation of mads in al_mad_pool.c (user mode) line 800

[Tzachi Dar]

Hi Fab,
 
I think that the code in the function __get_mad_element has a bug.
 
Here is the problematic code:
 
 /* Obtain a MAD item from the stack. */
 cl_spinlock_acquire( &h_pool->obj.lock );
 p_mad_item = cl_qlist_remove_head( &h_pool->mad_stack );
 p_mad_element = PARENT_STRUCT( p_mad_item, al_mad_element_t, list_item
);
 p_mad = PARENT_STRUCT( p_mad_element, mad_item_t, al_mad_element );
 if( p_mad_item == cl_qlist_end( &h_pool->mad_stack ) )
 {
  /* The stack was empty.  Grow the pool and obtain a new item. */
  cl_spinlock_release( &h_pool->obj.lock );
  status = __grow_mad_pool( h_pool, &p_mad );
  if( status != IB_SUCCESS )
  {
   CL_TRACE_EXIT( AL_DBG_ERROR, g_al_dbg_lvl,
    ("grow_mad_pool failed with status %s.\n",
    ib_get_err_str(status)) );
   return status;
  }
 }
 else
 {
  cl_spinlock_release( &h_pool->obj.lock );
 }

As you can see, if there are no items in the pool, the condition in if(
p_mad_item == cl_qlist_end( &h_pool->mad_stack ) )
will be true, and we will try to enter the if. The comment there says
that  /* The stack was empty.  Grow the pool and obtain a new item. */.
However, IMHO, this is not the correct implementation since after the
pool has grown, we don't ask for a new item from the pool. 
 
I also think, that the approach that cl_qlist_remove_head returns one
item passed the end, and if we enlarge the pool this object will be
valid is also not correct since the lock is being freed and only later
taken inside __grow_mad_pool which might already be too late.
 
Thanks
Tzachi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20060404/1461d2cd/attachment.html>