linux-kernel - Re: sun50i-a64-pinctrl WARN

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: <20170428220314.GI22354@htj.duckdns.org>
Date:   Fri, 28 Apr 2017 18:03:14 -0400
From:   Tejun Heo <tj@...nel.org>
To:     Andre Przywara <andre.przywara@....com>
Cc:     Icenowy Zheng <icenowy@...c.xyz>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Adam Borowski <kilobyte@...band.pl>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: sun50i-a64-pinctrl WARN_ON drivers/base/dd.c:349

Hello,

On Tue, Apr 18, 2017 at 10:12:16AM +0100, Andre Przywara wrote:
> Yeah, so I stack-dumped on the zero allocations and indeed they are
> called from cleanup functions:
> drivers/pinctrl/pinmux.c:pinmux_generic_free_functions():
> 	devm_kzalloc(sizeof(*indices) * pctldev->num_functions, ...)
> (and another one I don't know from the top of the my head, logs at home)
>
> So my hunch was that once EPROBE_DEFER triggers the devres cleanup, it
> uses some reverse list traversal to release all allocated resources
> (backwards!), so missing those which get (appended) during the process.
> But I don't think that would not work with the locking.
> So I have to dig deeper tonight in my logs.

If this is a valid use case, we can change devm to repeat till empty
but it's a weird thing to do to allocate from a release function.

So, something like this.  Only compile tested.

Thanks.

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 71d5770..d2a9f34 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -509,13 +509,21 @@ static int release_nodes(struct device *dev, struct list_head *first,
 int devres_release_all(struct device *dev)
 {
 	unsigned long flags;
+	int cnt = 0, ret;
 
 	/* Looks like an uninitialized device structure */
 	if (WARN_ON(dev->devres_head.next == NULL))
 		return -ENODEV;
-	spin_lock_irqsave(&dev->devres_lock, flags);
-	return release_nodes(dev, dev->devres_head.next, &dev->devres_head,
-			     flags);
+
+	/* Release callbacks may create new nodes, repeat till empty */
+	do {
+		spin_lock_irqsave(&dev->devres_lock, flags);
+		ret = release_nodes(dev, dev->devres_head.next,
+				    &dev->devres_head, flags);
+		cnt += ret;
+	} while (ret);
+
+	return cnt;
 }
 
 /**