netdev - Re: [PATCH net 3/5] net/ncsi: Fix stale link state of inactive channels on failover

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20161014103722.GB6278@gwshan>
Date:   Fri, 14 Oct 2016 21:37:23 +1100
From:   Gavin Shan <gwshan@...ux.vnet.ibm.com>
To:     Joel Stanley <joel@....id.au>
Cc:     Gavin Shan <gwshan@...ux.vnet.ibm.com>, netdev@...r.kernel.org,
        davem@...emloft.net
Subject: Re: [PATCH net 3/5] net/ncsi: Fix stale link state of inactive
 channels on failover

On Fri, Oct 14, 2016 at 04:32:28PM +1030, Joel Stanley wrote:
>On Fri, Oct 14, 2016 at 1:23 PM, Gavin Shan <gwshan@...ux.vnet.ibm.com> wrote:
>> The issue was found on BCM5718 which has two NCSI channels in one
>> package: C0 and C1. Both of them are connected to different LANs,
>> means they are in link-up state and C0 is chosen  as the active
>> one until resetting BCM5718 happens as below.
>>
>> Resetting BCM5718 results in LSC (Link State Change) AEN packet
>> received on C0, meaning LSC AEN is missed on C1. When LSC AEN packet
>> received on C0 to report link-down, it fails over to C1 because C1
>> is in link-up state as software can see. However, C1 is in link-down
>> state in hardware. It means the link state is out of synchronization
>> between hardware and software, resulting in inappropriate channel (C1)
>> selected as active one.
>>
>> This resolves the issue by sending separate GLS (Get Link Status)
>> commands to all channels in the package before trying to do failover.
>> The last link state on all channels in the package is retrieved. With
>> it, C0 is selected as active one as expected.
>
>I follow this, and can see that happening in the
>ncsi_dev_state_suspend_gls state. However, what is
>
>> -               nd->state = ncsi_dev_state_suspend_dcnt;
>> +               if (ndp->flags & NCSI_DEV_RESHUFFLE)
>> +                       nd->state = ncsi_dev_state_suspend_gls;
>> +               else
>> +                       nd->state = ncsi_dev_state_suspend_dcnt;
>
>However, what is this doing? I'm not quite sure what
>NCSI_DEV_RESHUFFLE is and why we enable it?
>

NCSI_DEV_RESHUFFLE is set when we need failover, which happens on
resetting NIC or unplugging the cable connected to the active channel
(port) or other events. The first step for failover is to suspend
currently active channel and then choose the best one (channel's link
state is important factor) to be active. ncsi_dev_state_suspend_gls
ensures we will get updated link state of available channels before
choosing and enabling next active channel. If there are no failover
happening, we needn't get the update link state on the available
channels and the state ncsi_dev_state_suspend_gls will be skipped.

I think I need put comments here to explain the change in next revision.

Thanks,
Gavin

>>
>>                 ret = ncsi_xmit_cmd(&nca);
>>                 if (ret)
>>                         goto error;
>>
>>                 break;
>> +       case ncsi_dev_state_suspend_gls:
>> +               ndp->pending_req_num = np->channel_num;
>> +
>> +               nca.type = NCSI_PKT_CMD_GLS;
>> +               nca.package = np->id;
>> +               nd->state = ncsi_dev_state_suspend_dcnt;
>> +
>> +               NCSI_FOR_EACH_CHANNEL(np, nc) {
>> +                       nca.channel = nc->id;
>> +                       ret = ncsi_xmit_cmd(&nca);
>> +                       if (ret)
>> +                               goto error;
>> +               }
>> +
>> +               break;
>>         case ncsi_dev_state_suspend_dcnt:
>>         case ncsi_dev_state_suspend_dc:
>>         case ncsi_dev_state_suspend_deselect:
>> --
>> 2.1.0
>>
>