Commit 1f0304df authored by Jason Andryuk's avatar Jason Andryuk Committed by Juergen Gross
Browse files

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: default avatarMarek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Fixes: fd8aa909 ("xen: optimize xenbus driver for multiple concurrent xenstore accesses")
Cc: stable@vger.kernel.org
Signed-off-by: default avatarJason Andryuk <jason.andryuk@amd.com>
Reviewed-by: default avatarJuergen Gross <jgross@suse.com>
Signed-off-by: default avatarJuergen Gross <jgross@suse.com>
Message-ID: <20250506210935.5607-1-jason.andryuk@amd.com>
parent 90989869

drivers/xen/xenbus/xenbus.h

+2 −0
Original line number Diff line number Diff line
@@ -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);

drivers/xen/xenbus/xenbus_comms.c

+4 −5
Original line number Diff line number Diff line
@@ -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);

drivers/xen/xenbus/xenbus_dev_frontend.c

+1 −1
Original line number Diff line number Diff line
@@ -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);

drivers/xen/xenbus/xenbus_xs.c

+16 −2
Original line number Diff line number Diff line
@@ -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;