linux-kernel - Re: [PATCH v3 2/3] io_uring: avoid ring quiesce while registering/unregistering eventfd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ffa271c7-3f49-2b5a-b67e-3bb1b052ee4e@kernel.dk>
Date:   Thu, 3 Feb 2022 10:56:56 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Usama Arif <usama.arif@...edance.com>, io-uring@...r.kernel.org,
        asml.silence@...il.com, linux-kernel@...r.kernel.org
Cc:     fam.zheng@...edance.com
Subject: Re: [PATCH v3 2/3] io_uring: avoid ring quiesce while
 registering/unregistering eventfd

On 2/3/22 10:41 AM, Usama Arif wrote:
> @@ -1726,13 +1732,24 @@ static inline struct io_uring_cqe *io_get_cqe(struct io_ring_ctx *ctx)
>  	return &rings->cqes[tail & mask];
>  }
>  
> -static inline bool io_should_trigger_evfd(struct io_ring_ctx *ctx)
> +static void io_eventfd_signal(struct io_ring_ctx *ctx)
>  {
> -	if (likely(!ctx->cq_ev_fd))
> -		return false;
> +	struct io_ev_fd *ev_fd;
> +
> +	rcu_read_lock();
> +	/* rcu_dereference ctx->io_ev_fd once and use it for both for checking and eventfd_signal */
> +	ev_fd = rcu_dereference(ctx->io_ev_fd);
> +
> +	if (likely(!ev_fd))
> +		goto out;
>  	if (READ_ONCE(ctx->rings->cq_flags) & IORING_CQ_EVENTFD_DISABLED)
> -		return false;
> -	return !ctx->eventfd_async || io_wq_current_is_worker();
> +		goto out;
> +
> +	if (!ctx->eventfd_async || io_wq_current_is_worker())
> +		eventfd_signal(ev_fd->cq_ev_fd, 1);
> +
> +out:
> +	rcu_read_unlock();
>  }

Like Pavel pointed out, we still need the fast path (of not having an
event fd registered at all) to just do the cheap check and not need rcu
lock/unlock. Outside of that, I think this looks fine.

>  static int io_eventfd_unregister(struct io_ring_ctx *ctx)
>  {
> -	if (ctx->cq_ev_fd) {
> -		eventfd_ctx_put(ctx->cq_ev_fd);
> -		ctx->cq_ev_fd = NULL;
> -		return 0;
> +	struct io_ev_fd *ev_fd;
> +	int ret;
> +
> +	mutex_lock(&ctx->ev_fd_lock);
> +	ev_fd = rcu_dereference_protected(ctx->io_ev_fd, lockdep_is_held(&ctx->ev_fd_lock));
> +	if (!ev_fd) {
> +		ret = -ENXIO;
> +		goto out;
>  	}
> +	synchronize_rcu();
> +	eventfd_ctx_put(ev_fd->cq_ev_fd);
> +	kfree(ev_fd);
> +	rcu_assign_pointer(ctx->io_ev_fd, NULL);
> +	ret = 0;
>  
> -	return -ENXIO;
> +out:
> +	mutex_unlock(&ctx->ev_fd_lock);
> +	return ret;
>  }

synchronize_rcu() can take a long time, and I think this is in the wrong
spot. It should be on the register side, IFF we need to expedite the
completion of a previous event fd unregistration. If we do it that way,
at least it'll only happen if it's necessary. What do you think?

-- 
Jens Axboe