[PATCH] klist: avoid accesses after waking klist_remove()

[PATCH] klist: avoid accesses after waking klist_remove()

[Karl Mehltretter]

klist_release() publishes waiter->woken before its final accesses to the
stack waiter and node. klist_remove() can then return, allowing the waiter
to go out of scope and its caller to free or reuse the node.

Clear n_klist and take a task reference before publishing woken. Use
release/acquire accesses for that publication and wake the referenced task.

The task reference keeps the waiter task alive if it returns and exits
before wake_up_process(). wake_up_process() provides the full barrier
required by the sleep/wakeup protocol, so remove the explicit mb().

Fixes: 8b0c250be489 ("[PATCH] add klist_node_attached() to determine if a node is on a list or not.")
Fixes: 210272a28465 ("driver core: Remove completion from struct klist_node")
Reported-by: syzbot+9cb1ac7fce4944ba9165@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9cb1ac7fce4944ba9165
Tested-by: syzbot+9cb1ac7fce4944ba9165@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 lib/klist.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lib/klist.c b/lib/klist.c
index 332a4fbf18ff..7242f4cc1c1b 100644
--- a/lib/klist.c
+++ b/lib/klist.c
@@ -36,6 +36,7 @@
 #include <linux/klist.h>
 #include <linux/export.h>
 #include <linux/sched.h>
+#include <linux/sched/task.h>
 
 /*
  * Use the lowest bit of n_klist to mark deleted nodes and exclude
@@ -187,18 +188,23 @@ static void klist_release(struct kref *kref)
 
 	WARN_ON(!knode_dead(n));
 	list_del(&n->n_node);
+	knode_set_klist(n, NULL);
 	spin_lock(&klist_remove_lock);
 	list_for_each_entry_safe(waiter, tmp, &klist_remove_waiters, list) {
+		struct task_struct *p;
+
 		if (waiter->node != n)
 			continue;
 
+		p = waiter->process;
+		get_task_struct(p);
 		list_del(&waiter->list);
-		waiter->woken = 1;
-		mb();
-		wake_up_process(waiter->process);
+		/* Publish only after the final waiter and n accesses */
+		smp_store_release(&waiter->woken, 1);
+		wake_up_process(p);
+		put_task_struct(p);
 	}
 	spin_unlock(&klist_remove_lock);
-	knode_set_klist(n, NULL);
 }
 
 static int klist_dec_and_del(struct klist_node *n)
@@ -250,7 +256,8 @@ void klist_remove(struct klist_node *n)
 
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (waiter.woken)
+		/* Pairs with the release store in klist_release() */
+		if (smp_load_acquire(&waiter.woken))
 			break;
 		schedule();
 	}
-- 
2.39.5 (Apple Git-154)

Re: [PATCH] klist: avoid accesses after waking klist_remove()

[Greg Kroah-Hartman]

On Tue, Aug 25, 2026 at 06:04:08AM +0200, Karl Mehltretter wrote:
> klist_release() publishes waiter->woken before its final accesses to the
> stack waiter and node. klist_remove() can then return, allowing the waiter
> to go out of scope and its caller to free or reuse the node.

But does that ever actually happen?

> Clear n_klist and take a task reference before publishing woken. Use
> release/acquire accesses for that publication and wake the referenced task.
> 
> The task reference keeps the waiter task alive if it returns and exits
> before wake_up_process(). wake_up_process() provides the full barrier
> required by the sleep/wakeup protocol, so remove the explicit mb().

I'm confused, what actual bug is here?

> Fixes: 8b0c250be489 ("[PATCH] add klist_node_attached() to determine if a node is on a list or not.")
> Fixes: 210272a28465 ("driver core: Remove completion from struct klist_node")
> Reported-by: syzbot+9cb1ac7fce4944ba9165@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9cb1ac7fce4944ba9165
> Tested-by: syzbot+9cb1ac7fce4944ba9165@syzkaller.appspotmail.com

Are you sure?  the whole bind/unbind mess that syzbot is throwing at us
right now is really causing people to go down odd paths in thinking it's
even a valid thing to consider at all.

