[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aB0Rmd1PCxA_7Gch@mail-itl>
Date: Thu, 8 May 2025 22:18:32 +0200
From: Marek Marczykowski-Górecki <marmarek@...isiblethingslab.com>
To: Jason Andryuk <jason.andryuk@....com>
Cc: Juergen Gross <jgross@...e.com>,
Stefano Stabellini <sstabellini@...nel.org>,
Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
Boris Ostrovsky <boris.ostrovsky@...cle.com>,
stable@...r.kernel.org, xen-devel@...ts.xenproject.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xenbus: Use kref to track req lifetime
On Tue, May 06, 2025 at 05:09:33PM -0400, Jason Andryuk wrote:
> Marek reported seeing a NULL pointer fault in the xenbus_thread
> callstack:
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> RIP: e030:__wake_up_common+0x4c/0x180
> Call Trace:
> <TASK>
> __wake_up_common_lock+0x82/0xd0
> process_msg+0x18e/0x2f0
> xenbus_thread+0x165/0x1c0
>
> process_msg+0x18e is req->cb(req). req->cb is set to xs_wake_up(), a
> thin wrapper around wake_up(), or xenbus_dev_queue_reply(). It seems
> like it was xs_wake_up() in this case.
>
> It seems like req may have woken up the xs_wait_for_reply(), which
> kfree()ed the req. When xenbus_thread resumes, it faults on the zero-ed
> data.
>
> Linux Device Drivers 2nd edition states:
> "Normally, a wake_up call can cause an immediate reschedule to happen,
> meaning that other processes might run before wake_up returns."
> ... which would match the behaviour observed.
>
> Change to keeping two krefs on each request. One for the caller, and
> one for xenbus_thread. Each will kref_put() when finished, and the last
> will free it.
>
> This use of kref matches the description in
> Documentation/core-api/kref.rst
>
> Link: https://lore.kernel.org/xen-devel/ZO0WrR5J0xuwDIxW@mail-itl/
> Reported-by: "Marek Marczykowski-Górecki" <marmarek@...isiblethingslab.com>
> Fixes: fd8aa9095a95 ("xen: optimize xenbus driver for multiple concurrent xenstore accesses")
> Cc: stable@...r.kernel.org
> Signed-off-by: Jason Andryuk <jason.andryuk@....com>
> ---
> Kinda RFC-ish as I don't know if it fixes Marek's issue. This does seem
> like the correct approach if we are seeing req free()ed out from under
> xenbus_thread.
Thanks for the patch! I don't have easy way to test if it definitely
fixes the issues (due to poor reproduction rate), but it looks very
likely. I did run it through our CI and at least there it didn't crash
(but again, it doesn't happen often).
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists