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

[Fabian Tillier]

Hi Tzachi,

On 4/4/06, Tzachi Dar <tzachid at mellanox.co.il> wrote:
>
> 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.

__grow_mad_pool takes a pointer to a pointer to a mad as second
parameter, which it uses to return a MAD from the newly grown pool, if
growth succeeds.  This newly MAD is allocated under lock, so there's
no synchronization issue here.  See line 1121.

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

You lost me here.  p_mad_item is not used after the list is grown. 
There is a bug, however, that p_mad_element is not updated if the pool
is grown, so that will use a bad pointer at line 819, but that fix is
trivial and in an error case that you can't hit for MADs used on
QP0/QP1.

What is the problem you're trying to solve?

- Fab