Is this a "valid" bind path for this driver and hardware?  If it is,
then maybe we should consider this, but if it isn't, then thinking that
klist is where the bug is isn't correct.

I'm going to be submitting a "taint" flag for when bind/unbind is used,
as this all is a debugging aid that has gone rouge...

thanks,

greg k-h

Re: [PATCH] klist: avoid accesses after waking klist_remove()

[Karl Mehltretter]

On Tue, Aug 25, 2026 at 07:25:48AM +0100, Greg Kroah-Hartman wrote:
> > to go out of scope and its caller to free or reuse the node.
> 
> But does that ever actually happen?

klist_remove() waits until the node is removed so the containing object can
be freed. bus_remove_driver() can later free drv->p, while device_move()
immediately adds the same node to another list.

> > The task reference keeps the waiter task alive if it returns and exits
> > before wake_up_process(). wake_up_process() provides the full barrier
> > required by the sleep/wakeup protocol, so remove the explicit mb().
> 
> I'm confused, what actual bug is here?

I found this during a targeted klist.c review.

Only later I cam across the syzbot report. That report became too prominent
in the commit message.

klist_release() sets waiter->woken, then reads waiter->process and later
writes n->n_klist. After seeing woken, the waiter can return on another CPU.

My initial KASAN reproducer was klist-only. One thread held an iterator while
another removed and freed the object. A delay after the wakeup made
the race easy, hit KASAN on n_klist in qemu arm64. 

> > Closes: https://syzkaller.appspot.com/bug?extid=9cb1ac7fce4944ba9165
> > Tested-by: syzbot+9cb1ac7fce4944ba9165@syzkaller.appspotmail.com
> 
> Are you sure?  the whole bind/unbind mess that syzbot is throwing at us
> right now is really causing people to go down odd paths in thinking it's
> even a valid thing to consider at all.

I did not verify the dummy_udc bind/unbind sequence. The klist issue
does not depend on this USB path.

I can reword a v2 around the generic reproducer. anything else I should
investigate before sending it ?

Thanks,
Karl

Re: [PATCH] klist: avoid accesses after waking klist_remove()

[Greg Kroah-Hartman]

On Tue, Aug 25, 2026 at 09:03:25AM +0200, Karl Mehltretter wrote:
> On Tue, Aug 25, 2026 at 07:25:48AM +0100, Greg Kroah-Hartman wrote:
> > > to go out of scope and its caller to free or reuse the node.
> > 
> > But does that ever actually happen?
> 
> klist_remove() waits until the node is removed so the containing object can
> be freed. bus_remove_driver() can later free drv->p, while device_move()
> immediately adds the same node to another list.
> 
> > > The task reference keeps the waiter task alive if it returns and exits
> > > before wake_up_process(). wake_up_process() provides the full barrier
> > > required by the sleep/wakeup protocol, so remove the explicit mb().
> > 
> > I'm confused, what actual bug is here?
> 
> I found this during a targeted klist.c review.
> 
> Only later I cam across the syzbot report. That report became too prominent
> in the commit message.
> 
> klist_release() sets waiter->woken, then reads waiter->process and later
> writes n->n_klist. After seeing woken, the waiter can return on another CPU.
> 
> My initial KASAN reproducer was klist-only. One thread held an iterator while
> another removed and freed the object. A delay after the wakeup made
> the race easy, hit KASAN on n_klist in qemu arm64. 

Yes, but real-world use causes this in the kernel today?

> 
> > > Closes: https://syzkaller.appspot.com/bug?extid=9cb1ac7fce4944ba9165
> > > Tested-by: syzbot+9cb1ac7fce4944ba9165@syzkaller.appspotmail.com
> > 
> > Are you sure?  the whole bind/unbind mess that syzbot is throwing at us
> > right now is really causing people to go down odd paths in thinking it's
> > even a valid thing to consider at all.
> 
> I did not verify the dummy_udc bind/unbind sequence. The klist issue
> does not depend on this USB path.
> 
> I can reword a v2 around the generic reproducer. anything else I should
> investigate before sending it ?

Again, what user of this api in the kernel is causing this type of
problem to require these changes?

thanks,

greg k-h