[openib-general] RE: [PATCH] osm: segfault fix in osm_get_gid_by_mad_addr

[Eitan Zahavi]

Hi Hal,

I will re-send the patch with fixes.
I also replied to the comments below

> See comments below.
> 
> >      p_physp = osm_port_get_phys_ptr( p_port,
p_port->default_port_num);
> > -    request_gid.unicast.interface_id = p_physp->port_guid;
> > -    request_gid.unicast.prefix = p_subn->opt.subnet_prefix;
> > +    p_gid->unicast.interface_id = p_physp->port_guid;
> > +    p_gid->unicast.prefix = p_subn->opt.subnet_prefix;
> >    }
> >    else
> >    {
> 
> Isn't an error status needed to be returned for this else ?
[EZ] Correct
> 
> > @@ -382,8 +383,24 @@ osm_infr_rcv_process_set_method(
> >    inform_info_rec.inform_record.subscriber_enum = 0;
> >
> >    /* update the subscriber GID according to mad address */
> > -  inform_info_rec.inform_record.subscriber_gid =
> > -    osm_get_gid_by_mad_addr( p_rcv->p_log, p_rcv->p_subn, &p_madw-
> >mad_addr );
> > +  res = osm_get_gid_by_mad_addr(
> > +    p_rcv->p_log,
> > +    p_rcv->p_subn,
> > +    &p_madw->mad_addr,
> > +    &inform_info_rec.inform_record.subscriber_gid);
> > +  if ( res != NULL )
> 
> Should this be IB_SUCCESS rather than NULL ?
[EZ] True.
> 
> > +  {
> >     * MODIFICATIONS DONE ON INCOMING REQUEST:
> > Index: opensm/osm_sa_mcmember_record.c
> > ===================================================================
> > --- opensm/osm_sa_mcmember_record.c	(revision 7670)
> > +++ opensm/osm_sa_mcmember_record.c	(working copy)
> > @@ -437,12 +437,21 @@ __add_new_mgrp_port(
> >  {
> >    boolean_t proxy_join;
> >    ib_gid_t requester_gid;
> > +  ib_api_status_t res;
> >
> >    /* set the proxy_join if the requester gid is not identical to
the
> >       joined gid */
> > -  requester_gid = osm_get_gid_by_mad_addr( p_rcv->p_log,
> > +  res = osm_get_gid_by_mad_addr( p_rcv->p_log,
> >                                             p_rcv->p_subn,
> > -                                           p_mad_addr );
> > +                                 p_mad_addr, &requester_gid );
> > +  if ( res != IB_SUCCESS )
> > +  {
> > +    osm_log( p_rcv->p_log, OSM_LOG_ERROR,
> > +             "__add_new_mgrp_port: ERR 1B22: "
> > +             "Could not find GUID for requestor.\n" );
> 
> ERR 1B22 is already in use.
[EZ] OK last was 1B28 using 1B29
> 
> > +
> > +    return IB_INVALID_PARAMETER;
> > +  }
> 
> Also, based on this change, the caller of __add_new_mgrp_port should
not
> just send SA error with IB_SA_MAD_STATUS_NO_RESOURCES but rather base
it
> off the error status now.
[EZ] Correct. But I think there is no error message that fits exactly
the case where the requester is not known to the SM. I will use invalid
parameter.
> 
> -- Hal
> 
> >    if (!memcmp(&p_recvd_mcmember_rec->port_gid, &requester_gid,
> >                sizeof(ib_gid_t)))
> > @@ -755,6 +764,7 @@ __validate_modify(IN osm_mcmr_recv_t* co
> >    ib_net64_t portguid;
> >    ib_gid_t request_gid;
> >    osm_physp_t* p_request_physp;
> > +  ib_api_status_t res;
> >
> >    portguid = p_recvd_mcmember_rec->port_gid.unicast.interface_id;
> >
> > @@ -775,9 +785,19 @@ __validate_modify(IN osm_mcmr_recv_t* co
> >    {
> >      /* The proxy_join is not set. Modifying can by done only
> >         if the requester GID == PortGID */
> > -    request_gid = osm_get_gid_by_mad_addr(p_rcv->p_log,
> > +    res = osm_get_gid_by_mad_addr(p_rcv->p_log,
> >                                            p_rcv->p_subn,
> > -                                          p_mad_addr );
> > +                                  p_mad_addr,
> > +                                  &request_gid);
> > +
> > +    if ( res != IB_SUCCESS )
> > +    {
> > +      osm_log( p_rcv->p_log, OSM_LOG_DEBUG,
> > +               "__validate_modify: "
> > +               "Could not find any port by given request
address.\n"
> > +               );
> > +      return FALSE;
> > +    }
> >
> >      if (memcmp(&((*pp_mcm_port)->port_gid), &request_gid,
sizeof(ib_gid_t)))
> >      {
> >