[PATCH] opensm: bug in trap report for MC create(66) and delete(67) traps

[PATCH] opensm: bug in trap report for MC create(66) and delete(67) traps

[Eli Dorfman (Voltaire)]

In this case the GID in the data details is the MGID and
not the source gid. So the SM gid should be taken as the source gid.
There was also an error in comparing the subnet prefix and guid due to
host/network order mismatch.

Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
---
 opensm/opensm/osm_inform.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
index 8108213..5f48376 100644
--- a/opensm/opensm/osm_inform.c
+++ b/opensm/opensm/osm_inform.c
@@ -460,18 +460,16 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
 		}
 	}
 
-	/* Check if there is a pkey match. o13-17.1.1 */
-	/* Check if the issuer of the trap is the SM. If it is, then the gid
+	/* In case of GID IN(64) or GID OUT(65) traps the source gid
 	   comparison should be done on the trap source (saved as the gid in the
 	   data details field).
-	   If the issuer gid is not the SM - then it is the guid of the trap
-	   source */
-	if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
-	     p_subn->opt.subnet_prefix)
-	    && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
-		p_subn->sm_port_guid))
-		/* The issuer is the SM then this is trap 64-67 - compare the gid
-		   with the gid saved on the data details */
+	   In all other cases the issuer gis is the trap source.
+	   This is also the case for MC CREATE(66) and MC DELETE(67) where the
+	   data details gid is MGID */
+	if (p_ntc->g_or_v.generic.trap_num == 64 ||
+	    p_ntc->g_or_v.generic.trap_num == 65 )
+		/* The issuer of these traps is the SM so source_gid 
+		   is the gid saved on the data details */
 		source_gid = p_ntc->data_details.ntc_64_67.gid;
 	else
 		source_gid = p_ntc->issuer_gid;
@@ -495,6 +493,7 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
 		goto Exit;
 	}
 
+	/* Check if there is a pkey match. o13-17.1.1 */
 	if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
 		OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
 		/* According to o13-17.1.2 - If this informInfo does not have
-- 
1.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] opensm: bug in trap report for MC create(66) and delete(67) traps

[Sasha Khapyorsky]

Hi Eli,

On 16:13 Tue 02 Feb     , Eli Dorfman (Voltaire) wrote:
> 
> In this case the GID in the data details is the MGID and
> not the source gid. So the SM gid should be taken as the source gid.

There the source port is detected in order P_Key matching check between
source and trap destination port.

So in case of 66,67 trap you will just check P_Key match between trap
destination port and SM port, which is likely useless due to fact of
already working SA communication between both.

What should be really checked in this case is a good question. At least
I cannot find some explicit explanations in the spec.

I would guess that the needed check is that trap destination port has MC
group's P_Key (as present in MCMember Record) value.

Thoughts?

> There was also an error in comparing the subnet prefix and guid due to
> host/network order mismatch.

Agree about this.

> 
> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
> ---
>  opensm/opensm/osm_inform.c |   19 +++++++++----------
>  1 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
> index 8108213..5f48376 100644
> --- a/opensm/opensm/osm_inform.c
> +++ b/opensm/opensm/osm_inform.c
> @@ -460,18 +460,16 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>  		}
>  	}
>  
> -	/* Check if there is a pkey match. o13-17.1.1 */
> -	/* Check if the issuer of the trap is the SM. If it is, then the gid
> +	/* In case of GID IN(64) or GID OUT(65) traps the source gid
>  	   comparison should be done on the trap source (saved as the gid in the
>  	   data details field).
> -	   If the issuer gid is not the SM - then it is the guid of the trap
> -	   source */
> -	if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
> -	     p_subn->opt.subnet_prefix)
> -	    && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
> -		p_subn->sm_port_guid))
> -		/* The issuer is the SM then this is trap 64-67 - compare the gid
> -		   with the gid saved on the data details */
> +	   In all other cases the issuer gis is the trap source.
> +	   This is also the case for MC CREATE(66) and MC DELETE(67) where the
> +	   data details gid is MGID */
> +	if (p_ntc->g_or_v.generic.trap_num == 64 ||
> +	    p_ntc->g_or_v.generic.trap_num == 65 )

ib_mad_notice_attr_t.g_or_v.generic.trap_num is encoded in network byte
order.

Sasha

> +		/* The issuer of these traps is the SM so source_gid 
> +		   is the gid saved on the data details */
>  		source_gid = p_ntc->data_details.ntc_64_67.gid;
>  	else
>  		source_gid = p_ntc->issuer_gid;
> @@ -495,6 +493,7 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>  		goto Exit;
>  	}
>  
> +	/* Check if there is a pkey match. o13-17.1.1 */
>  	if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>  		OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>  		/* According to o13-17.1.2 - If this informInfo does not have
> -- 
> 1.5.5
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] opensm: bug in trap report for MC create(66) and delete(67) traps

[Eli Dorfman (Voltaire)]

Subject: [PATCH] Wrong handling of MC create and delete traps

For these traps the GID in the data details is the MGID and
not the source port gid.
So the SM should check that subscriber port has the pkey of the MC group.
There was also an error in comparing the subnet prefix and guid due to
host/network order mismatch.

Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
---
 opensm/opensm/osm_inform.c |  151 ++++++++++++++++++++++++++++---------------
 1 files changed, 98 insertions(+), 53 deletions(-)

diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
index 8108213..ae4fe71 100644
--- a/opensm/opensm/osm_inform.c
+++ b/opensm/opensm/osm_inform.c
@@ -341,6 +341,103 @@ Exit:
 	return status;
 }
 
+static int is_access_permitted( osm_infr_t *p_infr_rec,
+				osm_infr_match_ctxt_t *p_infr_match )
+{
+	cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
+	ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
+	ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
+	uint16_t trap_num = cl_ntoh16(p_ntc->g_or_v.generic.trap_num);
+	osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
+	osm_log_t *p_log = p_infr_rec->sa->p_log;
+	char gid_str[INET6_ADDRSTRLEN];
+	osm_mgrp_t *p_mgrp;
+	ib_gid_t source_gid;
+	osm_port_t *p_src_port;
+	osm_port_t *p_dest_port;
+
+	/* In case of GID_IN(64) or GID_OUT(65) traps the source gid
+	   comparison should be done on the trap source (saved as the gid in the
+	   data details field).
+	   For traps MC_CREATE(66) or MC_DELETE(67) the data details gid is
+	   the MGID. We need to check whether subscriber has the pky of 
+	   the MC group.
+	   In all other cases the issuer gis is the trap source.
+	*/
+	if (trap_num >= 64 && trap_num <= 67 )
+		/* The issuer of these traps is the SM so source_gid 
+		   is the gid saved on the data details */
+		source_gid = p_ntc->data_details.ntc_64_67.gid;
+	else
+		source_gid = p_ntc->issuer_gid;
+
+	p_dest_port =
+	    cl_ptr_vector_get(&p_subn->port_lid_tbl,
+			      cl_ntoh16(p_infr_rec->report_addr.dest_lid));
+	if (!p_dest_port) {
+		OSM_LOG(p_log, OSM_LOG_INFO,
+			"Cannot find destination port with LID:%u\n",
+			cl_ntoh16(p_infr_rec->report_addr.dest_lid));
+		goto Exit;
+	}
+
+	switch (trap_num) {
+		case 66:
+		case 67:
+			p_mgrp = osm_get_mgrp_by_mgid(p_subn, &source_gid);
+			if (!p_mgrp) {
+				OSM_LOG(p_log, OSM_LOG_INFO,
+					"Cannot find MGID %s\n",
+					inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str));
+				goto Exit;
+			}
+			
+			if (!osm_physp_has_pkey(p_log, 
+						p_mgrp->mcmember_rec.pkey, 
+						p_dest_port->p_physp)) {
+				OSM_LOG(p_log, OSM_LOG_INFO,
+					"MGID %s and port GUID:0x%016" PRIx64 " do not share same pkey\n",
+					inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str),
+					cl_ntoh64(p_dest_port->guid));
+				goto Exit;
+			}
+			break;
+
+		default:
+			p_src_port =
+			    osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
+			if (!p_src_port) {
+				OSM_LOG(p_log, OSM_LOG_INFO,
+					"Cannot find source port with GUID:0x%016" PRIx64 "\n",
+					cl_ntoh64(source_gid.unicast.interface_id));
+				goto Exit;
+			}
+
+
+			/* Check if there is a pkey match. o13-17.1.1 */
+			if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
+				OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
+				/* According to o13-17.1.2 - If this informInfo does not have
+				   lid_range_begin of 0xFFFF, then this informInfo request
+				   should be removed from database */
+				if (p_ii->lid_range_begin != 0xFFFF) {
+					OSM_LOG(p_log, OSM_LOG_VERBOSE,
+						"Pkey mismatch on lid_range_begin != 0xFFFF. "
+						"Need to remove this informInfo from db\n");
+					/* add the informInfo record to the remove_infr list */
+					cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
+				}
+				goto Exit;
+			}
+			break;
+	}
+
+	return 1;
+Exit:
+	return 0;	
+}
+
+
 /**********************************************************************
  * This routine compares a given Notice and a ListItem of InformInfo type.
  * PREREQUISITE:
@@ -351,15 +448,10 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
 {
 	osm_infr_match_ctxt_t *p_infr_match = (osm_infr_match_ctxt_t *) context;
 	ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
-	cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
 	osm_infr_t *p_infr_rec = (osm_infr_t *) p_list_item;
 	ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
 	cl_status_t status = CL_NOT_FOUND;
 	osm_log_t *p_log = p_infr_rec->sa->p_log;
-	osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
-	ib_gid_t source_gid;
-	osm_port_t *p_src_port;
-	osm_port_t *p_dest_port;
 
 	OSM_LOG_ENTER(p_log);
 
@@ -460,55 +552,8 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
 		}
 	}
 
-	/* Check if there is a pkey match. o13-17.1.1 */
-	/* Check if the issuer of the trap is the SM. If it is, then the gid
-	   comparison should be done on the trap source (saved as the gid in the
-	   data details field).
-	   If the issuer gid is not the SM - then it is the guid of the trap
-	   source */
-	if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
-	     p_subn->opt.subnet_prefix)
-	    && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
-		p_subn->sm_port_guid))
-		/* The issuer is the SM then this is trap 64-67 - compare the gid
-		   with the gid saved on the data details */
-		source_gid = p_ntc->data_details.ntc_64_67.gid;
-	else
-		source_gid = p_ntc->issuer_gid;
-
-	p_src_port =
-	    osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
-	if (!p_src_port) {
-		OSM_LOG(p_log, OSM_LOG_INFO,
-			"Cannot find source port with GUID:0x%016" PRIx64 "\n",
-			cl_ntoh64(source_gid.unicast.interface_id));
+	if (!is_access_permitted(p_infr_rec, p_infr_match))
 		goto Exit;
-	}
-
-	p_dest_port =
-	    cl_ptr_vector_get(&p_subn->port_lid_tbl,
-			      cl_ntoh16(p_infr_rec->report_addr.dest_lid));
-	if (!p_dest_port) {
-		OSM_LOG(p_log, OSM_LOG_INFO,
-			"Cannot find destination port with LID:%u\n",
-			cl_ntoh16(p_infr_rec->report_addr.dest_lid));
-		goto Exit;
-	}
-
-	if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
-		OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
-		/* According to o13-17.1.2 - If this informInfo does not have
-		   lid_range_begin of 0xFFFF, then this informInfo request
-		   should be removed from database */
-		if (p_ii->lid_range_begin != 0xFFFF) {
-			OSM_LOG(p_log, OSM_LOG_VERBOSE,
-				"Pkey mismatch on lid_range_begin != 0xFFFF. "
-				"Need to remove this informInfo from db\n");
-			/* add the informInfo record to the remove_infr list */
-			cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
-		}
-		goto Exit;
-	}
 
 	/* send the report to the address provided in the inform record */
 	OSM_LOG(p_log, OSM_LOG_DEBUG, "MATCH! Sending Report...\n");
