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

[Tzachi Dar]

OK, I understand my mistake now.

There wasn't a particular problem that I was trying to solve. When
looking for the error in handling mads that were received with GRH, I
saw this code, and I thought there was an error here.

Thanks again
Tzachi

> -----Original Message-----
> From: ftillier.sst at gmail.com [mailto:ftillier.sst at gmail.com] 
> On Behalf Of Fabian Tillier
> Sent: Tuesday, April 04, 2006 7:26 PM
> To: Tzachi Dar
> Cc: openib-windows at openib.org
> Subject: Re: [Openib-windows] Wrong allocation of mads in 
> al_mad_pool.c (user mode) line 800
> 
> 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
> 
> 
>