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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAOhMmr5ucF3pa4jp9RLEzJNs29oVT0qAXmywNnd+Xe2seoRJfg@mail.gmail.com>
Date:   Tue, 4 May 2021 14:31:15 -0500
From:   Lijun Pan <lijunp213@...il.com>
To:     Dany Madden <drt@...ux.ibm.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 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.

> 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.
>
>
> >
> >                         if (adapter->state == VNIC_OPEN) {
> >                                 /* When we dropped rtnl, ibmvnic_open() got
> > --
> > 2.18.2
> >