On Sun, Aug 2, 2009 at 6:59 AM, Hal Rosenstock <[email protected]>wrote:
> HI Sasha, > > On Sun, Aug 2, 2009 at 6:32 AM, Sasha Khapyorsky <[email protected]>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 <[email protected]> >> > --- >> > 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 >> [email protected] >> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general >> >> To unsubscribe, please visit >> http://openib.org/mailman/listinfo/openib-general >> > >
_______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
