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

[Hal Rosenstock]

Hi Yael,
 
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 );
> -    }
>    }
> 
> +  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 )
>