Re: [net-next PATCH v2] net/rds: Accept peer connection reject messages due to incompatible version

[Yanjun Zhu <yanjun.zhu@oracle.com>]

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@oracle.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@oracle.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@oracle.com>

Thanks, Gerd.

Zhu Yanjun

>
> So I send my tested-by in advance.
>
> Reviewed-and-tested-by: Zhu Yanjun <yanjun.zhu@oracle.com>
>
> Thanks,
>
> Zhu Yanjun
>
>> so far you are unable to reproduce the issue on net-next. Thanks.
>>
>> Regards,
>> Santosh