xenbus: Use kref to track req lifetime

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@invisiblethingslab.com>
Fixes: fd8aa9095a95 ("xen: optimize xenbus driver for multiple concurrent xenstore accesses")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
Message-ID: <20250506210935.5607-1-jason.andryuk@amd.com>

diff --git a/drivers/xen/xenbus/xenbus.h b/drivers/xen/xenbus/xenbus.h
index 13821e7e825efb3ff74a1920512e0cfe799f88c2..9ac0427724a30136dcd15f0cd1b4929e66de3d88 100644 (file)
--- a/drivers/xen/xenbus/xenbus.h
+++ b/drivers/xen/xenbus/xenbus.h
@@ -77,6 +77,7 @@ enum xb_req_state {
 struct xb_req_data {
        struct list_head list;
        wait_queue_head_t wq;
+       struct kref kref;
        struct xsd_sockmsg msg;
        uint32_t caller_req_id;
        enum xsd_sockmsg_type type;
@@ -103,6 +104,7 @@ int xb_init_comms(void);
 void xb_deinit_comms(void);
 int xs_watch_msg(struct xs_watch_event *event);
 void xs_request_exit(struct xb_req_data *req);
+void xs_free_req(struct kref *kref);
 
 int xenbus_match(struct device *_dev, const struct device_driver *_drv);
 int xenbus_dev_probe(struct device *_dev);

diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
index e5fda0256feb3d476aeee5f2d1e28ac7bd29f226..82df2da1b880b899f7ac75b13eda7bb23040fcd8 100644 (file)
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -309,8 +309,8 @@ static int process_msg(void)
                        virt_wmb();
                        req->state = xb_req_state_got_reply;
                        req->cb(req);
-               } else
-                       kfree(req);
+               }
+               kref_put(&req->kref, xs_free_req);
        }
 
        mutex_unlock(&xs_response_mutex);
@@ -386,14 +386,13 @@ static int process_writes(void)
        state.req->msg.type = XS_ERROR;
        state.req->err = err;
        list_del(&state.req->list);
-       if (state.req->state == xb_req_state_aborted)
-               kfree(state.req);
-       else {
+       if (state.req->state != xb_req_state_aborted) {
                /* write err, then update state */
                virt_wmb();
                state.req->state = xb_req_state_got_reply;
                wake_up(&state.req->wq);
        }
+       kref_put(&state.req->kref, xs_free_req);
 
        mutex_unlock(&xb_write_mutex);
 

diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
index 46f8916597e53db15eb01cb9cec98c9e0a3056ac..f5c21ba64df571c70f6dc0105867fbd674dfbb18 100644 (file)
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -406,7 +406,7 @@ void xenbus_dev_queue_reply(struct xb_req_data *req)
        mutex_unlock(&u->reply_mutex);
 
        kfree(req->body);
-       kfree(req);
+       kref_put(&req->kref, xs_free_req);
 
        kref_put(&u->kref, xenbus_file_free);
 

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index d32c726f7a12d0f5c8462852ba4322dd5ffe4ac5..dcf9182c8451adb1591c5b9a07c50e897acd0052 100644 (file)
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -112,6 +112,12 @@ static void xs_suspend_exit(void)
        wake_up_all(&xs_state_enter_wq);
 }
 
+void xs_free_req(struct kref *kref)
+{
+       struct xb_req_data *req = container_of(kref, struct xb_req_data, kref);
+       kfree(req);
+}
+
 static uint32_t xs_request_enter(struct xb_req_data *req)
 {
        uint32_t rq_id;
@@ -237,6 +243,12 @@ static void xs_send(struct xb_req_data *req, struct xsd_sockmsg *msg)
        req->caller_req_id = req->msg.req_id;
        req->msg.req_id = xs_request_enter(req);
 
+       /*
+        * Take 2nd ref.  One for this thread, and the second for the
+        * xenbus_thread.
+        */
+       kref_get(&req->kref);
+
        mutex_lock(&xb_write_mutex);
        list_add_tail(&req->list, &xb_write_list);
        notify = list_is_singular(&xb_write_list);
@@ -261,8 +273,8 @@ static void *xs_wait_for_reply(struct xb_req_data *req, struct xsd_sockmsg *msg)
        if (req->state == xb_req_state_queued ||
            req->state == xb_req_state_wait_reply)
                req->state = xb_req_state_aborted;
-       else
-               kfree(req);
+
+       kref_put(&req->kref, xs_free_req);
        mutex_unlock(&xb_write_mutex);
 
        return ret;
@@ -291,6 +303,7 @@ int xenbus_dev_request_and_reply(struct xsd_sockmsg *msg, void *par)
        req->cb = xenbus_dev_queue_reply;
        req->par = par;
        req->user_req = true;
+       kref_init(&req->kref);
 
        xs_send(req, msg);
 
@@ -319,6 +332,7 @@ static void *xs_talkv(struct xenbus_transaction t,
        req->num_vecs = num_vecs;
        req->cb = xs_wake_up;
        req->user_req = false;
+       kref_init(&req->kref);
 
        msg.req_id = 0;
        msg.tx_id = t.id;