-- 
1.5.5

There is still a problem with GID OUT trap that is not sent since source port
was already removed.
Patch will follow.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] opensm: bug in trap report for MC create(66) and delete(67) traps

[Hal Rosenstock]

On Thu, Feb 4, 2010 at 12:43 PM, Eli Dorfman (Voltaire)
<dorfman.eli@gmail.com> wrote:
>
> Subject: [PATCH] Wrong handling of MC create and delete traps
>
> For these traps the GID in the data details is the MGID and
> not the source port gid.
> So the SM should check that subscriber port has the pkey of the MC group.
> There was also an error in comparing the subnet prefix and guid due to
> host/network order mismatch.
>
> Signed-off-by: Eli Dorfman <elid@voltaire.com>
> ---
>  opensm/opensm/osm_inform.c |  151 ++++++++++++++++++++++++++++---------------
>  1 files changed, 98 insertions(+), 53 deletions(-)
>
> diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
> index 8108213..ae4fe71 100644
> --- a/opensm/opensm/osm_inform.c
> +++ b/opensm/opensm/osm_inform.c
> @@ -341,6 +341,103 @@ Exit:
>        return status;
>  }
>
> +static int is_access_permitted( osm_infr_t *p_infr_rec,
> +                               osm_infr_match_ctxt_t *p_infr_match )
> +{
> +       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
> +       ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
> +       ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
> +       uint16_t trap_num = cl_ntoh16(p_ntc->g_or_v.generic.trap_num);
> +       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
> +       osm_log_t *p_log = p_infr_rec->sa->p_log;
> +       char gid_str[INET6_ADDRSTRLEN];
> +       osm_mgrp_t *p_mgrp;
> +       ib_gid_t source_gid;
> +       osm_port_t *p_src_port;
> +       osm_port_t *p_dest_port;
> +
> +       /* In case of GID_IN(64) or GID_OUT(65) traps the source gid
> +          comparison should be done on the trap source (saved as the gid in the
> +          data details field).
> +          For traps MC_CREATE(66) or MC_DELETE(67) the data details gid is
> +          the MGID. We need to check whether subscriber has the pky of


                   typo  ^^^^

                           pkey


> +          the MC group.

Shouldn't this be the subscriber has a compatible pkey with MC group ?
The MC group has a full member PKey and the members can be full or
limited.

> +          In all other cases the issuer gis is the trap source.

                                               typo  ^^^
                                                       gid

-- Hal

> +       */
> +       if (trap_num >= 64 && trap_num <= 67 )
> +               /* The issuer of these traps is the SM so source_gid
> +                  is the gid saved on the data details */
> +               source_gid = p_ntc->data_details.ntc_64_67.gid;
> +       else
> +               source_gid = p_ntc->issuer_gid;
> +
> +       p_dest_port =
> +           cl_ptr_vector_get(&p_subn->port_lid_tbl,
> +                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
> +       if (!p_dest_port) {
> +               OSM_LOG(p_log, OSM_LOG_INFO,
> +                       "Cannot find destination port with LID:%u\n",
> +                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
> +               goto Exit;
> +       }
> +
> +       switch (trap_num) {
> +               case 66:
> +               case 67:
> +                       p_mgrp = osm_get_mgrp_by_mgid(p_subn, &source_gid);
> +                       if (!p_mgrp) {
> +                               OSM_LOG(p_log, OSM_LOG_INFO,
> +                                       "Cannot find MGID %s\n",
> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str));
> +                               goto Exit;
> +                       }
> +
> +                       if (!osm_physp_has_pkey(p_log,
> +                                               p_mgrp->mcmember_rec.pkey,
> +                                               p_dest_port->p_physp)) {
> +                               OSM_LOG(p_log, OSM_LOG_INFO,
> +                                       "MGID %s and port GUID:0x%016" PRIx64 " do not share same pkey\n",
> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str),
> +                                       cl_ntoh64(p_dest_port->guid));
> +                               goto Exit;
> +                       }
> +                       break;
> +
> +               default:
> +                       p_src_port =
> +                           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
> +                       if (!p_src_port) {
> +                               OSM_LOG(p_log, OSM_LOG_INFO,
> +                                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
> +                                       cl_ntoh64(source_gid.unicast.interface_id));
> +                               goto Exit;
> +                       }
> +
> +
> +                       /* Check if there is a pkey match. o13-17.1.1 */
> +                       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
> +                               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
> +                               /* According to o13-17.1.2 - If this informInfo does not have
> +                                  lid_range_begin of 0xFFFF, then this informInfo request
> +                                  should be removed from database */
> +                               if (p_ii->lid_range_begin != 0xFFFF) {
> +                                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
> +                                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
> +                                               "Need to remove this informInfo from db\n");
> +                                       /* add the informInfo record to the remove_infr list */
> +                                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
> +                               }
> +                               goto Exit;
> +                       }
> +                       break;
> +       }
> +
> +       return 1;
> +Exit:
> +       return 0;
> +}
> +
> +
>  /**********************************************************************
>  * This routine compares a given Notice and a ListItem of InformInfo type.
>  * PREREQUISITE:
> @@ -351,15 +448,10 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>  {
>        osm_infr_match_ctxt_t *p_infr_match = (osm_infr_match_ctxt_t *) context;
>        ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
> -       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>        osm_infr_t *p_infr_rec = (osm_infr_t *) p_list_item;
>        ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>        cl_status_t status = CL_NOT_FOUND;
>        osm_log_t *p_log = p_infr_rec->sa->p_log;
> -       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
> -       ib_gid_t source_gid;
> -       osm_port_t *p_src_port;
> -       osm_port_t *p_dest_port;
>
>        OSM_LOG_ENTER(p_log);
>
> @@ -460,55 +552,8 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>                }
>        }
>
> -       /* Check if there is a pkey match. o13-17.1.1 */
> -       /* Check if the issuer of the trap is the SM. If it is, then the gid
> -          comparison should be done on the trap source (saved as the gid in the
> -          data details field).
> -          If the issuer gid is not the SM - then it is the guid of the trap
> -          source */
> -       if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
> -            p_subn->opt.subnet_prefix)
> -           && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
> -               p_subn->sm_port_guid))
> -               /* The issuer is the SM then this is trap 64-67 - compare the gid
> -                  with the gid saved on the data details */
> -               source_gid = p_ntc->data_details.ntc_64_67.gid;
> -       else
> -               source_gid = p_ntc->issuer_gid;
> -
> -       p_src_port =
> -           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
> -       if (!p_src_port) {
> -               OSM_LOG(p_log, OSM_LOG_INFO,
> -                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
> -                       cl_ntoh64(source_gid.unicast.interface_id));
> +       if (!is_access_permitted(p_infr_rec, p_infr_match))
>                goto Exit;
> -       }
> -
> -       p_dest_port =
> -           cl_ptr_vector_get(&p_subn->port_lid_tbl,
> -                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
> -       if (!p_dest_port) {
> -               OSM_LOG(p_log, OSM_LOG_INFO,
> -                       "Cannot find destination port with LID:%u\n",
> -                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
> -               goto Exit;
> -       }
> -
> -       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
> -               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
> -               /* According to o13-17.1.2 - If this informInfo does not have
> -                  lid_range_begin of 0xFFFF, then this informInfo request
> -                  should be removed from database */
> -               if (p_ii->lid_range_begin != 0xFFFF) {
> -                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
> -                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
> -                               "Need to remove this informInfo from db\n");
> -                       /* add the informInfo record to the remove_infr list */
> -                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
> -               }
> -               goto Exit;
> -       }
>
>        /* send the report to the address provided in the inform record */
>        OSM_LOG(p_log, OSM_LOG_DEBUG, "MATCH! Sending Report...\n");
> --
> 1.5.5
>
> There is still a problem with GID OUT trap that is not sent since source port
> was already removed.
> Patch will follow.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] opensm: bug in trap report for MC create(66) and delete(67) traps

[Eli Dorfman]

