On Mon, Feb 8, 2010 at 11:05 AM, Eli Dorfman (Voltaire) wrote: > Hal Rosenstock wrote: >> On Sun, Feb 7, 2010 at 4:47 AM, Eli Dorfman (Voltaire) >> wrote: >>> Hal Rosenstock wrote: >>>> On Fri, Feb 5, 2010 at 9:18 AM, Eli Dorfman wrote: >>>>> On Thu, Feb 4, 2010 at 10:52 PM, Hal Rosenstock >>>>> wrote: >>>>>> On Thu, Feb 4, 2010 at 12:43 PM, 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 >>>>>>> --- >>>>>>>  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 >>>>>>> >>> > > NrybXǧv^)޺{.n+{ٚ{ayʇڙ,jfh/oScڳ9u&jw(階ݢj"mzޖfh~m