linux-kernel - Re: WARN @lib/refcount.c:128 during hot unplug of I/O adapter.

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: <87efwyv0j7.fsf@concordia.ellerman.id.au>
Date:   Wed, 12 Apr 2017 12:10:20 +1000
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Tyrel Datwyler <turtle.in.the.kernel@...il.com>,
        Tyrel Datwyler <tyreld@...ux.vnet.ibm.com>,
        Sachin Sant <sachinp@...ux.vnet.ibm.com>,
        linuxppc-dev@...abs.org
Cc:     Nathan Fontenot <nfont@...ux.vnet.ibm.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: WARN @lib/refcount.c:128 during hot unplug of I/O adapter.

Tyrel Datwyler <turtle.in.the.kernel@...il.com> writes:
> On 04/11/2017 02:00 AM, Michael Ellerman wrote:
>> Tyrel Datwyler <tyreld@...ux.vnet.ibm.com> writes:
>>> I started looking at it when Bharata submitted a patch trying to fix the
>>> issue for CPUs, but got side tracked by other things. I suspect that
>>> this underflow has actually been an issue for quite some time, and we
>>> are just now becoming aware of it thanks to the recount_t patchset being
>>> merged.
>> 
>> Yes I agree. Which means it might be broken in existing distros.
>
> Definitely. I did some profiling last night, and I understand the
> hotplug case. It turns out to be as I suggested in the original thread
> about CPUs. When the devicetree code was worked to move the tree out of
> proc and into sysfs the sysfs detach code added a of_node_put to remove
> the original of_init reference. pSeries Being the sole original
> *dynamic* device tree user we had always issued a of_node_put in our
> dlpar specific detach function to achieve that end. So, this should be a
> pretty straight forward trivial fix.

Excellent, thanks.

> However, for the case where devices are present at boot it appears we a
> leaking a lot of references resulting in the device nodes never actually
> being released/freed after a dlpar remove. In the CPU case after boot I
> count 8 more references taken than the hotplug case, and corresponding
> of_node_put's are not called at dlpar remove time either. That will take
> some time to track them down, review and clean up.

Yes that is a perennial problem unfortunately which we've never come up
with a good solution for.

The (old) patch below might help track some of them down. I remember
having a script to process the output of the trace and find mismatches,
but I can't find it right now - but I'm sure you can hack up something
:)

cheers


diff --git a/arch/powerpc/include/asm/trace.h b/arch/powerpc/include/asm/trace.h
index 32e36b16773f..ad32365082a0 100644
--- a/arch/powerpc/include/asm/trace.h
+++ b/arch/powerpc/include/asm/trace.h
@@ -168,6 +168,44 @@ TRACE_EVENT(hash_fault,
 		      __entry->addr, __entry->access, __entry->trap)
 );
 
+TRACE_EVENT(of_node_get,
+
+	TP_PROTO(struct device_node *dn, int val),
+
+	TP_ARGS(dn, val),
+
+	TP_STRUCT__entry(
+		__field(struct device_node *, dn)
+		__field(int, val)
+	),
+
+	TP_fast_assign(
+		__entry->dn = dn;
+		__entry->val = val;
+	),
+
+	TP_printk("get %d -> %d %s", __entry->val - 1, __entry->val, __entry->dn->full_name)
+);
+
+TRACE_EVENT(of_node_put,
+
+	TP_PROTO(struct device_node *dn, int val),
+
+	TP_ARGS(dn, val),
+
+	TP_STRUCT__entry(
+		__field(struct device_node *, dn)
+		__field(int, val)
+	),
+
+	TP_fast_assign(
+		__entry->dn = dn;
+		__entry->val = val;
+	),
+
+	TP_printk("put %d -> %d %s", __entry->val + 1, __entry->val, __entry->dn->full_name)
+);
+
 #endif /* _TRACE_POWERPC_H */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index c647bd1b6903..f5c3d761f3cd 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -14,6 +14,8 @@
 
 #include "of_private.h"
 
+#include <asm/trace.h>
+
 /**
  * of_node_get() - Increment refcount of a node
  * @node:	Node to inc refcount, NULL is supported to simplify writing of
@@ -23,8 +25,12 @@
  */
 struct device_node *of_node_get(struct device_node *node)
 {
-	if (node)
+	if (node) {
 		kobject_get(&node->kobj);
+
+		trace_of_node_get(node, atomic_read(&node->kobj.kref.refcount));
+	}
+
 	return node;
 }
 EXPORT_SYMBOL(of_node_get);
@@ -36,8 +42,10 @@ EXPORT_SYMBOL(of_node_get);
  */
 void of_node_put(struct device_node *node)
 {
-	if (node)
+	if (node) {
 		kobject_put(&node->kobj);
+		trace_of_node_put(node, atomic_read(&node->kobj.kref.refcount));
+	}
 }
 EXPORT_SYMBOL(of_node_put);