Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 11.3 \(3445.6.18\))
Subject: Re: [PATCH IB/core 2/2] IB/cm: Send authentic pkey in REQ msg and
 check eligibility of the pkeys
From:   =?utf-8?Q?H=C3=A5kon_Bugge?= <haakon.bugge@oracle.com>
In-Reply-To: <325ba4af-b52e-4f70-9d89-38dafe10ba99@dev.mellanox.co.il>
Date:   Fri, 11 May 2018 12:55:00 +0200
Cc:     Doug Ledford <dledford@redhat.com>,
        Don Hiatt <don.hiatt@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Sean Hefty <sean.hefty@intel.com>,
        OFED mailing list <linux-rdma@vger.kernel.org>,
        linux-kernel@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <23B160D2-6631-42A6-8A4D-644E90E8A704@oracle.com>
References: <20180509093020.24503-1-Haakon.Bugge@oracle.com>
 <20180509093020.24503-3-Haakon.Bugge@oracle.com>
 <ab1f16cd-aa60-0c43-841c-aed301fd2a7e@dev.mellanox.co.il>
 <F9802EE9-4420-44EE-93DC-27CC799B427E@oracle.com>
 <3bee76df-49a6-cf3c-6df4-749a6309358e@dev.mellanox.co.il>
 <B3360947-5905-47BD-ABC4-6C4BB32F5DA6@oracle.com>
 <325ba4af-b52e-4f70-9d89-38dafe10ba99@dev.mellanox.co.il>