On Thu, Feb 4, 2010 at 10:52 PM, Hal Rosenstock
<hal.rosenstock@gmail.com> wrote:
> On Thu, Feb 4, 2010 at 12:43 PM, Eli Dorfman (Voltaire)
> <dorfman.eli@gmail.com> wrote:
>>
>> Subject: [PATCH] Wrong handling of MC create and delete traps
>>
>> For these traps the GID in the data details is the MGID and
>> not the source port gid.
>> So the SM should check that subscriber port has the pkey of the MC group.
>> There was also an error in comparing the subnet prefix and guid due to
>> host/network order mismatch.
>>
>> Signed-off-by: Eli Dorfman <elid@voltaire.com>
>> ---
>>  opensm/opensm/osm_inform.c |  151 ++++++++++++++++++++++++++++---------------
>>  1 files changed, 98 insertions(+), 53 deletions(-)
>>
>> diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
>> index 8108213..ae4fe71 100644
>> --- a/opensm/opensm/osm_inform.c
>> +++ b/opensm/opensm/osm_inform.c
>> @@ -341,6 +341,103 @@ Exit:
>>        return status;
>>  }
>>
>> +static int is_access_permitted( osm_infr_t *p_infr_rec,
>> +                               osm_infr_match_ctxt_t *p_infr_match )
>> +{
>> +       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>> +       ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>> +       ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>> +       uint16_t trap_num = cl_ntoh16(p_ntc->g_or_v.generic.trap_num);
>> +       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>> +       osm_log_t *p_log = p_infr_rec->sa->p_log;
>> +       char gid_str[INET6_ADDRSTRLEN];
>> +       osm_mgrp_t *p_mgrp;
>> +       ib_gid_t source_gid;
>> +       osm_port_t *p_src_port;
>> +       osm_port_t *p_dest_port;
>> +
>> +       /* In case of GID_IN(64) or GID_OUT(65) traps the source gid
>> +          comparison should be done on the trap source (saved as the gid in the
>> +          data details field).
>> +          For traps MC_CREATE(66) or MC_DELETE(67) the data details gid is
>> +          the MGID. We need to check whether subscriber has the pky of
>
>
>                   typo  ^^^^
>
>                           pkey
>
>
>> +          the MC group.
>
> Shouldn't this be the subscriber has a compatible pkey with MC group ?
> The MC group has a full member PKey and the members can be full or
> limited.

I accept the correction.
Sasha, can you please change this in the commit (only if there are not
other remarks).

BTW, there is no explicit reference in the IB spec for MC group
create/delete trap (at least I didn't find it).

>
>> +          In all other cases the issuer gis is the trap source.
>
>                                               typo  ^^^
>                                                       gid
>

and this typo of course.

Thanks,
Eli
> -- Hal
>
>> +       */
>> +       if (trap_num >= 64 && trap_num <= 67 )
>> +               /* The issuer of these traps is the SM so source_gid
>> +                  is the gid saved on the data details */
>> +               source_gid = p_ntc->data_details.ntc_64_67.gid;
>> +       else
>> +               source_gid = p_ntc->issuer_gid;
>> +
>> +       p_dest_port =
>> +           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>> +                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>> +       if (!p_dest_port) {
>> +               OSM_LOG(p_log, OSM_LOG_INFO,
>> +                       "Cannot find destination port with LID:%u\n",
>> +                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>> +               goto Exit;
>> +       }
>> +
>> +       switch (trap_num) {
>> +               case 66:
>> +               case 67:
>> +                       p_mgrp = osm_get_mgrp_by_mgid(p_subn, &source_gid);
>> +                       if (!p_mgrp) {
>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>> +                                       "Cannot find MGID %s\n",
>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str));
>> +                               goto Exit;
>> +                       }
>> +
>> +                       if (!osm_physp_has_pkey(p_log,
>> +                                               p_mgrp->mcmember_rec.pkey,
>> +                                               p_dest_port->p_physp)) {
>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>> +                                       "MGID %s and port GUID:0x%016" PRIx64 " do not share same pkey\n",
>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str),
>> +                                       cl_ntoh64(p_dest_port->guid));
>> +                               goto Exit;
>> +                       }
>> +                       break;
>> +
>> +               default:
>> +                       p_src_port =
>> +                           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>> +                       if (!p_src_port) {
>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>> +                                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>> +                                       cl_ntoh64(source_gid.unicast.interface_id));
>> +                               goto Exit;
>> +                       }
>> +
>> +
>> +                       /* Check if there is a pkey match. o13-17.1.1 */
>> +                       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>> +                               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>> +                               /* According to o13-17.1.2 - If this informInfo does not have
>> +                                  lid_range_begin of 0xFFFF, then this informInfo request
>> +                                  should be removed from database */
>> +                               if (p_ii->lid_range_begin != 0xFFFF) {
>> +                                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>> +                                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>> +                                               "Need to remove this informInfo from db\n");
>> +                                       /* add the informInfo record to the remove_infr list */
>> +                                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>> +                               }
>> +                               goto Exit;
>> +                       }
>> +                       break;
>> +       }
>> +
>> +       return 1;
>> +Exit:
>> +       return 0;
>> +}
>> +
>> +
>>  /**********************************************************************
>>  * This routine compares a given Notice and a ListItem of InformInfo type.
>>  * PREREQUISITE:
>> @@ -351,15 +448,10 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>  {
>>        osm_infr_match_ctxt_t *p_infr_match = (osm_infr_match_ctxt_t *) context;
>>        ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>> -       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>        osm_infr_t *p_infr_rec = (osm_infr_t *) p_list_item;
>>        ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>        cl_status_t status = CL_NOT_FOUND;
>>        osm_log_t *p_log = p_infr_rec->sa->p_log;
>> -       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>> -       ib_gid_t source_gid;
>> -       osm_port_t *p_src_port;
>> -       osm_port_t *p_dest_port;
>>
>>        OSM_LOG_ENTER(p_log);
>>
>> @@ -460,55 +552,8 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>                }
>>        }
>>
>> -       /* Check if there is a pkey match. o13-17.1.1 */
>> -       /* Check if the issuer of the trap is the SM. If it is, then the gid
>> -          comparison should be done on the trap source (saved as the gid in the
>> -          data details field).
>> -          If the issuer gid is not the SM - then it is the guid of the trap
>> -          source */
>> -       if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
>> -            p_subn->opt.subnet_prefix)
>> -           && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
>> -               p_subn->sm_port_guid))
>> -               /* The issuer is the SM then this is trap 64-67 - compare the gid
>> -                  with the gid saved on the data details */
>> -               source_gid = p_ntc->data_details.ntc_64_67.gid;
>> -       else
>> -               source_gid = p_ntc->issuer_gid;
>> -
>> -       p_src_port =
>> -           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>> -       if (!p_src_port) {
>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>> -                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>> -                       cl_ntoh64(source_gid.unicast.interface_id));
>> +       if (!is_access_permitted(p_infr_rec, p_infr_match))
>>                goto Exit;
>> -       }
>> -
>> -       p_dest_port =
>> -           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>> -                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>> -       if (!p_dest_port) {
>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>> -                       "Cannot find destination port with LID:%u\n",
>> -                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>> -               goto Exit;
>> -       }
>> -
>> -       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>> -               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>> -               /* According to o13-17.1.2 - If this informInfo does not have
>> -                  lid_range_begin of 0xFFFF, then this informInfo request
>> -                  should be removed from database */
>> -               if (p_ii->lid_range_begin != 0xFFFF) {
>> -                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>> -                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>> -                               "Need to remove this informInfo from db\n");
>> -                       /* add the informInfo record to the remove_infr list */
>> -                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>> -               }
>> -               goto Exit;
>> -       }
>>
>>        /* send the report to the address provided in the inform record */
>>        OSM_LOG(p_log, OSM_LOG_DEBUG, "MATCH! Sending Report...\n");
>> --
>> 1.5.5
>>
>> There is still a problem with GID OUT trap that is not sent since source port
>> was already removed.
>> Patch will follow.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH v2] opensm: bug in trap report for MC create(66) and delete(67) traps

[Hal Rosenstock]

