[openib-general] Re: [PATCH] Opensm - bug in osm_sa_path_record with 0 records
Hal Rosenstock
halr at voltaire.com
Sat Nov 5 07:40:14 PST 2005
Hi Yael,
On Thu, 2005-11-03 at 11:51, Hal Rosenstock wrote:
I committed this with minor format changes. Any changes (based on
questions in my emails) should be incremental to this. Thanks.
-- Hal
> 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 )
> > >
>
> _______________________________________________
> openib-general mailing list
> openib-general at openib.org
> http://openib.org/mailman/listinfo/openib-general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
More information about the general
mailing list