| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 7 Mar 2019 11:28:36 +0800
From: Yanjun Zhu <yanjun.zhu@...cle.com>
To: Santosh Shilimkar <santosh.shilimkar@...cle.com>,
Gerd Rausch <gerd.rausch@...cle.com>, netdev@...r.kernel.org
Cc: David Miller <davem@...emloft.net>
Subject: Re: [net-next PATCH v2] net/rds: Accept peer connection reject
messages due to incompatible version
On 2019/3/7 10:09, Yanjun Zhu wrote:
>
> On 2019/3/7 9:55, Santosh Shilimkar wrote:
>>
>>
>> On 3/6/2019 5:49 PM, Gerd Rausch wrote:
>>> Prior to
>>> commit d021fabf525ff ("rds: rdma: add consumer reject")
>>>
>>> function "rds_rdma_cm_event_handler_cmn" would always honor a rejected
>>> connection attempt by issuing a "rds_conn_drop".
>>>
>>> The commit mentioned above added a "break", eliminating
>>> the "fallthrough" case and made the "rds_conn_drop" rather conditional:
>>>
>>> Now it only happens if a "consumer defined" reject (i.e. "rdma_reject")
>>> carries an integer-value of "1" inside "private_data":
>>>
>>>> if (!conn)
>>>> + break;
>>>> + err = (int *)rdma_consumer_reject_data(cm_id,
>>>> event, &len);
>>>> + if (!err || (err && ((*err) ==
>>>> RDS_RDMA_REJ_INCOMPAT))) {
>>>> + pr_warn("RDS/RDMA: conn <%pI6c, %pI6c>
>>>> rejected, dropping connection\n",
>>>> + &conn->c_laddr, &conn->c_faddr);
>>>> + conn->c_proposed_version =
>>>> RDS_PROTOCOL_COMPAT_VERSION;
>>>> + rds_conn_drop(conn);
>>>> + }
>>>> rdsdebug("Connection rejected: %s\n",
>>>> rdma_reject_msg(cm_id, event->status));
>>>> + break;
>>>> /* FALLTHROUGH */
>>>
>>> A number of issues are worth mentioning here:
>>> #1) Previous versions of the RDS code simply rejected a connection
>>> by calling "rdma_reject(cm_id, NULL, 0);"
>>> So the value of the payload in "private_data" will not be "1",
>>> but "0".
>>>
>>> #2) Now the code has become dependent on host byte order and sizing.
>>> If one peer is big-endian, the other is little-endian,
>>> or there's a difference in sizeof(int) (e.g. ILP64 vs LP64),
>>> the *err check does not work as intended.
>>>
>>> #3) There is no check for "len" to see if the data behind *err is
>>> even valid.
>>> Luckily, it appears that the "rdma_reject(cm_id, NULL, 0)"
>>> will always
>>> carry 148 bytes of zeroized payload.
>>> But that should probably not be relied upon here.
>>>
>>> #4) With the added "break;",
>>> we might as well drop the misleading "/* FALLTHROUGH */"
>>> comment.
>>>
>>> This commit does _not_ address issue #2, as the sender would have to
>>> agree on a byte order as well.
>>>
>>> Here is the sequence of messages in this observed error-scenario:
>>> Host-A is pre-QoS changes (excluding the commit mentioned above)
>>> Host-B is post-QoS changes (including the commit mentioned above)
>>>
>>> #1 Host-B
>>> issues a connection request via function
>>> "rds_conn_path_transition"
>>> connection state transitions to "RDS_CONN_CONNECTING"
>>>
>>> #2 Host-A
>>> rejects the incompatible connection request (from #1)
>>> It does so by calling "rdma_reject(cm_id, NULL, 0);"
>>>
>>> #3 Host-B
>>> receives an "RDMA_CM_EVENT_REJECTED" event (from #2)
>>> But since the code is changed in the way described above,
>>> it won't drop the connection here, simply because "*err == 0".
>>>
>>> #4 Host-A
>>> issues a connection request
>>>
>>> #5 Host-B
>>> receives an "RDMA_CM_EVENT_CONNECT_REQUEST" event
>>> and ends up calling "rds_ib_cm_handle_connect".
>>> But since the state is already in "RDS_CONN_CONNECTING"
>>> (as of #1) it will end up issuing a "rdma_reject" without
>>> dropping the connection:
>>> if (rds_conn_state(conn) == RDS_CONN_CONNECTING) {
>>> /* Wait and see - our connect may still be succeeding */
>>> rds_ib_stats_inc(s_ib_connect_raced);
>>> }
>>> goto out;
>>>
>>> #6 Host-A
>>> receives an "RDMA_CM_EVENT_REJECTED" event (from #5),
>>> drops the connection and tries again (goto #4) until it gives up.
>>>
>>> Fixes: d021fabf525ff ("rds: rdma: add consumer reject")
>>> Signed-off-by: Gerd Rausch <gerd.rausch@...cle.com>
>>> ---
>>> net/rds/rdma_transport.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> Changes in submitted patch v2:
>>> * Dropped the "Orabug:" line from the commit-log message (as requested)
>>> * Added a "Fixes:" line to the commit-log-message
>>>
>> Thanks Gerd for posting an update. The fix looks correct as already
>> mentioned in earlier post.
>> FWIW,
>> Acked-by: Santosh Shilimkar<santosh.shilimkar@...cle.com>
>>
>> Hi Yanjun,
>> Please provide your tested-by since you mentioned offlist that
>
> OK. Now I am working with Gerd to reproduce this bug.
>
> I think this problem can be reproduced since Gerd is confident with it.
Sorry. The HCA device in my test env is
CA 'mlx4_0'
CA type: MT26428
Number of ports: 2
Firmware version: 2.11.2010
Hardware version: b0
Node GUID: 0x0002c903000a7a30
System image GUID: 0x0002c903000a7a33
Port 1:
State: Active
Physical state: LinkUp
Rate: 40
Base lid: 50
LMC: 0
SM lid: 26
Capability mask: 0x02590868
Port GUID: 0x0002c903000a7a31
Link layer: InfiniBand
Port 2:
State: Active
Physical state: LinkUp
Rate: 40
Base lid: 51
LMC: 0
SM lid: 26
Capability mask: 0x02590868
Port GUID: 0x0002c903000a7a32
Link layer: InfiniBand
And from Gerd
"
The setup I use that ran into the issue right way is comprised of:
2 CX4 HCAs wired up back to back in RoCE mode (LINK_TYPE=ETH)
"
Perhaps the HW causes this problem. Since I can not reproduce this bug
and make tests with this patch,
and I did review this patch, I change to this:
Reviewed-by: Zhu Yanjun <yanjun.zhu@...cle.com>
Thanks, Gerd.
Zhu Yanjun
>
> So I send my tested-by in advance.
>
> Reviewed-and-tested-by: Zhu Yanjun <yanjun.zhu@...cle.com>
>
> Thanks,
>
> Zhu Yanjun
>
>> so far you are unable to reproduce the issue on net-next. Thanks.
>>
>> Regards,
>> Santosh
Powered by blists - more mailing lists