On Fri, Feb 5, 2010 at 9:18 AM, Eli Dorfman <dorfman.eli@gmail.com> wrote:
> On Thu, Feb 4, 2010 at 10:52 PM, Hal Rosenstock
> <hal.rosenstock@gmail.com> wrote:
>> On Thu, Feb 4, 2010 at 12:43 PM, Eli Dorfman (Voltaire)
>> <dorfman.eli@gmail.com> wrote:
>>>
>>> Subject: [PATCH] Wrong handling of MC create and delete traps
>>>
>>> For these traps the GID in the data details is the MGID and
>>> not the source port gid.
>>> So the SM should check that subscriber port has the pkey of the MC group.
>>> There was also an error in comparing the subnet prefix and guid due to
>>> host/network order mismatch.
>>>
>>> Signed-off-by: Eli Dorfman <elid@voltaire.com>
>>> ---
>>>  opensm/opensm/osm_inform.c |  151 ++++++++++++++++++++++++++++---------------
>>>  1 files changed, 98 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
>>> index 8108213..ae4fe71 100644
>>> --- a/opensm/opensm/osm_inform.c
>>> +++ b/opensm/opensm/osm_inform.c
>>> @@ -341,6 +341,103 @@ Exit:
>>>        return status;
>>>  }
>>>
>>> +static int is_access_permitted( osm_infr_t *p_infr_rec,
>>> +                               osm_infr_match_ctxt_t *p_infr_match )
>>> +{
>>> +       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>> +       ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>> +       ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>> +       uint16_t trap_num = cl_ntoh16(p_ntc->g_or_v.generic.trap_num);
>>> +       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>> +       osm_log_t *p_log = p_infr_rec->sa->p_log;
>>> +       char gid_str[INET6_ADDRSTRLEN];
>>> +       osm_mgrp_t *p_mgrp;
>>> +       ib_gid_t source_gid;
>>> +       osm_port_t *p_src_port;
>>> +       osm_port_t *p_dest_port;
>>> +
>>> +       /* In case of GID_IN(64) or GID_OUT(65) traps the source gid
>>> +          comparison should be done on the trap source (saved as the gid in the
>>> +          data details field).
>>> +          For traps MC_CREATE(66) or MC_DELETE(67) the data details gid is
>>> +          the MGID. We need to check whether subscriber has the pky of
>>
>>
>>                   typo  ^^^^
>>
>>                           pkey
>>
>>
>>> +          the MC group.
>>
>> Shouldn't this be the subscriber has a compatible pkey with MC group ?
>> The MC group has a full member PKey and the members can be full or
>> limited.
>
> I accept the correction.

Doesn't this require a code change for handling trap cases 66-67 ?

> Sasha, can you please change this in the commit (only if there are not
> other remarks).

Is that what you are asking Sasha to do (beyond the typos) ?

>
> BTW, there is no explicit reference in the IB spec for MC group
> create/delete trap (at least I didn't find it).

Not sure what you mean by this. What didn't you find ?

-- Hal

>
>>
>>> +          In all other cases the issuer gis is the trap source.
>>
>>                                               typo  ^^^
>>                                                       gid
>>
>
> and this typo of course.
>
> Thanks,
> Eli
>> -- Hal
>>
>>> +       */
>>> +       if (trap_num >= 64 && trap_num <= 67 )
>>> +               /* The issuer of these traps is the SM so source_gid
>>> +                  is the gid saved on the data details */
>>> +               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>> +       else
>>> +               source_gid = p_ntc->issuer_gid;
>>> +
>>> +       p_dest_port =
>>> +           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>> +                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>> +       if (!p_dest_port) {
>>> +               OSM_LOG(p_log, OSM_LOG_INFO,
>>> +                       "Cannot find destination port with LID:%u\n",
>>> +                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>> +               goto Exit;
>>> +       }
>>> +
>>> +       switch (trap_num) {
>>> +               case 66:
>>> +               case 67:
>>> +                       p_mgrp = osm_get_mgrp_by_mgid(p_subn, &source_gid);
>>> +                       if (!p_mgrp) {
>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>> +                                       "Cannot find MGID %s\n",
>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str));
>>> +                               goto Exit;
>>> +                       }
>>> +
>>> +                       if (!osm_physp_has_pkey(p_log,
>>> +                                               p_mgrp->mcmember_rec.pkey,
>>> +                                               p_dest_port->p_physp)) {
>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>> +                                       "MGID %s and port GUID:0x%016" PRIx64 " do not share same pkey\n",
>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str),
>>> +                                       cl_ntoh64(p_dest_port->guid));
>>> +                               goto Exit;
>>> +                       }
>>> +                       break;
>>> +
>>> +               default:
>>> +                       p_src_port =
>>> +                           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>> +                       if (!p_src_port) {
>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>> +                                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>> +                                       cl_ntoh64(source_gid.unicast.interface_id));
>>> +                               goto Exit;
>>> +                       }
>>> +
>>> +
>>> +                       /* Check if there is a pkey match. o13-17.1.1 */
>>> +                       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>> +                               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>> +                               /* According to o13-17.1.2 - If this informInfo does not have
>>> +                                  lid_range_begin of 0xFFFF, then this informInfo request
>>> +                                  should be removed from database */
>>> +                               if (p_ii->lid_range_begin != 0xFFFF) {
>>> +                                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>> +                                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>> +                                               "Need to remove this informInfo from db\n");
>>> +                                       /* add the informInfo record to the remove_infr list */
>>> +                                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>> +                               }
>>> +                               goto Exit;
>>> +                       }
>>> +                       break;
>>> +       }
>>> +
>>> +       return 1;
>>> +Exit:
>>> +       return 0;
>>> +}
>>> +
>>> +
>>>  /**********************************************************************
>>>  * This routine compares a given Notice and a ListItem of InformInfo type.
>>>  * PREREQUISITE:
>>> @@ -351,15 +448,10 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>  {
>>>        osm_infr_match_ctxt_t *p_infr_match = (osm_infr_match_ctxt_t *) context;
>>>        ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>> -       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>>        osm_infr_t *p_infr_rec = (osm_infr_t *) p_list_item;
>>>        ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>>        cl_status_t status = CL_NOT_FOUND;
>>>        osm_log_t *p_log = p_infr_rec->sa->p_log;
>>> -       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>> -       ib_gid_t source_gid;
>>> -       osm_port_t *p_src_port;
>>> -       osm_port_t *p_dest_port;
>>>
>>>        OSM_LOG_ENTER(p_log);
>>>
>>> @@ -460,55 +552,8 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>                }
>>>        }
>>>
>>> -       /* Check if there is a pkey match. o13-17.1.1 */
>>> -       /* Check if the issuer of the trap is the SM. If it is, then the gid
>>> -          comparison should be done on the trap source (saved as the gid in the
>>> -          data details field).
>>> -          If the issuer gid is not the SM - then it is the guid of the trap
>>> -          source */
>>> -       if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
>>> -            p_subn->opt.subnet_prefix)
>>> -           && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
>>> -               p_subn->sm_port_guid))
>>> -               /* The issuer is the SM then this is trap 64-67 - compare the gid
>>> -                  with the gid saved on the data details */
>>> -               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>> -       else
>>> -               source_gid = p_ntc->issuer_gid;
>>> -
>>> -       p_src_port =
>>> -           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>> -       if (!p_src_port) {
>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>> -                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>> -                       cl_ntoh64(source_gid.unicast.interface_id));
>>> +       if (!is_access_permitted(p_infr_rec, p_infr_match))
>>>                goto Exit;
>>> -       }
>>> -
>>> -       p_dest_port =
>>> -           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>> -                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>> -       if (!p_dest_port) {
>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>> -                       "Cannot find destination port with LID:%u\n",
>>> -                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>> -               goto Exit;
>>> -       }
>>> -
>>> -       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>> -               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>> -               /* According to o13-17.1.2 - If this informInfo does not have
>>> -                  lid_range_begin of 0xFFFF, then this informInfo request
>>> -                  should be removed from database */
>>> -               if (p_ii->lid_range_begin != 0xFFFF) {
>>> -                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>> -                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>> -                               "Need to remove this informInfo from db\n");
>>> -                       /* add the informInfo record to the remove_infr list */
>>> -                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>> -               }
>>> -               goto Exit;
>>> -       }
>>>
>>>        /* send the report to the address provided in the inform record */
>>>        OSM_LOG(p_log, OSM_LOG_DEBUG, "MATCH! Sending Report...\n");
>>> --
>>> 1.5.5
>>>
>>> There is still a problem with GID OUT trap that is not sent since source port
>>> was already removed.
>>> Patch will follow.
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>

Re: [PATCH v2] opensm: bug in trap report for MC create(66) and delete(67) traps

[Eli Dorfman (Voltaire)]