To:     Hal Rosenstock <hal@dev.mellanox.co.il>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On 10 May 2018, at 18:54, Hal Rosenstock <hal@dev.mellanox.co.il> =
wrote:
>=20
> On 5/10/2018 11:16 AM, H=C3=A5kon Bugge wrote:
>>=20
>>=20
>>> On 10 May 2018, at 16:01, Hal Rosenstock <hal@dev.mellanox.co.il> =
wrote:
>>>=20
>>> On 5/10/2018 5:16 AM, H=C3=A5kon Bugge wrote:
>>>>=20
>>>>=20
>>>>> On 9 May 2018, at 13:28, Hal Rosenstock <hal@dev.mellanox.co.il> =
wrote:
>>>>>=20
>>>>> On 5/9/2018 5:30 AM, H=C3=A5kon Bugge wrote:
>>>>>> There is no point in using RDMA CM to establish a connection =
between
>>>>>> two QPs that cannot possible communicate. Particularly, if both =
the
>>>>>> active and passive side use limited pkeys, they are not able to
>>>>>> communicate.
>>>>>>=20
>>>>>> In order to detect this situation, the authentic pkey is used in =
the
>>>>>> CM REQ message. The authentic pkey is the one that the HCA =
inserts
>>>>>> into the BTH in the IB packets.
>>>>>>=20
>>>>>> When the passive side receives the REQ, commit ("ib_core: A full =
pkey
>>>>>> is required to match a limited one") ensures that
>>>>>> ib_find_matched_cached_pkey() fails unless at least one of the =
pkeys
>>>>>> compared has the full-member bit.
>>>>>>=20
>>>>>> In the limited-to-limited case, this will prohibit the connection =
to
>>>>>> be formed, and thus, Pkey Violation Traps will not be sent to the =
SM.
>>>>>>=20
>>>>>> Signed-off-by: H=C3=A5kon Bugge <haakon.bugge@oracle.com>
>>>>>> ---
>>>>>> drivers/infiniband/core/cm.c | 39 =
++++++++++++++++++++++++++++++++-------
>>>>>> include/rdma/ib_cm.h         |  4 +++-
>>>>>> 2 files changed, 35 insertions(+), 8 deletions(-)
>>>>>>=20
>>>>>> diff --git a/drivers/infiniband/core/cm.c =
b/drivers/infiniband/core/cm.c
>>>>>> index a92e1a5c202b..52ed51d5bd2a 100644
>>>>>> --- a/drivers/infiniband/core/cm.c
>>>>>> +++ b/drivers/infiniband/core/cm.c
>>>>>> @@ -3,6 +3,7 @@
>>>>>> * Copyright (c) 2004 Topspin Corporation.  All rights reserved.
>>>>>> * Copyright (c) 2004, 2005 Voltaire Corporation.  All rights =
reserved.
>>>>>> * Copyright (c) 2005 Sun Microsystems, Inc. All rights reserved.
>>>>>> + * Copyright (c) 2018 Oracle and/or its affiliates. All rights =
reserved.
>>>>>> *
>>>>>> * This software is available to you under a choice of one of two
>>>>>> * licenses.  You may choose to be licensed under the terms of the =
GNU
>>>>>> @@ -91,6 +92,7 @@ static const char * const =
ibcm_rej_reason_strs[] =3D {
>>>>>> 	[IB_CM_REJ_INVALID_CLASS_VERSION]	=3D "invalid class =
version",
>>>>>> 	[IB_CM_REJ_INVALID_FLOW_LABEL]		=3D "invalid flow =
label",
>>>>>> 	[IB_CM_REJ_INVALID_ALT_FLOW_LABEL]	=3D "invalid alt flow =
label",
>>>>>> +	[IB_CM_REJ_INVALID_PKEY]		=3D "invalid =
PKey",
>>>>>=20
>>>>> If this patch goes ahead, IBA spec for CM should be updated to =
include this.
>>>>=20
>>>> Sure, I see:
>>>>=20
>>>> 33 Invalid Alternate Flow Label
>>>>=20
>>>> as the latest in the spec.
>>>=20
>>> This appears to be an implementation rather than architectural =
issue. If
>>> there is limited <-> limited CM, then the MAD would never get up to =
the
>>> CM layer as it would be filtered by end port pkey enforcement. This =
is
>>> only needed to protect against current code which can send on the =
full
>>> rather than limited pkey (in BTH), right ?
>>=20
>> We are talking about two things here. The PKey in the BTH and the =
PKey in the CM REQ payload. They differ.
>=20
> Yes, I understand the difference between the 2 potentially different =
PKeys.
>=20
>> I am out of office, but if my memory serves me correct, the PKey in =
the BTH in the MAD packet will be the default PKey.=20
>=20
> MADs are sent on reversible paths (13.5.4.1) where the response can be
> constructed from only header info in the request. BTH:pkey doesn't
> __have to be__ the default pkey. It just needs to be a common/shared
> pkey between the source/destination pair.
>=20
> For example, there are some configurations where the port is only a
> limited member of the default partition (and full member of other non
> default partition(s)).
>=20
> There are also configurations where the port is both full and limited
> member on one or more partition(s). This is the allow_both_pkeys TRUE
> case for OpenSM which also uses the 'both' syntax in the partitions
> config file. This appears to be the case to which you are referring.
>=20
> CM MAD could be sent on default partition but that may not work if =
both
> source and dest are only limited members of the default partition.
>=20
> CM MAD could be sent on any common/shared pkey between the
> source/destination ports.
>=20
>> Further, we have per IBTA:
>>=20
>> C10-132: Packets sent to the General Service Interface QP (QP1) shall =
be accepted if the P_Key in the packet matches any valid P_Key in the =
P_Key Table of the port on which the packet arrived. Matching is defined =
in 10.9.3 Partition Key Matching.
>>=20
>> So, by architecture, the MAD arrives
> In the case of port being both full and limited member of the same
> partition on which CM MAD comes in on (as defined by pkey in BTH).
>=20
> I was talking about limited <-> limited CM and didn't realize you were
> talking about the =E2=80=98both' case.

OK. So we agree that for the most typical cases, where the BTH.PKey is =
the default one, the MAD will be received and not trapped by the PKey =
check.

>>>=20
>>>=20
>>>>>=20
>>>>>> };
>>>>>>=20
>>>>>> const char *__attribute_const__ ibcm_reject_msg(int reason)
>>>>>> @@ -518,8 +520,8 @@ static int cm_init_av_by_path(struct =
sa_path_rec *path, struct cm_av *av,
>>>>>> 		return -EINVAL;
>>>>>> 	cm_dev =3D port->cm_dev;
>>>>>>=20
>>>>>> -	ret =3D ib_find_cached_pkey(cm_dev->ib_device, =
port->port_num,
>>>>>> -				  be16_to_cpu(path->pkey), =
&av->pkey_index);
>>>>>> +	ret =3D ib_find_matched_cached_pkey(cm_dev->ib_device, =
port->port_num,
>>>>>> +					  =
be16_to_cpu(path->pkey), &av->pkey_index);
>>>>>> 	if (ret)
>>>>>> 		return ret;
>>>>>>=20
>>>>>> @@ -1241,7 +1243,7 @@ static void cm_format_req(struct cm_req_msg =
*req_msg,
>>>>>> 	cm_req_set_starting_psn(req_msg, =
cpu_to_be32(param->starting_psn));
>>>>>> 	cm_req_set_local_resp_timeout(req_msg,
>>>>>> 				      param->local_cm_response_timeout);
>>>>>> -	req_msg->pkey =3D param->primary_path->pkey;
>>>>>> +	req_msg->pkey =3D cpu_to_be16(cm_id_priv->pkey);
>>>>>> 	cm_req_set_path_mtu(req_msg, param->primary_path->mtu);
>>>>>> 	cm_req_set_max_cm_retries(req_msg, param->max_cm_retries);
>>>>>>=20
>>>>>> @@ -1396,7 +1398,23 @@ int ib_send_cm_req(struct ib_cm_id *cm_id,
>>>>>> 	cm_id_priv->responder_resources =3D param->responder_resources;
>>>>>> 	cm_id_priv->retry_count =3D param->retry_count;
>>>>>> 	cm_id_priv->path_mtu =3D param->primary_path->mtu;
>>>>>> -	cm_id_priv->pkey =3D param->primary_path->pkey;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * We want to send the pkey used in the BTH in packets
>>>>>> +	 * sent. This, in order for the passive side to =
determine if
>>>>>> +	 * communication is permitted by the respective pkeys.
>>>>>> +	 *
>>>>>> +	 * The pkey in the paths are derived from the MGID, =
which has
>>>>>> +	 * the full membership bit set. Hence, we retrieve the =
pkey by
>>>>>> +	 * using the address vector's pkey_index.
>>>>>=20
>>>>> The paths usually come from the SM and I don't expect SM to =
provide path
>>>>> between ports of only limited members of partition.
>>>>=20
>>>> In my case, it does.=20
>>>=20
>>> Which OpenSM flavor are you talking about (upstream, MLNX, or other) =
?
>>=20
>> I guess other is the right term, as its Oracle=E2=80=99s flavour, =
3.2.6_20170609.
>=20
> I'll check upstream OpenSM when I get a chance to see if same problem
> exists. Oracle OpenSM forked from upstream long time ago and not sure
> how similar the path record code is. There were various impacts for =
the
> shared port virtualization (for CX-3) in terms of adding the ability =
to
> allow both pkeys.

I just had a chat with Line H. here in Oracle. She confirms that =
Oracle=E2=80=99s SM and the upstream one act similar in this matter.

Further, using shared port virtualization, OpenSM will adhere to the =
physical ports only, and doesn=E2=80=99t even know the PKey assignments =
to the VFs assigned the VMs.

>>> What is OpenSM version string ? Is allow_both_pkeys TRUE or FALSE ?
>>=20
>> The ports do have both limited and full memberships, and the =
partitions.conf file contains both =E2=80=9Climited=E2=80=9D, =
=E2=80=9Cboth=E2=80=9D, and the =E2=80=9Cfull=E2=80=9D keywords.
>>=20
>>>>> Default ACM provider
>>>>> forms path from multicast group parameters including pkey. Is that =
the
>>>>> scenario of concern ?
>>>>=20
>>>> Also RDMA CM does that. Do an ibdump of a CM REQ message sent from =
a limited port, and you will see the PKey is the full member in the CM =
REQ msg.
>>>=20
>>> Not exactly sure what you mean by RDMA CM does this as there are a
>>> number of different ways to use RDMA CM.
>>=20
>> I am talking specifically about the CM REQ message. For example, if =
you run an IB-UV test program and specifies that RDMA CM should be used =
for connection management. Or it could be kernel ULPs. My point is that =
the pkey field in the CM REQ message (i.e. not the one in the BTH) is =
always the full one, even though the port uses the limited one.
>=20
> When you write "port uses limited one", you mean BTH:pkey is limited
> member but REQ:pkey is full member, right ?

Yes, but the BTH:pkey is, as stated above most often the default pkey =
(limited or full), whilst REQ:pkey is a designated, limited pkey.

>>> Are you referring to using
>>> IPoIB for address resolution ? Another way is for a path record to =
be
>>> passed in from user space and there is no control over it=E2=80=99s =
contents.
>>=20
>> I did not bring up the path record or address resolution issue ;-)
>=20
> You mentioned that "The pkey in the paths are derived from the MGID" =
and
> brought this issue up in my thinking about this issue.

Yes, just to inform why it is full - when it shouldn=E2=80=99t be.

>> But I guess you are alluding to the limited-to-limited case should 1) =
not have been able to perform address resolution and/or 2) the path =
record should not have been returned. I agree to both 1) and 2).
>>=20
>> But if both 1) and 2) succeeds, this patch series is a last resort to =
stop the connection to be established and avoid pkey trap violation =
traps being sent.
>>=20
>>> At a minimum, I think the comment mentioning MGID should be =
clarified.
>>=20
>> Is the question if an MC group attached by a limited port will =
include other limited ports or only full member ports?=20
>>=20
>> With the OpenSM I am running, the MC group contains limited members.
>=20
> That is problematic as any sending to MC group by limited members will
> cause pkey violation traps to SM.

