linux-kernel - Re: [PATCH v3] workqueue: don't skip lockdep work dependency in cancel_work

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: <30f87339d12e258f2d8ad44b1e59fc8acff1a5f5.camel@sipsolutions.net>
Date:   Wed, 13 Sep 2023 20:30:37 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Heyi Guo <guoheyi@...ux.alibaba.com>
Cc:     Lai Jiangshan <jiangshanlai@...il.com>, Tejun Heo <tj@...nel.org>,
        Hillf Danton <hdanton@...a.com>,
        LKML <linux-kernel@...r.kernel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH v3] workqueue: don't skip lockdep work dependency in
 cancel_work_sync()

On Wed, 2023-09-13 at 10:25 -0700, Florian Fainelli wrote:
> 
> I would refrain from reverting without giving a fighting chance to the 
> author to address it. It seems a bit strange that we do this locking 
> dance while it might have been simpler to introduce a 
> ftgmac100_reset_unlocked() and ftgmac100_reset() and call both at the 
> intended places, something like the completely untested patch attached 
> maybe?

Not sure that's right - it probably wants RTNL for some reason, but with
this patch you don't hold it when coming from ftgmac100_adjust_link()?
(If it did, it'd have deadlocked getting here after all, since it
acquires it)

Not sure why it needs RTNL though, that doesn't seem so bad, and holds
some internal locks. Or maybe it doesn't really, only if there's a
phydev and/or mii_bus, so maybe it needs a driver lock? Well, there's a
note about the reset task, that might be it?

static int ftgmac100_stop(struct net_device *netdev)
{
        struct ftgmac100 *priv = netdev_priv(netdev);

        /* Note about the reset task: We are called with the rtnl lock
         * held, so we are synchronized against the core of the reset
         * task. We must not try to synchronously cancel it otherwise
         * we can deadlock. But since it will test for netif_running()
         * which has already been cleared by the net core, we don't
         * anything special to do.
         */



But it really kind of feels the locking model in this driver is a bit
off.

johannes