Hal Rosenstock wrote:
> On Fri, Feb 5, 2010 at 9:18 AM, Eli Dorfman <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Thu, Feb 4, 2010 at 10:52 PM, Hal Rosenstock
>> <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Thu, Feb 4, 2010 at 12:43 PM, Eli Dorfman (Voltaire)
>>> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> Subject: [PATCH] Wrong handling of MC create and delete traps
>>>>
>>>> For these traps the GID in the data details is the MGID and
>>>> not the source port gid.
>>>> So the SM should check that subscriber port has the pkey of the MC group.
>>>> There was also an error in comparing the subnet prefix and guid due to
>>>> host/network order mismatch.
>>>>
>>>> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>>  opensm/opensm/osm_inform.c |  151 ++++++++++++++++++++++++++++---------------
>>>>  1 files changed, 98 insertions(+), 53 deletions(-)
>>>>
>>>> diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
>>>> index 8108213..ae4fe71 100644
>>>> --- a/opensm/opensm/osm_inform.c
>>>> +++ b/opensm/opensm/osm_inform.c
>>>> @@ -341,6 +341,103 @@ Exit:
>>>>        return status;
>>>>  }
>>>>
>>>> +static int is_access_permitted( osm_infr_t *p_infr_rec,
>>>> +                               osm_infr_match_ctxt_t *p_infr_match )
>>>> +{
>>>> +       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>>> +       ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>>> +       ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>>> +       uint16_t trap_num = cl_ntoh16(p_ntc->g_or_v.generic.trap_num);
>>>> +       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>>> +       osm_log_t *p_log = p_infr_rec->sa->p_log;
>>>> +       char gid_str[INET6_ADDRSTRLEN];
>>>> +       osm_mgrp_t *p_mgrp;
>>>> +       ib_gid_t source_gid;
>>>> +       osm_port_t *p_src_port;
>>>> +       osm_port_t *p_dest_port;
>>>> +
>>>> +       /* In case of GID_IN(64) or GID_OUT(65) traps the source gid
>>>> +          comparison should be done on the trap source (saved as the gid in the
>>>> +          data details field).
>>>> +          For traps MC_CREATE(66) or MC_DELETE(67) the data details gid is
>>>> +          the MGID. We need to check whether subscriber has the pky of
>>>
>>>                   typo  ^^^^
>>>
>>>                           pkey
>>>
>>>
>>>> +          the MC group.
>>> Shouldn't this be the subscriber has a compatible pkey with MC group ?
>>> The MC group has a full member PKey and the members can be full or
>>> limited.
>> I accept the correction.
> 
> Doesn't this require a code change for handling trap cases 66-67 ?

I think that you referred to the comment since the code is handling this properly (in my opinion).

> 
>> Sasha, can you please change this in the commit (only if there are not
>> other remarks).
> 
> Is that what you are asking Sasha to do (beyond the typos) ?

I asked Sasha to fix only the typo in commit.

> 
>> BTW, there is no explicit reference in the IB spec for MC group
>> create/delete trap (at least I didn't find it).
> 
> Not sure what you mean by this. What didn't you find ?

in the spec see o13-17.1.2

Thanks,
Eli
> 
> -- Hal
> 
>>>> +          In all other cases the issuer gis is the trap source.
>>>                                               typo  ^^^
>>>                                                       gid
>>>
>> and this typo of course.
>>
>> Thanks,
>> Eli
>>> -- Hal
>>>
>>>> +       */
>>>> +       if (trap_num >= 64 && trap_num <= 67 )
>>>> +               /* The issuer of these traps is the SM so source_gid
>>>> +                  is the gid saved on the data details */
>>>> +               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>>> +       else
>>>> +               source_gid = p_ntc->issuer_gid;
>>>> +
>>>> +       p_dest_port =
>>>> +           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>>> +                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>> +       if (!p_dest_port) {
>>>> +               OSM_LOG(p_log, OSM_LOG_INFO,
>>>> +                       "Cannot find destination port with LID:%u\n",
>>>> +                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>> +               goto Exit;
>>>> +       }
>>>> +
>>>> +       switch (trap_num) {
>>>> +               case 66:
>>>> +               case 67:
>>>> +                       p_mgrp = osm_get_mgrp_by_mgid(p_subn, &source_gid);
>>>> +                       if (!p_mgrp) {
>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>> +                                       "Cannot find MGID %s\n",
>>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str));
>>>> +                               goto Exit;
>>>> +                       }
>>>> +
>>>> +                       if (!osm_physp_has_pkey(p_log,
>>>> +                                               p_mgrp->mcmember_rec.pkey,
>>>> +                                               p_dest_port->p_physp)) {
>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>> +                                       "MGID %s and port GUID:0x%016" PRIx64 " do not share same pkey\n",
>>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str),
>>>> +                                       cl_ntoh64(p_dest_port->guid));
>>>> +                               goto Exit;
>>>> +                       }
>>>> +                       break;
>>>> +
>>>> +               default:
>>>> +                       p_src_port =
>>>> +                           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>>> +                       if (!p_src_port) {
>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>> +                                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>>> +                                       cl_ntoh64(source_gid.unicast.interface_id));
>>>> +                               goto Exit;
>>>> +                       }
>>>> +
>>>> +
>>>> +                       /* Check if there is a pkey match. o13-17.1.1 */
>>>> +                       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>>> +                               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>>> +                               /* According to o13-17.1.2 - If this informInfo does not have
>>>> +                                  lid_range_begin of 0xFFFF, then this informInfo request
>>>> +                                  should be removed from database */
>>>> +                               if (p_ii->lid_range_begin != 0xFFFF) {
>>>> +                                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>>> +                                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>>> +                                               "Need to remove this informInfo from db\n");
>>>> +                                       /* add the informInfo record to the remove_infr list */
>>>> +                                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>>> +                               }
>>>> +                               goto Exit;
>>>> +                       }
>>>> +                       break;
>>>> +       }
>>>> +
>>>> +       return 1;
>>>> +Exit:
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +
>>>>  /**********************************************************************
>>>>  * This routine compares a given Notice and a ListItem of InformInfo type.
>>>>  * PREREQUISITE:
>>>> @@ -351,15 +448,10 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>>  {
>>>>        osm_infr_match_ctxt_t *p_infr_match = (osm_infr_match_ctxt_t *) context;
>>>>        ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>>> -       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>>>        osm_infr_t *p_infr_rec = (osm_infr_t *) p_list_item;
>>>>        ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>>>        cl_status_t status = CL_NOT_FOUND;
>>>>        osm_log_t *p_log = p_infr_rec->sa->p_log;
>>>> -       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>>> -       ib_gid_t source_gid;
>>>> -       osm_port_t *p_src_port;
>>>> -       osm_port_t *p_dest_port;
>>>>
>>>>        OSM_LOG_ENTER(p_log);
>>>>
>>>> @@ -460,55 +552,8 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>>                }
>>>>        }
>>>>
>>>> -       /* Check if there is a pkey match. o13-17.1.1 */
>>>> -       /* Check if the issuer of the trap is the SM. If it is, then the gid
>>>> -          comparison should be done on the trap source (saved as the gid in the
>>>> -          data details field).
>>>> -          If the issuer gid is not the SM - then it is the guid of the trap
>>>> -          source */
>>>> -       if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
>>>> -            p_subn->opt.subnet_prefix)
>>>> -           && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
>>>> -               p_subn->sm_port_guid))
>>>> -               /* The issuer is the SM then this is trap 64-67 - compare the gid
>>>> -                  with the gid saved on the data details */
>>>> -               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>>> -       else
>>>> -               source_gid = p_ntc->issuer_gid;
>>>> -
>>>> -       p_src_port =
>>>> -           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>>> -       if (!p_src_port) {
>>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>>> -                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>>> -                       cl_ntoh64(source_gid.unicast.interface_id));
>>>> +       if (!is_access_permitted(p_infr_rec, p_infr_match))
>>>>                goto Exit;
>>>> -       }
>>>> -
>>>> -       p_dest_port =
>>>> -           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>>> -                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>> -       if (!p_dest_port) {
>>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>>> -                       "Cannot find destination port with LID:%u\n",
>>>> -                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>> -               goto Exit;
>>>> -       }
>>>> -
>>>> -       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>>> -               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>>> -               /* According to o13-17.1.2 - If this informInfo does not have
>>>> -                  lid_range_begin of 0xFFFF, then this informInfo request
>>>> -                  should be removed from database */
>>>> -               if (p_ii->lid_range_begin != 0xFFFF) {
>>>> -                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>>> -                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>>> -                               "Need to remove this informInfo from db\n");
>>>> -                       /* add the informInfo record to the remove_infr list */
>>>> -                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>>> -               }
>>>> -               goto Exit;
>>>> -       }
>>>>
>>>>        /* send the report to the address provided in the inform record */
>>>>        OSM_LOG(p_log, OSM_LOG_DEBUG, "MATCH! Sending Report...\n");
>>>> --
>>>> 1.5.5
>>>>
>>>> There is still a problem with GID OUT trap that is not sent since source port
>>>> was already removed.
>>>> Patch will follow.
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] opensm: bug in trap report for MC create(66) and delete(67) traps

[Hal Rosenstock]