Sure, agree, but a separate issue from this series which handles the =
unicast case.

>>>>> If so, I still don't fully understand the scenario
>>>>> because limited members are not supposed to be part of a multicast
>>>>> group. There was some work started to extend this for =
client/server
>>>>> model but it was never completed. However, there may be hole(s) in
>>>>> various components of implementation which open(s) this door.
>>>>=20
>>>> I view OpenSM not returning a valid path between two limited =
members an orthogonal issue, as OpenSM is another component.
>>>=20
>>> Agreed. I think that there are other components too...
>>>=20
>>>> I think the CM REQ message should contain the correct PKey (fixed =
by this patch series).
>>>>=20
>>>> And in the event the passive side being a limited member and =
receives a CM REQ with a limited PKey, that connection should not be =
formed (fixed by this patch series).
>>> This is the backward compatibility for the current behavior on the
>>> active side so that REQ gets rejected rather than accepted, right ?
>>=20
>> Not sure about the backwards compatibility part, but I certainly =
agree to the rest. Put it another way, when the MAD payload=E2=80=99s =
pkey is limited in the CM REQ, and there is only a limited matching key =
in the pkey cache, a REJ is sent back.
>=20
> Understood.
>=20
>>> In general, the approach used when there is pkey mismatch is to =
silently
>>> drop packet rather than respond which causes a timeout on the =
requester
>>> side. In this specific case, I=E2=80=99m not sure which approach is =
better.
>>=20
>> It is not silently dropped when the port supports pkey trap messages. =
Then a trap message is sent to the OpenSM.
>=20
> I was only talking about the requester-responder part. Yes, pkey
> security trap would go to SM.
>=20
> As I wrote, I'm not sure which is preferable: to explictly reject as
> this patch proposes or just silently drop and penalize requester.

