netdev - Re: [PATCH net v3] net/smc: avoid data corruption caused by decline

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ccf3e279-b9d2-5bd1-b033-8071471720e0@linux.alibaba.com>
Date: Mon, 20 Nov 2023 17:55:20 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com>
To: Wen Gu <guwen@...ux.alibaba.com>, kgraul@...ux.ibm.com,
 wenjia@...ux.ibm.com, jaka@...ux.ibm.com, wintera@...ux.ibm.com
Cc: kuba@...nel.org, davem@...emloft.net, netdev@...r.kernel.org,
 linux-s390@...r.kernel.org, linux-rdma@...r.kernel.org,
 tonylu@...ux.alibaba.com, pabeni@...hat.com, edumazet@...gle.com
Subject: Re: [PATCH net v3] net/smc: avoid data corruption caused by decline



On 11/20/23 11:37 AM, Wen Gu wrote:
>
>
> On 2023/11/19 23:28, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@...ux.alibaba.com>
>>
>> We found a data corruption issue during testing of SMC-R on Redis
>> applications.
>>
>> The benchmark has a low probability of reporting a strange error as
>> shown below.
>>
>> "Error: Protocol error, got "\xe2" as reply type byte"
>>
>> Finally, we found that the retrieved error data was as follows:
>>
>> 0xE2 0xD4 0xC3 0xD9 0x04 0x00 0x2C 0x20 0xA6 0x56 0x00 0x16 0x3E 0x0C
>> 0xCB 0x04 0x02 0x01 0x00 0x00 0x20 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>> 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0xE2
>>
>> It is quite obvious that this is a SMC DECLINE message, which means that
>> the applications received SMC protocol message.
>> We found that this was caused by the following situations:
>>
>> client                  server
>>          ¦  proposal
>>          ------------->
>>          ¦  accept
>>          <-------------
>>          ¦  confirm
>>          ------------->
>> wait confirm
>
> I think there may be an ambiguity here, better for 'wait for llc 
> confirm link'.
> Could you please add 'clc' and 'llc' prefix to distinguish flows on 
> the diagram?
>

Looks Reasonable. I'll make changes in the next revision.

D. Wythe

> Thanks.
>
>>
>>          ¦failed llc confirm
>>          ¦   x------
>> (after 2s)timeout
>>                          wait rsp
>>
>> wait decline
>>
>> (after 1s) timeout
>>                          (after 2s) timeout
>>          ¦   decline
>>          -------------->
>>          ¦   decline
>>          <--------------
>>
>> As a result, a decline message was sent in the implementation, and this
>> message was read from TCP by the already-fallback connection.
>>
>> This patch double the client timeout as 2x of the server value,
>> With this simple change, the Decline messages should never cross or
>> collide (during Confirm link timeout).
>>
>> This issue requires an immediate solution, since the protocol updates
>> involve a more long-term solution.
>>
>> Fixes: 0fb0b02bd6fd ("net/smc: adapt SMC client code to use the LLC 
>> flow")
>> Signed-off-by: D. Wythe <alibuda@...ux.alibaba.com>
>
>
>> ---
>>   net/smc/af_smc.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index abd2667..8615cc0 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -598,8 +598,12 @@ static int smcr_clnt_conf_first_link(struct 
>> smc_sock *smc)
>>       struct smc_llc_qentry *qentry;
>>       int rc;
>>   -    /* receive CONFIRM LINK request from server over RoCE fabric */
>> -    qentry = smc_llc_wait(link->lgr, NULL, SMC_LLC_WAIT_TIME,
>> +    /* Receive CONFIRM LINK request from server over RoCE fabric.
>> +     * Increasing the client's timeout by twice as much as the server's
>> +     * timeout by default can temporarily avoid decline messages of
>> +     * both sides crossing or colliding
>> +     */
>> +    qentry = smc_llc_wait(link->lgr, NULL, 2 * SMC_LLC_WAIT_TIME,
>>                     SMC_LLC_CONFIRM_LINK);
>>       if (!qentry) {
>>           struct smc_clc_msg_decline dclc;