netdev - Re: [PATCH net] r8152: Re-order napi_disable in rtl8152

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

Message-ID: <CACeCKacWVFUPGhXaVJsS+CR+SNCa-9u3KfNK7QzH_bFPczC6uQ@mail.gmail.com>
Date:   Thu, 21 Nov 2019 00:50:18 -0800
From:   Prashant Malani <pmalani@...omium.org>
To:     Hayes Wang <hayeswang@...ltek.com>
Cc:     "grundler@...omium.org" <grundler@...omium.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        nic_swsd <nic_swsd@...ltek.com>
Subject: Re: [PATCH net] r8152: Re-order napi_disable in rtl8152_close

On Wed, Nov 20, 2019 at 7:00 PM Hayes Wang <hayeswang@...ltek.com> wrote:
>
> Prashant Malani [mailto:pmalani@...omium.org]
> > Sent: Thursday, November 21, 2019 3:40 AM
> [...]
> > @@ -4283,10 +4283,10 @@ static int rtl8152_close(struct net_device
> > *netdev)
> >       unregister_pm_notifier(&tp->pm_notifier);
> >  #endif
> >       tasklet_disable(&tp->tx_tl);
>
> Should tasklet_disable() be moved, too?
Perhaps; I'm not too familiar with what the tasklet bottom half does
to be able to conclusively say. Probably best to leave it as is
(somewhat symmetrical to rtl8152_open()) if it is not causing any
races in its current location?
Moving if after cancel_delayed_work_sync() would effectively not do
much IIUC since WORK_ENABLE is already cleared by then and that is one
of the guard clauses inside bottom_half(see :
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/usb/r8152.c#n2423),
so effectively, it's disabled as soon as WORK_ENABLE is cleared. I
might be mistaken here though.
>
> > -     napi_disable(&tp->napi);
> >       clear_bit(WORK_ENABLE, &tp->flags);
> >       usb_kill_urb(tp->intr_urb);
> >       cancel_delayed_work_sync(&tp->schedule);
> > +     napi_disable(&tp->napi);
> >       netif_stop_queue(netdev);
>
> Best Regards,
> Hayes
>
>