[ofa-general] Re: [PATCH] opensm: Change ib_smp_init_new to return success/failure status

[Hal Rosenstock]

On Sun, Aug 2, 2009 at 6:59 AM, Hal Rosenstock <hal.rosenstock at gmail.com>wrote:

> HI Sasha,
>
>  On Sun, Aug 2, 2009 at 6:32 AM, Sasha Khapyorsky <sashak at voltaire.com>wrote:
>
>> Hi Hal,
>>
>> On 09:53 Fri 31 Jul     , Hal Rosenstock wrote:
>> >
>> > based on valid/invalid hop count rather than relying on debug assert
>>
>> When could an invalid hop count be passed to this function?
>
>
> Some out of tree user.
>

Also, I think opensm can also do this now with such topologies (without some
other changes which are in the pipe).

-- Hal


>
>
>> And what could happen?
>
>
> It writes past the end of the path array.
>
>
>>
>> ib_smp_init_new() is a simple structure fill-up helper (inlined and
>> defined in header file) and I don't think that we need to check
>> parameters there.
>>
>> This patch also introduces sort of inconsistency - a hop count is checked
>> and other parameters aren't.
>
>
> It's to protect against writing past end of array. Do any other parameters
> need checking ? I think they just result in some timeout condition
> resulting.
>
> -- Hal
>
>
>>
>>
>> > Handle invalid status appropriate in callers of ib_smp_init_new
>> >
>> > Signed-off-by: Hal Rosenstock <hal.rosenstock at gmail.com>
>> > ---
>> > diff --git a/opensm/include/iba/ib_types.h
>> b/opensm/include/iba/ib_types.h
>> > index beb7492..6668d96 100644
>> > --- a/opensm/include/iba/ib_types.h
>> > +++ b/opensm/include/iba/ib_types.h
>> > @@ -4091,11 +4092,11 @@ static inline boolean_t OSM_API ib_smp_is_d(IN
>> const ib_smp_t * const p_smp)
>> >  *
>> >  * TODO
>> >  *    This is too big for inlining, but leave it here for now
>> > -*    since there is not yet another convient spot.
>> > +*    since there is not yet another convenient spot.
>> >  *
>> >  * SYNOPSIS
>> >  */
>> > -static inline void OSM_API
>> > +static inline boolean_t OSM_API
>> >  ib_smp_init_new(IN ib_smp_t * const p_smp,
>> >               IN const uint8_t method,
>> >               IN const ib_net64_t trans_id,
>> > @@ -4107,7 +4108,9 @@ ib_smp_init_new(IN ib_smp_t * const p_smp,
>> >               IN const ib_net16_t dr_slid, IN const ib_net16_t dr_dlid)
>> >  {
>> >       CL_ASSERT(p_smp);
>> > -     CL_ASSERT(hop_count < IB_SUBNET_PATH_HOPS_MAX);
>> > +
>> > +     if (hop_count >= IB_SUBNET_PATH_HOPS_MAX)
>> > +             return FALSE;
>> >       p_smp->base_ver = 1;
>> >       p_smp->mgmt_class = IB_MCLASS_SUBN_DIR;
>> >       p_smp->class_ver = 1;
>> > @@ -4130,6 +4133,7 @@ ib_smp_init_new(IN ib_smp_t * const p_smp,
>> >
>> >       /* copy the path */
>> >       memcpy(&p_smp->initial_path, path_out,
>> sizeof(p_smp->initial_path));
>> > +     return TRUE;
>> >  }
>> >
>> >  /*
>> > diff --git a/opensm/opensm/osm_req.c b/opensm/opensm/osm_req.c
>> > index be9a92b..7934173 100644
>> > --- a/opensm/opensm/osm_req.c
>> > +++ b/opensm/opensm/osm_req.c
>> > @@ -102,14 +102,21 @@ osm_req_get(IN osm_sm_t * sm,
>> >               ib_get_sm_attr_str(attr_id), cl_ntoh16(attr_id),
>> >               cl_ntoh32(attr_mod), cl_ntoh64(tid));
>> >
>> > -     ib_smp_init_new(osm_madw_get_smp_ptr(p_madw),
>> > -                     IB_MAD_METHOD_GET,
>> > -                     tid,
>> > -                     attr_id,
>> > -                     attr_mod,
>> > -                     p_path->hop_count,
>> > -                     sm->p_subn->opt.m_key,
>> > -                     p_path->path, IB_LID_PERMISSIVE,
>> IB_LID_PERMISSIVE);
>> > +     if (!ib_smp_init_new(osm_madw_get_smp_ptr(p_madw),
>> > +                          IB_MAD_METHOD_GET,
>> > +                          tid,
>> > +                          attr_id,
>> > +                          attr_mod,
>> > +                          p_path->hop_count,
>> > +                          sm->p_subn->opt.m_key,
>> > +                          p_path->path,
>> > +                          IB_LID_PERMISSIVE, IB_LID_PERMISSIVE)) {
>> > +             OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 1108: "
>> > +                     "ib_smp_init_new failed: hop count %d\n",
>> > +                     p_path->hop_count);
>>
>> This is assumption on how ib_smp_init_new() is actually implemented -
>> not perfect.
>>
>> Sasha
>>  _______________________________________________
>> general mailing list
>> general at lists.openfabrics.org
>> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>>
>> To unsubscribe, please visit
>> http://openib.org/mailman/listinfo/openib-general
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/general/attachments/20090802/358710bc/attachment.html>