Hal Rosenstock wrote:
> On Sun, Feb 7, 2010 at 4:47 AM, Eli Dorfman (Voltaire)
> <dorfman....@gmail.com> wrote:
>> Hal Rosenstock wrote:
>>> On Fri, Feb 5, 2010 at 9:18 AM, Eli Dorfman <dorfman....@gmail.com> wrote:
>>>> On Thu, Feb 4, 2010 at 10:52 PM, Hal Rosenstock
>>>> <hal.rosenst...@gmail.com> wrote:
>>>>> On Thu, Feb 4, 2010 at 12:43 PM, Eli Dorfman (Voltaire)
>>>>> <dorfman....@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 <e...@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.
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 majord...@vger.kernel.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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
