[openib-general] Re: [PATCH] osm: segfault fix in osm_get_gid_by_mad_addr
Hal Rosenstock
halr at voltaire.com
Mon Jun 5 03:07:57 PDT 2006
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 <eitan at mellanox.co.il>
>
> 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_t request_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,
> + IB_SA_MAD_STATUS_REQ_INVALID);
> + goto Exit;
> + }
>
> /*
> * 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.
> +
> + 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.
-- 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)))
> {
>
More information about the general
mailing list