[openib-general] Re: [PATCH] osm: segfault fix in osm_get_gid_by_mad_addr (take 2)
On Mon, 2006-06-05 at 08:34, Eitan Zahavi wrote: > Hi Hal > > I got a report regarding crashes in osm_get_gid_by_mad_addr. > It was missing a check on p_port looked up by LID. The affected > flows are reports and multicast joins. > > The fix modified the function to return status (instead of GID). > I did run some simulation flows after the fix but please double > check before commit. > > This time I hope I did not missed anything > > Eitan > > Signed-off-by: Eitan Zahavi <[EMAIL PROTECTED]> Thanks. Applied (with some cosmetic changes) to both trunk and 1.0 branch. -- Hal ___ openib-general mailing list openib-general@openib.org http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
[openib-general] RE: [PATCH] osm: segfault fix in osm_get_gid_by_mad_addr
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)))
> > {
> >
___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
[openib-general] Re: [PATCH] osm: segfault fix in osm_get_gid_by_mad_addr
Hi Eitan,
On Mon, 2006-06-05 at 05:36, Eitan Zahavi wrote:
> Hi Hal
>
> I got a report regarding crashes in osm_get_gid_by_mad_addr.
> It was missing a check on p_port looked up by LID. The affected
> flows are reports and multicast joins.
>
> The fix modified the function to return status (instead of GID).
> I did run some simulation flows after the fix but please double
> check before commit.
See comments below.
> Eitan
>
> Signed-off-by: Eitan Zahavi <[EMAIL PROTECTED]>
>
> Index: include/opensm/osm_subnet.h
> ===
> --- include/opensm/osm_subnet.h (revision 7542)
> +++ include/opensm/osm_subnet.h (working copy)
> @@ -770,11 +770,12 @@ struct _osm_port;
> *
> * SYNOPSIS
> */
> -ib_gid_t
> +ib_api_status_t
> osm_get_gid_by_mad_addr(
> IN struct _osm_log *p_log,
> IN const osm_subn_t *p_subn,
> - IN const struct _osm_mad_addr *p_mad_addr );
> + IN const struct _osm_mad_addr *p_mad_addr,
> + OUT ib_gid_t*p_gid);
> /*
> * PARAMETERS
> * p_log
> @@ -786,8 +787,11 @@ osm_get_gid_by_mad_addr(
> *p_mad_addr
> *[in] Pointer to mad address object.
> *
> +* p_gid
> +* [out] Pointer to teh GID structure to fill in.
> +*
> * RETURN VALUES
> -*Requestor gid object if found. Null otherwise.
> +*IB_SUCCESS if was able to find the GID by address given
> *
> * NOTES
> *
> Index: opensm/osm_subnet.c
> ===
> --- opensm/osm_subnet.c (revision 7670)
> +++ opensm/osm_subnet.c (working copy)
> @@ -236,16 +236,24 @@ osm_subn_init(
>
> /**
> **/
> -ib_gid_t
> +ib_api_status_t
> osm_get_gid_by_mad_addr(
>IN osm_log_t*p_log,
>IN const osm_subn_t *p_subn,
> - IN const osm_mad_addr_t *p_mad_addr )
> + IN const osm_mad_addr_t *p_mad_addr,
> + OUT ib_gid_t*p_gid)
> {
>const cl_ptr_vector_t* p_tbl;
>const osm_port_t* p_port = NULL;
>const osm_physp_t* p_physp = NULL;
> - ib_gid_trequest_gid;
> +
> + if ( p_gid == NULL )
> + {
> +osm_log( p_log, OSM_LOG_ERROR,
> + "osm_get_gid_by_mad_addr: ERR 7505 "
> + "Provided output GID is NULL\n");
> +return(IB_INVALID_PARAMETER);
> + }
>
>/* Find the port gid of the request in the subnet */
>p_tbl = &p_subn->port_lid_tbl;
> @@ -256,9 +264,18 @@ osm_get_gid_by_mad_addr(
>cl_ntoh16(p_mad_addr->dest_lid))
>{
> p_port = cl_ptr_vector_get( p_tbl, cl_ntoh16(p_mad_addr->dest_lid) );
> +if ( p_port == NULL )
> +{
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "osm_get_gid_by_mad_addr: "
> + "Did not find any port with LID: 0x%X\n",
> + cl_ntoh16(p_mad_addr->dest_lid)
> + );
> + return(IB_INVALID_PARAMETER);
> +}
> 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 ?
> @@ -270,7 +287,7 @@ osm_get_gid_by_mad_addr(
> );
>}
>
> - return request_gid;
> + return( IB_SUCCESS );
> }
>
> /**
> Index: opensm/osm_sa_informinfo.c
> ===
> --- opensm/osm_sa_informinfo.c(revision 7670)
> +++ opensm/osm_sa_informinfo.c(working copy)
> @@ -348,6 +348,7 @@ osm_infr_rcv_process_set_method(
>uint8_t subscribe;
>ib_net32_t qpn;
>uint8_t resp_time_val;
> + ib_api_status_t res;
>
>OSM_LOG_ENTER( p_rcv->p_log, osm_infr_rcv_process_set_method );
>
> @@ -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 ?
> + {
> +osm_log( p_rcv->p_log, OSM_LOG_ERROR,
> + "osm_infr_rcv_process_set_method: ERR 4308 "
> + "Got Subscribe Request from unknown LID: 0x%04X\n",
> + cl_ntoh16(p_madw->mad_addr.dest_lid)
> + );
> +osm_sa_send_error(
> + p_rcv->p_resp,
> + p_madw,
> +
