Commit 09098e62 authored by Bernd Schubert's avatar Bernd Schubert Committed by Miklos Szeredi
Browse files

fuse: {io-uring} Fix a possible req cancellation race 



task-A (application) might be in request_wait_answer and
try to remove the request when it has FR_PENDING set.

task-B (a fuse-server io-uring task) might handle this
request with FUSE_IO_URING_CMD_COMMIT_AND_FETCH, when
fetching the next request and accessed the req from
the pending list in fuse_uring_ent_assign_req().
That code path was not protected by fiq->lock and so
might race with task-A.

For scaling reasons we better don't use fiq->lock, but
add a handler to remove canceled requests from the queue.

This also removes usage of fiq->lock from
fuse_uring_add_req_to_ring_ent() altogether, as it was
there just to protect against this race and incomplete.

Also added is a comment why FR_PENDING is not cleared.

Fixes: c090c8ab ("fuse: Add io-uring sqe commit and fetch support")
Cc: <stable@vger.kernel.org> # v6.14
Reported-by: default avatarJoanne Koong <joannelkoong@gmail.com>
Closes: https://lore.kernel.org/all/CAJnrk1ZgHNb78dz-yfNTpxmW7wtT88A=m-zF0ZoLXKLUHRjNTw@mail.gmail.com/


Signed-off-by: default avatarBernd Schubert <bschubert@ddn.com>
Reviewed-by: default avatarJoanne Koong <joannelkoong@gmail.com>
Signed-off-by: default avatarMiklos Szeredi <mszeredi@redhat.com>
parent 4701f33a

fs/fuse/dev.c

+25 −9
Original line number Diff line number Diff line
@@ -407,6 +407,24 @@ static int queue_interrupt(struct fuse_req *req) 
	return 0;

}



bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock)

{

	spin_lock(lock);

	if (test_bit(FR_PENDING, &req->flags)) {

		/*

		 * FR_PENDING does not get cleared as the request will end

		 * up in destruction anyway.

		 */

		list_del(&req->list);

		spin_unlock(lock);

		__fuse_put_request(req);

		req->out.h.error = -EINTR;

		return true;

	}

	spin_unlock(lock);

	return false;

}



static void request_wait_answer(struct fuse_req *req)

{

	struct fuse_conn *fc = req->fm->fc;
@@ -428,23 +446,21 @@ static void request_wait_answer(struct fuse_req *req) 
	}



	if (!test_bit(FR_FORCE, &req->flags)) {

		bool removed;



		/* Only fatal signals may interrupt this */

		err = wait_event_killable(req->waitq,

					test_bit(FR_FINISHED, &req->flags));

		if (!err)

			return;



		spin_lock(&fiq->lock);

		/* Request is not yet in userspace, bail out */

		if (test_bit(FR_PENDING, &req->flags)) {

			list_del(&req->list);

			spin_unlock(&fiq->lock);

			__fuse_put_request(req);

			req->out.h.error = -EINTR;

		if (test_bit(FR_URING, &req->flags))

			removed = fuse_uring_remove_pending_req(req);

		else

			removed = fuse_remove_pending_req(req, &fiq->lock);

		if (removed)

			return;

	}

		spin_unlock(&fiq->lock);

	}



	/*

	 * Either request is already in userspace, or it was forced.

fs/fuse/dev_uring.c

+11 −4
Original line number Diff line number Diff line
@@ -726,8 +726,6 @@ static void fuse_uring_add_req_to_ring_ent(struct fuse_ring_ent *ent, 
					   struct fuse_req *req)

{

	struct fuse_ring_queue *queue = ent->queue;

	struct fuse_conn *fc = req->fm->fc;

	struct fuse_iqueue *fiq = &fc->iq;



	lockdep_assert_held(&queue->lock);
@@ -737,9 +735,7 @@ static void fuse_uring_add_req_to_ring_ent(struct fuse_ring_ent *ent, 
			ent->state);

	}



	spin_lock(&fiq->lock);

	clear_bit(FR_PENDING, &req->flags);

	spin_unlock(&fiq->lock);

	ent->fuse_req = req;

	ent->state = FRRS_FUSE_REQ;

	list_move(&ent->list, &queue->ent_w_req_queue);
@@ -1238,6 +1234,8 @@ void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req) 
	if (unlikely(queue->stopped))

		goto err_unlock;



	set_bit(FR_URING, &req->flags);

	req->ring_queue = queue;

	ent = list_first_entry_or_null(&queue->ent_avail_queue,

				       struct fuse_ring_ent, list);

	if (ent)
@@ -1276,6 +1274,8 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req) 
		return false;

	}



	set_bit(FR_URING, &req->flags);

	req->ring_queue = queue;

	list_add_tail(&req->list, &queue->fuse_req_bg_queue);



	ent = list_first_entry_or_null(&queue->ent_avail_queue,
@@ -1306,6 +1306,13 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req) 
	return true;

}



bool fuse_uring_remove_pending_req(struct fuse_req *req)

{

	struct fuse_ring_queue *queue = req->ring_queue;



	return fuse_remove_pending_req(req, &queue->lock);

}



static const struct fuse_iqueue_ops fuse_io_uring_ops = {

	/* should be send over io-uring as enhancement */

	.send_forget = fuse_dev_queue_forget,

fs/fuse/dev_uring_i.h

+6 −0
Original line number Diff line number Diff line
@@ -142,6 +142,7 @@ void fuse_uring_abort_end_requests(struct fuse_ring *ring); 
int fuse_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags);

void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req);

bool fuse_uring_queue_bq_req(struct fuse_req *req);

bool fuse_uring_remove_pending_req(struct fuse_req *req);



static inline void fuse_uring_abort(struct fuse_conn *fc)

{
@@ -200,6 +201,11 @@ static inline bool fuse_uring_ready(struct fuse_conn *fc) 
	return false;

}



static inline bool fuse_uring_remove_pending_req(struct fuse_req *req)

{

	return false;

}



#endif /* CONFIG_FUSE_IO_URING */



#endif /* _FS_FUSE_DEV_URING_I_H */

fs/fuse/fuse_dev_i.h

+1 −0
Original line number Diff line number Diff line
@@ -61,6 +61,7 @@ int fuse_copy_out_args(struct fuse_copy_state *cs, struct fuse_args *args, 
void fuse_dev_queue_forget(struct fuse_iqueue *fiq,

			   struct fuse_forget_link *forget);

void fuse_dev_queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req);

bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock);



#endif

fs/fuse/fuse_i.h

+3 −0
Original line number Diff line number Diff line
@@ -378,6 +378,7 @@ struct fuse_io_priv { 
 * FR_FINISHED:		request is finished

 * FR_PRIVATE:		request is on private list

 * FR_ASYNC:		request is asynchronous

 * FR_URING:		request is handled through fuse-io-uring

 */

enum fuse_req_flag {

	FR_ISREPLY,
@@ -392,6 +393,7 @@ enum fuse_req_flag { 
	FR_FINISHED,

	FR_PRIVATE,

	FR_ASYNC,

	FR_URING,

};



/**
@@ -441,6 +443,7 @@ struct fuse_req { 


#ifdef CONFIG_FUSE_IO_URING

	void *ring_entry;

	void *ring_queue;

#endif

};