netdev - Re: [PATCH net v3] ibmvnic: Continue with reset if set link down failed

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 04 May 2021 13:24:55 -0700
From:   Dany Madden <drt@...ux.ibm.com>
To:     Lijun Pan <lijunp213@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Sukadev Bhattiprolu <sukadev@...ux.ibm.com>,
        Thomas Falcon <tlfalcon@...ux.ibm.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>, netdev@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH net v3] ibmvnic: Continue with reset if set link down
 failed

On 2021-05-04 12:31, Lijun Pan wrote:
> On Tue, May 4, 2021 at 2:27 PM Lijun Pan <lijunp213@...il.com> wrote:
>> 
>> On Tue, May 4, 2021 at 2:14 PM Dany Madden <drt@...ux.ibm.com> wrote:
>> >
>> > When ibmvnic gets a FATAL error message from the vnicserver, it marks
>> > the Command Respond Queue (CRQ) inactive and resets the adapter. If this
>> > FATAL reset fails and a transmission timeout reset follows, the CRQ is
>> > still inactive, ibmvnic's attempt to set link down will also fail. If
>> > ibmvnic abandons the reset because of this failed set link down and this
>> > is the last reset in the workqueue, then this adapter will be left in an
>> > inoperable state.
>> >
>> > Instead, make the driver ignore this link down failure and continue to
>> > free and re-register CRQ so that the adapter has an opportunity to
>> > recover.
>> >
>> > Fixes: ed651a10875f ("ibmvnic: Updated reset handling")
>> > Signed-off-by: Dany Madden <drt@...ux.ibm.com>
>> > Reviewed-by: Rick Lindsley <ricklind@...ux.ibm.com>
>> > Reviewed-by: Sukadev Bhattiprolu <sukadev@...ux.ibm.com>
>> > ---
>> > Changes in V2:
>> > - Update description to clarify background for the patch
>> > - Include Reviewed-by tags
>> > Changes in V3:
>> > - Add comment above the code change
>> > ---
>> >  drivers/net/ethernet/ibm/ibmvnic.c | 11 +++++++++--
>> >  1 file changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> > index 5788bb956d73..9e005a08d43b 100644
>> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> > @@ -2017,8 +2017,15 @@ static int do_reset(struct ibmvnic_adapter *adapter,
>> >                         rtnl_unlock();
>> >                         rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_DN);
>> >                         rtnl_lock();
>> > -                       if (rc)
>> > -                               goto out;
>> > +
>> > +                       /* Attempted to set the link down. It could fail if the
>> > +                        * vnicserver has already torn down the CRQ. We will
>> > +                        * note it and continue with reset to reinit the CRQ.
>> > +                        */
>> > +                       if (rc) {
>> > +                               netdev_dbg(netdev,
>> > +                                          "Setting link down failed rc=%d. Continue anyway\n", rc);
>> > +                       }
>> 
>> There are other places which check and rely on the return value of
>> this function. Your change makes that inconsistent. Can you stop
> 
> To be more specific, __ibmvnic_close, __ibmvnic_open both call this
> set_link_state.
Inconsistent would have been not checking for the rc at all. Here we 
checked and noted it that there are times that it's ok to continue.

> 
>> posting new versions and soliciting the maintainer to accept it before
>> there is material change? There are many ways to make reset
>> successful. I think this is the worst approach of all.

Can you show me a patch that is better than this one, that has gone thru 
a 30+ hours of testing?

>> 
>> 
>> >
>> >                         if (adapter->state == VNIC_OPEN) {
>> >                                 /* When we dropped rtnl, ibmvnic_open() got
>> > --
>> > 2.18.2
>> >