On Sun, Feb 7, 2010 at 4:47 AM, Eli Dorfman (Voltaire)
<dorfman.eli@gmail.com> wrote:
> Hal Rosenstock wrote:
>> On Fri, Feb 5, 2010 at 9:18 AM, Eli Dorfman <dorfman.eli@gmail.com> wrote:
>>> On Thu, Feb 4, 2010 at 10:52 PM, Hal Rosenstock
>>> <hal.rosenstock@gmail.com> wrote:
>>>> On Thu, Feb 4, 2010 at 12:43 PM, Eli Dorfman (Voltaire)
>>>> <dorfman.eli@gmail.com> wrote:
>>>>> Subject: [PATCH] Wrong handling of MC create and delete traps
>>>>>
>>>>> For these traps the GID in the data details is the MGID and
>>>>> not the source port gid.
>>>>> So the SM should check that subscriber port has the pkey of the MC group.
>>>>> There was also an error in comparing the subnet prefix and guid due to
>>>>> host/network order mismatch.
>>>>>
>>>>> Signed-off-by: Eli Dorfman <elid@voltaire.com>
>>>>> ---
>>>>>  opensm/opensm/osm_inform.c |  151 ++++++++++++++++++++++++++++---------------
>>>>>  1 files changed, 98 insertions(+), 53 deletions(-)
>>>>>
>>>>> diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
>>>>> index 8108213..ae4fe71 100644
>>>>> --- a/opensm/opensm/osm_inform.c
>>>>> +++ b/opensm/opensm/osm_inform.c
>>>>> @@ -341,6 +341,103 @@ Exit:
>>>>>        return status;
>>>>>  }
>>>>>
>>>>> +static int is_access_permitted( osm_infr_t *p_infr_rec,
>>>>> +                               osm_infr_match_ctxt_t *p_infr_match )
>>>>> +{
>>>>> +       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>>>> +       ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>>>> +       ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>>>> +       uint16_t trap_num = cl_ntoh16(p_ntc->g_or_v.generic.trap_num);
>>>>> +       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>>>> +       osm_log_t *p_log = p_infr_rec->sa->p_log;
>>>>> +       char gid_str[INET6_ADDRSTRLEN];
>>>>> +       osm_mgrp_t *p_mgrp;
>>>>> +       ib_gid_t source_gid;
>>>>> +       osm_port_t *p_src_port;
>>>>> +       osm_port_t *p_dest_port;
>>>>> +
>>>>> +       /* In case of GID_IN(64) or GID_OUT(65) traps the source gid
>>>>> +          comparison should be done on the trap source (saved as the gid in the
>>>>> +          data details field).
>>>>> +          For traps MC_CREATE(66) or MC_DELETE(67) the data details gid is
>>>>> +          the MGID. We need to check whether subscriber has the pky of
>>>>
>>>>                   typo  ^^^^
>>>>
>>>>                           pkey
>>>>
>>>>
>>>>> +          the MC group.
>>>> Shouldn't this be the subscriber has a compatible pkey with MC group ?
>>>> The MC group has a full member PKey and the members can be full or
>>>> limited.
>>> I accept the correction.
>>
>> Doesn't this require a code change for handling trap cases 66-67 ?
>
> I think that you referred to the comment since the code is handling this properly (in my opinion).

I was referring to both the comment and the code since a port with a
compatible limited pkey should be able to receive the reports for MC
groups.

>
>>
>>> Sasha, can you please change this in the commit (only if there are not
>>> other remarks).
>>
>> Is that what you are asking Sasha to do (beyond the typos) ?
>
> I asked Sasha to fix only the typo in commit.
>
>>
>>> BTW, there is no explicit reference in the IB spec for MC group
>>> create/delete trap (at least I didn't find it).
>>
>> Not sure what you mean by this. What didn't you find ?
>
> in the spec see o13-17.1.2

Yes, there appear to be some holes in the spec in terms of this and
maybe more in this area (event forwarding) but the intent seems clear.

-- Hal

> Thanks,
> Eli
>>
>> -- Hal
>>
>>>>> +          In all other cases the issuer gis is the trap source.
>>>>                                               typo  ^^^
>>>>                                                       gid
>>>>
>>> and this typo of course.
>>>
>>> Thanks,
>>> Eli
>>>> -- Hal
>>>>
>>>>> +       */
>>>>> +       if (trap_num >= 64 && trap_num <= 67 )
>>>>> +               /* The issuer of these traps is the SM so source_gid
>>>>> +                  is the gid saved on the data details */
>>>>> +               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>>>> +       else
>>>>> +               source_gid = p_ntc->issuer_gid;
>>>>> +
>>>>> +       p_dest_port =
>>>>> +           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>>>> +                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>> +       if (!p_dest_port) {
>>>>> +               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>> +                       "Cannot find destination port with LID:%u\n",
>>>>> +                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>> +               goto Exit;
>>>>> +       }
>>>>> +
>>>>> +       switch (trap_num) {
>>>>> +               case 66:
>>>>> +               case 67:
>>>>> +                       p_mgrp = osm_get_mgrp_by_mgid(p_subn, &source_gid);
>>>>> +                       if (!p_mgrp) {
>>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>> +                                       "Cannot find MGID %s\n",
>>>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str));
>>>>> +                               goto Exit;
>>>>> +                       }
>>>>> +
>>>>> +                       if (!osm_physp_has_pkey(p_log,
>>>>> +                                               p_mgrp->mcmember_rec.pkey,
>>>>> +                                               p_dest_port->p_physp)) {
>>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>> +                                       "MGID %s and port GUID:0x%016" PRIx64 " do not share same pkey\n",
>>>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str),
>>>>> +                                       cl_ntoh64(p_dest_port->guid));
>>>>> +                               goto Exit;
>>>>> +                       }
>>>>> +                       break;
>>>>> +
>>>>> +               default:
>>>>> +                       p_src_port =
>>>>> +                           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>>>> +                       if (!p_src_port) {
>>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>> +                                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>>>> +                                       cl_ntoh64(source_gid.unicast.interface_id));
>>>>> +                               goto Exit;
>>>>> +                       }
>>>>> +
>>>>> +
>>>>> +                       /* Check if there is a pkey match. o13-17.1.1 */
>>>>> +                       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>>>> +                               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>>>> +                               /* According to o13-17.1.2 - If this informInfo does not have
>>>>> +                                  lid_range_begin of 0xFFFF, then this informInfo request
>>>>> +                                  should be removed from database */
>>>>> +                               if (p_ii->lid_range_begin != 0xFFFF) {
>>>>> +                                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>>>> +                                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>>>> +                                               "Need to remove this informInfo from db\n");
>>>>> +                                       /* add the informInfo record to the remove_infr list */
>>>>> +                                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>>>> +                               }
>>>>> +                               goto Exit;
>>>>> +                       }
>>>>> +                       break;
>>>>> +       }
>>>>> +
>>>>> +       return 1;
>>>>> +Exit:
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +
>>>>>  /**********************************************************************
>>>>>  * This routine compares a given Notice and a ListItem of InformInfo type.
>>>>>  * PREREQUISITE:
>>>>> @@ -351,15 +448,10 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>>>  {
>>>>>        osm_infr_match_ctxt_t *p_infr_match = (osm_infr_match_ctxt_t *) context;
>>>>>        ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>>>> -       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>>>>        osm_infr_t *p_infr_rec = (osm_infr_t *) p_list_item;
>>>>>        ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>>>>        cl_status_t status = CL_NOT_FOUND;
>>>>>        osm_log_t *p_log = p_infr_rec->sa->p_log;
>>>>> -       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>>>> -       ib_gid_t source_gid;
>>>>> -       osm_port_t *p_src_port;
>>>>> -       osm_port_t *p_dest_port;
>>>>>
>>>>>        OSM_LOG_ENTER(p_log);
>>>>>
>>>>> @@ -460,55 +552,8 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>>>                }
>>>>>        }
>>>>>
>>>>> -       /* Check if there is a pkey match. o13-17.1.1 */
>>>>> -       /* Check if the issuer of the trap is the SM. If it is, then the gid
>>>>> -          comparison should be done on the trap source (saved as the gid in the
>>>>> -          data details field).
>>>>> -          If the issuer gid is not the SM - then it is the guid of the trap
>>>>> -          source */
>>>>> -       if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
>>>>> -            p_subn->opt.subnet_prefix)
>>>>> -           && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
>>>>> -               p_subn->sm_port_guid))
>>>>> -               /* The issuer is the SM then this is trap 64-67 - compare the gid
>>>>> -                  with the gid saved on the data details */
>>>>> -               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>>>> -       else
>>>>> -               source_gid = p_ntc->issuer_gid;
>>>>> -
>>>>> -       p_src_port =
>>>>> -           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>>>> -       if (!p_src_port) {
>>>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>> -                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>>>> -                       cl_ntoh64(source_gid.unicast.interface_id));
>>>>> +       if (!is_access_permitted(p_infr_rec, p_infr_match))
>>>>>                goto Exit;
>>>>> -       }
>>>>> -
>>>>> -       p_dest_port =
>>>>> -           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>>>> -                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>> -       if (!p_dest_port) {
>>>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>> -                       "Cannot find destination port with LID:%u\n",
>>>>> -                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>> -               goto Exit;
>>>>> -       }
>>>>> -
>>>>> -       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>>>> -               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>>>> -               /* According to o13-17.1.2 - If this informInfo does not have
>>>>> -                  lid_range_begin of 0xFFFF, then this informInfo request
>>>>> -                  should be removed from database */
>>>>> -               if (p_ii->lid_range_begin != 0xFFFF) {
>>>>> -                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>>>> -                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>>>> -                               "Need to remove this informInfo from db\n");
>>>>> -                       /* add the informInfo record to the remove_infr list */
>>>>> -                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>>>> -               }
>>>>> -               goto Exit;
>>>>> -       }
>>>>>
>>>>>        /* send the report to the address provided in the inform record */
>>>>>        OSM_LOG(p_log, OSM_LOG_DEBUG, "MATCH! Sending Report...\n");
>>>>> --
>>>>> 1.5.5
>>>>>
>>>>> There is still a problem with GID OUT trap that is not sent since source port
>>>>> was already removed.
>>>>> Patch will follow.
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>
>

Re: [PATCH v2] opensm: bug in trap report for MC create(66) and delete(67) traps

[Eli Dorfman (Voltaire)]

