[openib-general] Re: [PATCH] Opensm - bug in osm_sa_path_record with 0 records

[Hal Rosenstock]

One additional comment on this:

On Thu, 2005-11-03 at 08:50, Hal Rosenstock wrote:
> On Thu, 2005-11-03 at 08:07, Yael Kalka wrote:
> > Hi Hal,
> > 
> > During some testing of path record we found a bug in the code.
> > If the number of records return is zero, then there is clearing of 
> > non allocated memory.
> > I've added some changes to the __osm_pr_rcv_respond function, to match
> > other sa responses.
> > Attached is a patch to fix it.
> 
> A couple of minor comments below.
> 
> -- Hal
> 
> > Thanks,
> > Yael
> > 
> > Thanks,
> > Yael
> > 
> > Signed-off-by:  Yael Kalka <yael at mellanox.co.il>
> > 
> > Index: opensm/osm_sa_path_record.c
> > ===================================================================
> > --- opensm/osm_sa_path_record.c	(revision 3955)
> > +++ opensm/osm_sa_path_record.c	(working copy)
> > @@ -1448,7 +1448,7 @@ __osm_pr_rcv_respond(
> >    osm_madw_t*              p_resp_madw;
> >    const ib_sa_mad_t*       p_sa_mad;
> >    ib_sa_mad_t*          p_resp_sa_mad;
> > -  size_t                num_rec, num_copied;
> > +  size_t                num_rec, num_copied, pre_trim_num_rec;
> >  #ifndef VENDOR_RMPP_SUPPORT
> >    size_t		trim_num_rec;
> >  #endif
> > @@ -1456,6 +1456,7 @@ __osm_pr_rcv_respond(
> >    ib_api_status_t          status;
> >    const ib_sa_mad_t*    p_rcvd_mad = osm_madw_get_sa_mad_ptr( p_madw );
> >    osm_pr_item_t*        p_pr_item;
> > +  uint32_t              i;
> >  
> >    OSM_LOG_ENTER( p_rcv->p_log, __osm_pr_rcv_respond );
> >  
> > @@ -1483,6 +1484,7 @@ __osm_pr_rcv_respond(
> >      goto Exit;
> >    }
> >  
> > +  pre_trim_num_rec = num_rec;
> >  #ifndef VENDOR_RMPP_SUPPORT
> >    trim_num_rec = (MAD_BLOCK_SIZE - IB_SA_MAD_HDR_SIZE) / sizeof(ib_path_rec_t);
> >    if (trim_num_rec < num_rec)
> > @@ -1495,11 +1497,15 @@ __osm_pr_rcv_respond(
> >    }
> >  #endif
> >  
> > -  if( osm_log_is_active( p_rcv->p_log, OSM_LOG_DEBUG ) )
> > -  {
> >      osm_log( p_rcv->p_log, OSM_LOG_DEBUG,
> >               "__osm_pr_rcv_respond: "
> >               "Generating response with %u records.\n", num_rec );
> > +
> > +  if ((p_rcvd_mad->method == IB_MAD_METHOD_GET) && (num_rec == 0))
> > +  {
> > +    osm_sa_send_error( p_rcv->p_resp, p_madw,
> > +                       IB_SA_MAD_STATUS_NO_RECORDS );
> > +    goto Exit;
> >    }
> 
> This can be moved up immediately after the C15-0.1.30 clause, OK ?
> 
> >    /*
> > @@ -1514,6 +1520,16 @@ __osm_pr_rcv_respond(
> >      osm_log( p_rcv->p_log, OSM_LOG_ERROR,
> >               "__osm_pr_rcv_respond: ERR 1F14: "
> >               "Unable to allocate MAD.\n" );
> > +
> > +    for( i = 0; i < num_rec; i++ )
> > +    {
> > +      p_pr_item = (osm_pr_item_t*)cl_qlist_remove_head( p_list );
> > +      cl_qlock_pool_put( &p_rcv->pr_pool, &p_pr_item->pool_item );
> > +    }
> > +
> > +    osm_sa_send_error( p_rcv->p_resp, p_madw,
> > +                       IB_SA_MAD_STATUS_NO_RESOURCES );
> > +
> 
> osm_sa_send_error also attempts to get a MAD from the pool. Is there a
> chance this succeeds after the one in this routine fails ? (Should this
> be eliminated ?)
> 
> >      goto Exit;
> >    }
> >  
> > @@ -1528,6 +1544,8 @@ __osm_pr_rcv_respond(
> >    p_resp_sa_mad->attr_offset =
> >      ib_get_attr_offset( sizeof(ib_path_rec_t) );
> >  
> > +  p_resp_pr = (ib_path_rec_t*)ib_sa_mad_get_payload_ptr( p_resp_sa_mad );
> > +
> >  #ifndef VENDOR_RMPP_SUPPORT
> >    /* we support only one packet RMPP - so we will set the first and
> >       last flags for gettable */
> > @@ -1542,37 +1560,19 @@ __osm_pr_rcv_respond(
> >      p_resp_sa_mad->rmpp_flags = IB_RMPP_FLAG_ACTIVE;
> >  #endif
> >  
> > -  p_resp_pr = (ib_path_rec_t*)ib_sa_mad_get_payload_ptr( p_resp_sa_mad );
> > -
> > -  if ( num_rec == 0 )
> > -  {
> > -    if (p_resp_sa_mad->method == IB_MAD_METHOD_GET_RESP)
> > -      p_resp_sa_mad->status = IB_SA_MAD_STATUS_NO_RECORDS;
> > -    cl_memclr( p_resp_pr, sizeof(*p_resp_pr) );
> > -  }
> > -  else
> > +  for ( i = 0; i < pre_trim_num_rec; i++ )
> >    {
> >      p_pr_item = (osm_pr_item_t*)cl_qlist_remove_head( p_list );
> > -
> > -    /* we need to track the number of copied items so we can
> > -     * stop the copy - but clear them all
> > -     */
> > -    num_copied = 0;
> > -
> > -    while( p_pr_item != (osm_pr_item_t*)cl_qlist_end( p_list ) )
> > -    {
> > -      /*  Copy the Path Records from the list into the MAD */
> > -      if (num_copied < num_rec)
> > -      {
> > +    /* copy only if not trimmed */
> > +    if (i < num_rec)
> >          *p_resp_pr = p_pr_item->path_rec;
> > -        num_copied++;
> > -      }
> > +
> >        cl_qlock_pool_put( &p_rcv->pr_pool, &p_pr_item->pool_item );
> >        p_resp_pr++;
> > -      p_pr_item = (osm_pr_item_t*)cl_qlist_remove_head( p_list );
> > -    }
> >    }

Should p_resp_pr only be incremented if i < num_recs ?

Also, these comments apply to all the other SA record code as well.

-- Hal

> > +  CL_ASSERT( cl_is_qlist_empty( p_list ) );
> > +
> >    status = osm_vendor_send( p_resp_madw->h_bind, p_resp_madw, FALSE );
> >  
> >    if( status != IB_SUCCESS )
> >