Since the infrastructure is in place, i.e., the REJ reasons, my view is =
that an explicit REJ with a distinct reason is the most obvious.


Thxs, H=C3=A5kon

>=20
> -- Hal
>=20
>>> Also, is there a similar issue with SIDR_REQ ?
>>=20
>> Good point, most probaly!
>>=20
>>> Read me correct, I am also in favour of fixing the OpenSM to not =
return a valid (but useless) path record in this case.
>>>=20
>>> Understood and agreed.
>>=20
>>=20
>> Thxs, H=C3=A5kon
>>=20
>>>=20
>>> -- Hal
>>>=20
>>>>=20
>>>> Thxs, H=C3=A5kon
>>>>=20
>>>>=20
>>>>>=20
>>>>> -- Hal
>>>>>=20
>>>>>> +	 */
>>>>>> +	ret =3D ib_get_cached_pkey(cm_id_priv->id.device,
>>>>>> +				 cm_id_priv->av.port->port_num,
>>>>>> +				 cm_id_priv->av.pkey_index,
>>>>>> +				 &cm_id_priv->pkey);
>>>>>> +	if (ret)
>>>>>> +		goto error1;
>>>>>> +
>>>>>> 	cm_id_priv->qp_type =3D param->qp_type;
>>>>>>=20
>>>>>> 	ret =3D cm_alloc_msg(cm_id_priv, &cm_id_priv->msg);
>>>>>> @@ -1956,16 +1974,19 @@ static int cm_req_handler(struct cm_work =
*work)
>>>>>> 				 cm_id_priv);
>>>>>> 	if (ret) {
>>>>>> 		int err;
>>>>>> +		int rej_reason =3D (ret =3D=3D -ENOENT ?
>>>>>> +				  IB_CM_REJ_INVALID_PKEY :
>>>>>> +				  IB_CM_REJ_INVALID_GID);
>>>>>>=20
>>>>>> 		err =3D ib_get_cached_gid(work->port->cm_dev->ib_device,
>>>>>> 					work->port->port_num, 0,
>>>>>> 					&work->path[0].sgid,
>>>>>> 					NULL);
>>>>>> 		if (err)
>>>>>> -			ib_send_cm_rej(cm_id, =
IB_CM_REJ_INVALID_GID,
>>>>>> +			ib_send_cm_rej(cm_id, rej_reason,
>>>>>> 				       NULL, 0, NULL, 0);
>>>>>> 		else
>>>>>> -			ib_send_cm_rej(cm_id, =
IB_CM_REJ_INVALID_GID,
>>>>>> +			ib_send_cm_rej(cm_id, rej_reason,
>>>>>> 				       &work->path[0].sgid,
>>>>>> 				       sizeof(work->path[0].sgid),
>>>>>> 				       NULL, 0);
>>>>>> @@ -1975,7 +1996,11 @@ static int cm_req_handler(struct cm_work =
*work)
>>>>>> 		ret =3D cm_init_av_by_path(&work->path[1], =
&cm_id_priv->alt_av,
>>>>>> 					 cm_id_priv);
>>>>>> 		if (ret) {
>>>>>> -			ib_send_cm_rej(cm_id, =
IB_CM_REJ_INVALID_ALT_GID,
>>>>>> +			int rej_reason =3D (ret =3D=3D -ENOENT ?
>>>>>> +					  IB_CM_REJ_INVALID_PKEY =
:
>>>>>> +					  =
IB_CM_REJ_INVALID_ALT_GID);
>>>>>> +
>>>>>> +			ib_send_cm_rej(cm_id, rej_reason,
>>>>>> 				       &work->path[0].sgid,
>>>>>> 				       sizeof(work->path[0].sgid), NULL, =
0);
>>>>>> 			goto rejected;
>>>>>> diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
>>>>>> index 7979cb04f529..56b62303946a 100644
>>>>>> --- a/include/rdma/ib_cm.h
>>>>>> +++ b/include/rdma/ib_cm.h
>>>>>> @@ -3,6 +3,7 @@
>>>>>> * Copyright (c) 2004 Topspin Corporation.  All rights reserved.
>>>>>> * Copyright (c) 2004 Voltaire Corporation.  All rights reserved.
>>>>>> * Copyright (c) 2005 Sun Microsystems, Inc. All rights reserved.
>>>>>> + * Copyright (c) 2018 Oracle and/or its affiliates. All rights =
reserved.
>>>>>> *
>>>>>> * This software is available to you under a choice of one of two
>>>>>> * licenses.  You may choose to be licensed under the terms of the =
GNU
>>>>>> @@ -183,7 +184,8 @@ enum ib_cm_rej_reason {
>>>>>> 	IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID	=3D 30,
>>>>>> 	IB_CM_REJ_INVALID_CLASS_VERSION		=3D 31,
>>>>>> 	IB_CM_REJ_INVALID_FLOW_LABEL		=3D 32,
>>>>>> -	IB_CM_REJ_INVALID_ALT_FLOW_LABEL	=3D 33
>>>>>> +	IB_CM_REJ_INVALID_ALT_FLOW_LABEL	=3D 33,
>>>>>> +	IB_CM_REJ_INVALID_PKEY			=3D 34,
>>>>>> };
>>>>>>=20
>>>>>> struct ib_cm_rej_event_param {
>>>>>>=20
>>>>> --
>>>>> 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
>>>>=20
>>>>=20
>>> --
>>> 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
>>=20
>>=20
> --
> 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