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. > 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
