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

[Hal Rosenstock]

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.


> 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/1ef72d67/attachment.html>