Re: [PATCH net v2] net/smc: avoid data corruption caused by decline

["D. Wythe" <alibuda@linux.alibaba.com>]

On 11/17/23 2:47 PM, Wen Gu wrote:
>
>
> On 2023/11/17 12:59, D. Wythe wrote:
>
>> From: "D. Wythe" <alibuda@linux.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
>>
>>      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,
>
> Is the client's timeout doubled?
>
> From the code below, it is server's timeout that has been doubled.
>

Forget to fix description, i'll fix that in next revision.

>> 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@linux.alibaba.com>
>> ---
>>   include/net/netns/smc.h |  2 ++
>>   net/smc/af_smc.c        |  3 ++-
>>   net/smc/smc_sysctl.c    | 12 ++++++++++++
>>   3 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/netns/smc.h b/include/net/netns/smc.h
>> index 582212a..5198896 100644
>> --- a/include/net/netns/smc.h
>> +++ b/include/net/netns/smc.h
>> @@ -22,5 +22,7 @@ struct netns_smc {
>>       int                sysctl_smcr_testlink_time;
>>       int                sysctl_wmem;
>>       int                sysctl_rmem;
>> +    /* server's Confirm Link timeout in seconds */
>> +    int                sysctl_smcr_srv_confirm_link_timeout;
>>   };
>>   #endif
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index abd2667..b86ad30 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -1870,7 +1870,8 @@ static int smcr_serv_conf_first_link(struct 
>> smc_sock *smc)
>>           return SMC_CLC_DECL_TIMEOUT_CL;
>>         /* receive CONFIRM LINK response from client over the RoCE 
>> fabric */
>> -    qentry = smc_llc_wait(link->lgr, link, SMC_LLC_WAIT_TIME,
>> +    qentry = smc_llc_wait(link->lgr, link,
>> + sock_net(&smc->sk)->smc.sysctl_smcr_srv_confirm_link_timeout,
>>                     SMC_LLC_CONFIRM_LINK);
>>       if (!qentry) {
>>           struct smc_clc_msg_decline dclc;
>> diff --git a/net/smc/smc_sysctl.c b/net/smc/smc_sysctl.c
>> index 5cbc18c..919f3f7 100644
>> --- a/net/smc/smc_sysctl.c
>> +++ b/net/smc/smc_sysctl.c
>> @@ -51,6 +51,13 @@
>>           .proc_handler    = proc_dointvec_jiffies,
>>       },
>>       {
>> +        .procname    = "smcr_srv_confirm_link_timeout",
>> +        .data        = 
>> &init_net.smc.sysctl_smcr_srv_confirm_link_timeout,
>> +        .maxlen        = sizeof(int),
>> +        .mode        = 0644,
>> +        .proc_handler    = proc_dointvec_jiffies,
>> +    },
>> +    {
>>           .procname    = "wmem",
>>           .data        = &init_net.smc.sysctl_wmem,
>>           .maxlen        = sizeof(int),
>> @@ -95,6 +102,11 @@ int __net_init smc_sysctl_net_init(struct net *net)
>>       net->smc.sysctl_autocorking_size = SMC_AUTOCORKING_DEFAULT_SIZE;
>>       net->smc.sysctl_smcr_buf_type = SMCR_PHYS_CONT_BUFS;
>>       net->smc.sysctl_smcr_testlink_time = 
>> SMC_LLC_TESTLINK_DEFAULT_TIME;
>> +    /* Increasing the server's timeout by twice as much as the client's
>> +     * timeout by default can temporarily avoid decline messages of
>> +     * both side been crossed or collided.
>
> 'both sides' or maybe better for
>
> '..avoid decline messages of both sides crossing or colliding.'
>
>
Look nice. I'll adopt that.
>
> Thanks,
> Wen Gu
>
>> +     */
>> +    net->smc.sysctl_smcr_srv_confirm_link_timeout = 2 * 
>> SMC_LLC_WAIT_TIME;
>>       WRITE_ONCE(net->smc.sysctl_wmem, net_smc_wmem_init);
>>       WRITE_ONCE(net->smc.sysctl_rmem, net_smc_rmem_init);