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

["D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx>]

On 11/17/23 2:47 PM, Wen Gu wrote:
> On 2023/11/17 12:59, D. Wythe wrote:
>
> > From: "D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxx>
> > ---
> >   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);