Date:   Thu, 7 Mar 2019 10:09:32 +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 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.

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