Hal Rosenstock wrote:
> On Sun, Feb 7, 2010 at 4:47 AM, Eli Dorfman (Voltaire)
> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hal Rosenstock wrote:
>>> On Fri, Feb 5, 2010 at 9:18 AM, Eli Dorfman <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On Thu, Feb 4, 2010 at 10:52 PM, Hal Rosenstock
>>>> <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> On Thu, Feb 4, 2010 at 12:43 PM, Eli Dorfman (Voltaire)
>>>>> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> Subject: [PATCH] Wrong handling of MC create and delete traps
>>>>>>
>>>>>> For these traps the GID in the data details is the MGID and
>>>>>> not the source port gid.
>>>>>> So the SM should check that subscriber port has the pkey of the MC group.
>>>>>> There was also an error in comparing the subnet prefix and guid due to
>>>>>> host/network order mismatch.
>>>>>>
>>>>>> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>>>>>> ---
>>>>>>  opensm/opensm/osm_inform.c |  151 ++++++++++++++++++++++++++++---------------
>>>>>>  1 files changed, 98 insertions(+), 53 deletions(-)
>>>>>>
>>>>>> diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
>>>>>> index 8108213..ae4fe71 100644
>>>>>> --- a/opensm/opensm/osm_inform.c
>>>>>> +++ b/opensm/opensm/osm_inform.c
>>>>>> @@ -341,6 +341,103 @@ Exit:
>>>>>>        return status;
>>>>>>  }
>>>>>>
>>>>>> +static int is_access_permitted( osm_infr_t *p_infr_rec,
>>>>>> +                               osm_infr_match_ctxt_t *p_infr_match )
>>>>>> +{
>>>>>> +       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>>>>> +       ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>>>>> +       ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>>>>> +       uint16_t trap_num = cl_ntoh16(p_ntc->g_or_v.generic.trap_num);
>>>>>> +       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>>>>> +       osm_log_t *p_log = p_infr_rec->sa->p_log;
>>>>>> +       char gid_str[INET6_ADDRSTRLEN];
>>>>>> +       osm_mgrp_t *p_mgrp;
>>>>>> +       ib_gid_t source_gid;
>>>>>> +       osm_port_t *p_src_port;
>>>>>> +       osm_port_t *p_dest_port;
>>>>>> +
>>>>>> +       /* In case of GID_IN(64) or GID_OUT(65) traps the source gid
>>>>>> +          comparison should be done on the trap source (saved as the gid in the
>>>>>> +          data details field).
>>>>>> +          For traps MC_CREATE(66) or MC_DELETE(67) the data details gid is
>>>>>> +          the MGID. We need to check whether subscriber has the pky of
>>>>>                   typo  ^^^^
>>>>>
>>>>>                           pkey
>>>>>
>>>>>
>>>>>> +          the MC group.
>>>>> Shouldn't this be the subscriber has a compatible pkey with MC group ?
>>>>> The MC group has a full member PKey and the members can be full or
>>>>> limited.
>>>> I accept the correction.
>>> Doesn't this require a code change for handling trap cases 66-67 ?
>> I think that you referred to the comment since the code is handling this properly (in my opinion).
> 
> I was referring to both the comment and the code since a port with a
> compatible limited pkey should be able to receive the reports for MC
> groups.

I agree and I think that the code is handling this case properly.
osm_physp_has_pkey() takes the 15 lower MGID pkey bits and checks whether it is the physp pkey table.

Eli
> 
>>>> Sasha, can you please change this in the commit (only if there are not
>>>> other remarks).
>>> Is that what you are asking Sasha to do (beyond the typos) ?
>> I asked Sasha to fix only the typo in commit.
>>
>>>> BTW, there is no explicit reference in the IB spec for MC group
>>>> create/delete trap (at least I didn't find it).
>>> Not sure what you mean by this. What didn't you find ?
>> in the spec see o13-17.1.2
> 
> Yes, there appear to be some holes in the spec in terms of this and
> maybe more in this area (event forwarding) but the intent seems clear.
> 
> -- Hal
> 
>> Thanks,
>> Eli
>>> -- Hal
>>>
>>>>>> +          In all other cases the issuer gis is the trap source.
>>>>>                                               typo  ^^^
>>>>>                                                       gid
>>>>>
>>>> and this typo of course.
>>>>
>>>> Thanks,
>>>> Eli
>>>>> -- Hal
>>>>>
>>>>>> +       */
>>>>>> +       if (trap_num >= 64 && trap_num <= 67 )
>>>>>> +               /* The issuer of these traps is the SM so source_gid
>>>>>> +                  is the gid saved on the data details */
>>>>>> +               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>>>>> +       else
>>>>>> +               source_gid = p_ntc->issuer_gid;
>>>>>> +
>>>>>> +       p_dest_port =
>>>>>> +           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>>>>> +                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>>> +       if (!p_dest_port) {
>>>>>> +               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>> +                       "Cannot find destination port with LID:%u\n",
>>>>>> +                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>>> +               goto Exit;
>>>>>> +       }
>>>>>> +
>>>>>> +       switch (trap_num) {
>>>>>> +               case 66:
>>>>>> +               case 67:
>>>>>> +                       p_mgrp = osm_get_mgrp_by_mgid(p_subn, &source_gid);
>>>>>> +                       if (!p_mgrp) {
>>>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>> +                                       "Cannot find MGID %s\n",
>>>>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str));
>>>>>> +                               goto Exit;
>>>>>> +                       }
>>>>>> +
>>>>>> +                       if (!osm_physp_has_pkey(p_log,
>>>>>> +                                               p_mgrp->mcmember_rec.pkey,
>>>>>> +                                               p_dest_port->p_physp)) {
>>>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>> +                                       "MGID %s and port GUID:0x%016" PRIx64 " do not share same pkey\n",
>>>>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str),
>>>>>> +                                       cl_ntoh64(p_dest_port->guid));
>>>>>> +                               goto Exit;
>>>>>> +                       }
>>>>>> +                       break;
>>>>>> +
>>>>>> +               default:
>>>>>> +                       p_src_port =
>>>>>> +                           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>>>>> +                       if (!p_src_port) {
>>>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>> +                                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>>>>> +                                       cl_ntoh64(source_gid.unicast.interface_id));
>>>>>> +                               goto Exit;
>>>>>> +                       }
>>>>>> +
>>>>>> +
>>>>>> +                       /* Check if there is a pkey match. o13-17.1.1 */
>>>>>> +                       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>>>>> +                               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>>>>> +                               /* According to o13-17.1.2 - If this informInfo does not have
>>>>>> +                                  lid_range_begin of 0xFFFF, then this informInfo request
>>>>>> +                                  should be removed from database */
>>>>>> +                               if (p_ii->lid_range_begin != 0xFFFF) {
>>>>>> +                                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>>>>> +                                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>>>>> +                                               "Need to remove this informInfo from db\n");
>>>>>> +                                       /* add the informInfo record to the remove_infr list */
>>>>>> +                                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>>>>> +                               }
>>>>>> +                               goto Exit;
>>>>>> +                       }
>>>>>> +                       break;
>>>>>> +       }
>>>>>> +
>>>>>> +       return 1;
>>>>>> +Exit:
>>>>>> +       return 0;
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>>  /**********************************************************************
>>>>>>  * This routine compares a given Notice and a ListItem of InformInfo type.
>>>>>>  * PREREQUISITE:
>>>>>> @@ -351,15 +448,10 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>>>>  {
>>>>>>        osm_infr_match_ctxt_t *p_infr_match = (osm_infr_match_ctxt_t *) context;
>>>>>>        ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>>>>> -       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>>>>>        osm_infr_t *p_infr_rec = (osm_infr_t *) p_list_item;
>>>>>>        ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>>>>>        cl_status_t status = CL_NOT_FOUND;
>>>>>>        osm_log_t *p_log = p_infr_rec->sa->p_log;
>>>>>> -       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>>>>> -       ib_gid_t source_gid;
>>>>>> -       osm_port_t *p_src_port;
>>>>>> -       osm_port_t *p_dest_port;
>>>>>>
>>>>>>        OSM_LOG_ENTER(p_log);
>>>>>>
>>>>>> @@ -460,55 +552,8 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>>>>                }
>>>>>>        }
>>>>>>
>>>>>> -       /* Check if there is a pkey match. o13-17.1.1 */
>>>>>> -       /* Check if the issuer of the trap is the SM. If it is, then the gid
>>>>>> -          comparison should be done on the trap source (saved as the gid in the
>>>>>> -          data details field).
>>>>>> -          If the issuer gid is not the SM - then it is the guid of the trap
>>>>>> -          source */
>>>>>> -       if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
>>>>>> -            p_subn->opt.subnet_prefix)
>>>>>> -           && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
>>>>>> -               p_subn->sm_port_guid))
>>>>>> -               /* The issuer is the SM then this is trap 64-67 - compare the gid
>>>>>> -                  with the gid saved on the data details */
>>>>>> -               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>>>>> -       else
>>>>>> -               source_gid = p_ntc->issuer_gid;
>>>>>> -
>>>>>> -       p_src_port =
>>>>>> -           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>>>>> -       if (!p_src_port) {
>>>>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>> -                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>>>>> -                       cl_ntoh64(source_gid.unicast.interface_id));
>>>>>> +       if (!is_access_permitted(p_infr_rec, p_infr_match))
>>>>>>                goto Exit;
>>>>>> -       }
>>>>>> -
>>>>>> -       p_dest_port =
>>>>>> -           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>>>>> -                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>>> -       if (!p_dest_port) {
>>>>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>> -                       "Cannot find destination port with LID:%u\n",
>>>>>> -                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>>> -               goto Exit;
>>>>>> -       }
>>>>>> -
>>>>>> -       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>>>>> -               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>>>>> -               /* According to o13-17.1.2 - If this informInfo does not have
>>>>>> -                  lid_range_begin of 0xFFFF, then this informInfo request
>>>>>> -                  should be removed from database */
>>>>>> -               if (p_ii->lid_range_begin != 0xFFFF) {
>>>>>> -                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>>>>> -                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>>>>> -                               "Need to remove this informInfo from db\n");
>>>>>> -                       /* add the informInfo record to the remove_infr list */
>>>>>> -                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>>>>> -               }
>>>>>> -               goto Exit;
>>>>>> -       }
>>>>>>
>>>>>>        /* send the report to the address provided in the inform record */
>>>>>>        OSM_LOG(p_log, OSM_LOG_DEBUG, "MATCH! Sending Report...\n");
>>>>>> --
>>>>>> 1.5.5
>>>>>>
>>>>>> There is still a problem with GID OUT trap that is not sent since source port
>>>>>> was already removed.
>>>>>> Patch will follow.
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>>>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] opensm: bug in trap report for MC create(66) and delete(67) traps

[Hal Rosenstock]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 14707 bytes --]

