linux-kernel - Re: [PATCH 1/2] 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 3 Feb 2022 08:55:04 -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 1/2] io_uring: avoid ring quiesce while
 registering/unregistering eventfd

On 2/3/22 8:11 AM, Usama Arif wrote:
> +static void io_eventfd_signal(struct io_ring_ctx *ctx)
> +{
> +	struct io_ev_fd *ev_fd;
> +
> +	rcu_read_lock();
> +	ev_fd = rcu_dereference(ctx->io_ev_fd);
> +
> +	if (!io_should_trigger_evfd(ctx, ev_fd))
> +		goto out;
> +
> +	eventfd_signal(ev_fd->cq_ev_fd, 1);
> +out:
> +	rcu_read_unlock();
> +}

Would be cleaner as:

static void io_eventfd_signal(struct io_ring_ctx *ctx)
{
	struct io_ev_fd *ev_fd;

	rcu_read_lock();
	ev_fd = rcu_dereference(ctx->io_ev_fd);

	if (io_should_trigger_evfd(ctx, ev_fd))
		eventfd_signal(ev_fd->cq_ev_fd, 1);

	rcu_read_unlock();
}

and might be worth considering pulling in the io_should_trigger_evfd()
code rather than have it be a separate helper now with just the one
caller.

> @@ -9353,35 +9374,67 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx,
>  
>  static int io_eventfd_register(struct io_ring_ctx *ctx, void __user *arg)
>  {
> +	struct io_ev_fd *ev_fd;
>  	__s32 __user *fds = arg;
> -	int fd;
> +	int fd, ret;
>  
> -	if (ctx->cq_ev_fd)
> -		return -EBUSY;
> +	mutex_lock(&ctx->ev_fd_lock);
> +	ret = -EBUSY;
> +	if (rcu_dereference_protected(ctx->io_ev_fd, lockdep_is_held(&ctx->ev_fd_lock)))
> +		goto out;
>  
> +	ret = -EFAULT;
>  	if (copy_from_user(&fd, fds, sizeof(*fds)))
> -		return -EFAULT;
> +		goto out;
>  
> -	ctx->cq_ev_fd = eventfd_ctx_fdget(fd);
> -	if (IS_ERR(ctx->cq_ev_fd)) {
> -		int ret = PTR_ERR(ctx->cq_ev_fd);
> +	ret = -ENOMEM;
> +	ev_fd = kmalloc(sizeof(*ev_fd), GFP_KERNEL);
> +	if (!ev_fd)
> +		goto out;
>  
> -		ctx->cq_ev_fd = NULL;
> -		return ret;
> +	ev_fd->cq_ev_fd = eventfd_ctx_fdget(fd);
> +	if (IS_ERR(ev_fd->cq_ev_fd)) {
> +		ret = PTR_ERR(ev_fd->cq_ev_fd);
> +		kfree(ev_fd);
> +		goto out;
>  	}
> +	ev_fd->ctx = ctx;
>  
> -	return 0;
> +	rcu_assign_pointer(ctx->io_ev_fd, ev_fd);
> +	ret = 0;
> +
> +out:
> +	mutex_unlock(&ctx->ev_fd_lock);
> +	return ret;
> +}

One thing that both mine and your version suffers from is if someone
does an eventfd unregister, and then immediately does an eventfd
register. If the rcu grace period hasn't passed, we'll get -EBUSY on
trying to do that, when I think the right behavior there would be to
wait for the grace period to pass.

I do think we need to handle that gracefully, spurious -EBUSY is
impossible for an application to deal with.

> @@ -11171,8 +11226,10 @@ SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode,
>  	mutex_lock(&ctx->uring_lock);
>  	ret = __io_uring_register(ctx, opcode, arg, nr_args);
>  	mutex_unlock(&ctx->uring_lock);
> +	rcu_read_lock();
>  	trace_io_uring_register(ctx, opcode, ctx->nr_user_files, ctx->nr_user_bufs,
> -							ctx->cq_ev_fd != NULL, ret);
> +				rcu_dereference(ctx->io_ev_fd) != NULL, ret);
> +	rcu_read_unlock();
>  out_fput:
>  	fdput(f);
>  	return ret;

We should probably just modify that tracepoint, kill that ev_fd argument
(it makes very little sense).

-- 
Jens Axboe