On Mon, Feb 8, 2010 at 11:05 AM, Eli Dorfman (Voltaire)
<dorfman.eli@gmail.com> wrote:
> Hal Rosenstock wrote:
>> On Sun, Feb 7, 2010 at 4:47 AM, Eli Dorfman (Voltaire)
>> <dorfman.eli@gmail.com> wrote:
>>> Hal Rosenstock wrote:
>>>> On Fri, Feb 5, 2010 at 9:18 AM, Eli Dorfman <dorfman.eli@gmail.com> wrote:
>>>>> On Thu, Feb 4, 2010 at 10:52 PM, Hal Rosenstock
>>>>> <hal.rosenstock@gmail.com> wrote:
>>>>>> On Thu, Feb 4, 2010 at 12:43 PM, Eli Dorfman (Voltaire)
>>>>>> <dorfman.eli@gmail.com> wrote:
>>>>>>> Subject: [PATCH] Wrong handling of MC create and delete traps
>>>>>>>
>>>>>>> For these traps the GID in the data details is the MGID and
>>>>>>> not the source port gid.
>>>>>>> So the SM should check that subscriber port has the pkey of the MC group.
>>>>>>> There was also an error in comparing the subnet prefix and guid due to
>>>>>>> host/network order mismatch.
>>>>>>>
>>>>>>> Signed-off-by: Eli Dorfman <elid@voltaire.com>
>>>>>>> ---
>>>>>>>  opensm/opensm/osm_inform.c |  151 ++++++++++++++++++++++++++++---------------
>>>>>>>  1 files changed, 98 insertions(+), 53 deletions(-)
>>>>>>>
>>>>>>> diff --git a/opensm/opensm/osm_inform.c b/opensm/opensm/osm_inform.c
>>>>>>> index 8108213..ae4fe71 100644
>>>>>>> --- a/opensm/opensm/osm_inform.c
>>>>>>> +++ b/opensm/opensm/osm_inform.c
>>>>>>> @@ -341,6 +341,103 @@ Exit:
>>>>>>>        return status;
>>>>>>>  }
>>>>>>>
>>>>>>> +static int is_access_permitted( osm_infr_t *p_infr_rec,
>>>>>>> +                               osm_infr_match_ctxt_t *p_infr_match )
>>>>>>> +{
>>>>>>> +       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>>>>>> +       ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>>>>>> +       ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>>>>>> +       uint16_t trap_num = cl_ntoh16(p_ntc->g_or_v.generic.trap_num);
>>>>>>> +       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>>>>>> +       osm_log_t *p_log = p_infr_rec->sa->p_log;
>>>>>>> +       char gid_str[INET6_ADDRSTRLEN];
>>>>>>> +       osm_mgrp_t *p_mgrp;
>>>>>>> +       ib_gid_t source_gid;
>>>>>>> +       osm_port_t *p_src_port;
>>>>>>> +       osm_port_t *p_dest_port;
>>>>>>> +
>>>>>>> +       /* In case of GID_IN(64) or GID_OUT(65) traps the source gid
>>>>>>> +          comparison should be done on the trap source (saved as the gid in the
>>>>>>> +          data details field).
>>>>>>> +          For traps MC_CREATE(66) or MC_DELETE(67) the data details gid is
>>>>>>> +          the MGID. We need to check whether subscriber has the pky of
>>>>>>                   typo  ^^^^
>>>>>>
>>>>>>                           pkey
>>>>>>
>>>>>>
>>>>>>> +          the MC group.
>>>>>> Shouldn't this be the subscriber has a compatible pkey with MC group ?
>>>>>> The MC group has a full member PKey and the members can be full or
>>>>>> limited.
>>>>> I accept the correction.
>>>> Doesn't this require a code change for handling trap cases 66-67 ?
>>> I think that you referred to the comment since the code is handling this properly (in my opinion).
>>
>> I was referring to both the comment and the code since a port with a
>> compatible limited pkey should be able to receive the reports for MC
>> groups.
>
> I agree and I think that the code is handling this case properly.
> osm_physp_has_pkey() takes the 15 lower MGID pkey bits and checks whether it is the physp pkey table.

You're right; the code handles it. I missed the ib_pkey_get_base call there.

-- Hal

>
> Eli
>>
>>>>> Sasha, can you please change this in the commit (only if there are not
>>>>> other remarks).
>>>> Is that what you are asking Sasha to do (beyond the typos) ?
>>> I asked Sasha to fix only the typo in commit.
>>>
>>>>> BTW, there is no explicit reference in the IB spec for MC group
>>>>> create/delete trap (at least I didn't find it).
>>>> Not sure what you mean by this. What didn't you find ?
>>> in the spec see o13-17.1.2
>>
>> Yes, there appear to be some holes in the spec in terms of this and
>> maybe more in this area (event forwarding) but the intent seems clear.
>>
>> -- Hal
>>
>>> Thanks,
>>> Eli
>>>> -- Hal
>>>>
>>>>>>> +          In all other cases the issuer gis is the trap source.
>>>>>>                                               typo  ^^^
>>>>>>                                                       gid
>>>>>>
>>>>> and this typo of course.
>>>>>
>>>>> Thanks,
>>>>> Eli
>>>>>> -- Hal
>>>>>>
>>>>>>> +       */
>>>>>>> +       if (trap_num >= 64 && trap_num <= 67 )
>>>>>>> +               /* The issuer of these traps is the SM so source_gid
>>>>>>> +                  is the gid saved on the data details */
>>>>>>> +               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>>>>>> +       else
>>>>>>> +               source_gid = p_ntc->issuer_gid;
>>>>>>> +
>>>>>>> +       p_dest_port =
>>>>>>> +           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>>>>>> +                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>>>> +       if (!p_dest_port) {
>>>>>>> +               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>>> +                       "Cannot find destination port with LID:%u\n",
>>>>>>> +                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>>>> +               goto Exit;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       switch (trap_num) {
>>>>>>> +               case 66:
>>>>>>> +               case 67:
>>>>>>> +                       p_mgrp = osm_get_mgrp_by_mgid(p_subn, &source_gid);
>>>>>>> +                       if (!p_mgrp) {
>>>>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>>> +                                       "Cannot find MGID %s\n",
>>>>>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str));
>>>>>>> +                               goto Exit;
>>>>>>> +                       }
>>>>>>> +
>>>>>>> +                       if (!osm_physp_has_pkey(p_log,
>>>>>>> +                                               p_mgrp->mcmember_rec.pkey,
>>>>>>> +                                               p_dest_port->p_physp)) {
>>>>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>>> +                                       "MGID %s and port GUID:0x%016" PRIx64 " do not share same pkey\n",
>>>>>>> +                                       inet_ntop(AF_INET6, source_gid.raw, gid_str, sizeof gid_str),
>>>>>>> +                                       cl_ntoh64(p_dest_port->guid));
>>>>>>> +                               goto Exit;
>>>>>>> +                       }
>>>>>>> +                       break;
>>>>>>> +
>>>>>>> +               default:
>>>>>>> +                       p_src_port =
>>>>>>> +                           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>>>>>> +                       if (!p_src_port) {
>>>>>>> +                               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>>> +                                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>>>>>> +                                       cl_ntoh64(source_gid.unicast.interface_id));
>>>>>>> +                               goto Exit;
>>>>>>> +                       }
>>>>>>> +
>>>>>>> +
>>>>>>> +                       /* Check if there is a pkey match. o13-17.1.1 */
>>>>>>> +                       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>>>>>> +                               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>>>>>> +                               /* According to o13-17.1.2 - If this informInfo does not have
>>>>>>> +                                  lid_range_begin of 0xFFFF, then this informInfo request
>>>>>>> +                                  should be removed from database */
>>>>>>> +                               if (p_ii->lid_range_begin != 0xFFFF) {
>>>>>>> +                                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>>>>>> +                                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>>>>>> +                                               "Need to remove this informInfo from db\n");
>>>>>>> +                                       /* add the informInfo record to the remove_infr list */
>>>>>>> +                                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>>>>>> +                               }
>>>>>>> +                               goto Exit;
>>>>>>> +                       }
>>>>>>> +                       break;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       return 1;
>>>>>>> +Exit:
>>>>>>> +       return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +
>>>>>>>  /**********************************************************************
>>>>>>>  * This routine compares a given Notice and a ListItem of InformInfo type.
>>>>>>>  * PREREQUISITE:
>>>>>>> @@ -351,15 +448,10 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>>>>>  {
>>>>>>>        osm_infr_match_ctxt_t *p_infr_match = (osm_infr_match_ctxt_t *) context;
>>>>>>>        ib_mad_notice_attr_t *p_ntc = p_infr_match->p_ntc;
>>>>>>> -       cl_list_t *p_infr_to_remove_list = p_infr_match->p_remove_infr_list;
>>>>>>>        osm_infr_t *p_infr_rec = (osm_infr_t *) p_list_item;
>>>>>>>        ib_inform_info_t *p_ii = &(p_infr_rec->inform_record.inform_info);
>>>>>>>        cl_status_t status = CL_NOT_FOUND;
>>>>>>>        osm_log_t *p_log = p_infr_rec->sa->p_log;
>>>>>>> -       osm_subn_t *p_subn = p_infr_rec->sa->p_subn;
>>>>>>> -       ib_gid_t source_gid;
>>>>>>> -       osm_port_t *p_src_port;
>>>>>>> -       osm_port_t *p_dest_port;
>>>>>>>
>>>>>>>        OSM_LOG_ENTER(p_log);
>>>>>>>
>>>>>>> @@ -460,55 +552,8 @@ static void match_notice_to_inf_rec(IN cl_list_item_t * p_list_item,
>>>>>>>                }
>>>>>>>        }
>>>>>>>
>>>>>>> -       /* Check if there is a pkey match. o13-17.1.1 */
>>>>>>> -       /* Check if the issuer of the trap is the SM. If it is, then the gid
>>>>>>> -          comparison should be done on the trap source (saved as the gid in the
>>>>>>> -          data details field).
>>>>>>> -          If the issuer gid is not the SM - then it is the guid of the trap
>>>>>>> -          source */
>>>>>>> -       if ((cl_ntoh64(p_ntc->issuer_gid.unicast.prefix) ==
>>>>>>> -            p_subn->opt.subnet_prefix)
>>>>>>> -           && (cl_ntoh64(p_ntc->issuer_gid.unicast.interface_id) ==
>>>>>>> -               p_subn->sm_port_guid))
>>>>>>> -               /* The issuer is the SM then this is trap 64-67 - compare the gid
>>>>>>> -                  with the gid saved on the data details */
>>>>>>> -               source_gid = p_ntc->data_details.ntc_64_67.gid;
>>>>>>> -       else
>>>>>>> -               source_gid = p_ntc->issuer_gid;
>>>>>>> -
>>>>>>> -       p_src_port =
>>>>>>> -           osm_get_port_by_guid(p_subn, source_gid.unicast.interface_id);
>>>>>>> -       if (!p_src_port) {
>>>>>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>>> -                       "Cannot find source port with GUID:0x%016" PRIx64 "\n",
>>>>>>> -                       cl_ntoh64(source_gid.unicast.interface_id));
>>>>>>> +       if (!is_access_permitted(p_infr_rec, p_infr_match))
>>>>>>>                goto Exit;
>>>>>>> -       }
>>>>>>> -
>>>>>>> -       p_dest_port =
>>>>>>> -           cl_ptr_vector_get(&p_subn->port_lid_tbl,
>>>>>>> -                             cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>>>> -       if (!p_dest_port) {
>>>>>>> -               OSM_LOG(p_log, OSM_LOG_INFO,
>>>>>>> -                       "Cannot find destination port with LID:%u\n",
>>>>>>> -                       cl_ntoh16(p_infr_rec->report_addr.dest_lid));
>>>>>>> -               goto Exit;
>>>>>>> -       }
>>>>>>> -
>>>>>>> -       if (osm_port_share_pkey(p_log, p_src_port, p_dest_port) == FALSE) {
>>>>>>> -               OSM_LOG(p_log, OSM_LOG_DEBUG, "Mismatch by Pkey\n");
>>>>>>> -               /* According to o13-17.1.2 - If this informInfo does not have
>>>>>>> -                  lid_range_begin of 0xFFFF, then this informInfo request
>>>>>>> -                  should be removed from database */
>>>>>>> -               if (p_ii->lid_range_begin != 0xFFFF) {
>>>>>>> -                       OSM_LOG(p_log, OSM_LOG_VERBOSE,
>>>>>>> -                               "Pkey mismatch on lid_range_begin != 0xFFFF. "
>>>>>>> -                               "Need to remove this informInfo from db\n");
>>>>>>> -                       /* add the informInfo record to the remove_infr list */
>>>>>>> -                       cl_list_insert_tail(p_infr_to_remove_list, p_infr_rec);
>>>>>>> -               }
>>>>>>> -               goto Exit;
>>>>>>> -       }
>>>>>>>
>>>>>>>        /* send the report to the address provided in the inform record */
>>>>>>>        OSM_LOG(p_log, OSM_LOG_DEBUG, "MATCH! Sending Report...\n");
>>>>>>> --
>>>>>>> 1.5.5
>>>>>>>
>>>>>>> There is still a problem with GID OUT trap that is not sent since source port
>>>>>>> was already removed.
>>>>>>> Patch will follow.
>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>
>>>
>
>
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH v2] opensm: bug in trap report for MC create(66) and delete(67) traps

[Sasha Khapyorsky]

On 19:43 Thu 04 Feb     , Eli Dorfman (Voltaire) wrote:
> 
> Subject: [PATCH] Wrong handling of MC create and delete traps
> 
> For these traps the GID in the data details is the MGID and
> not the source port gid.
> So the SM should check that subscriber port has the pkey of the MC group.
> There was also an error in comparing the subnet prefix and guid due to
> host/network order mismatch.
> 
> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>

Rebased and applied. Thanks.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html