linux-kernel - Re: [PATCH 07/30] aio: add delayed cancel support

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:   Wed, 28 Mar 2018 22:34:59 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Christoph Hellwig <hch@....de>
Cc:     Avi Kivity <avi@...lladb.com>, linux-aio@...ck.org,
        linux-fsdevel@...r.kernel.org, netdev@...r.kernel.org,
        linux-api@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/30] aio: add delayed cancel support

On Wed, Mar 28, 2018 at 05:35:26PM +0100, Al Viro wrote:
> On Wed, Mar 28, 2018 at 09:29:03AM +0200, Christoph Hellwig wrote:
> >  static void aio_fsync_work(struct work_struct *work)
> >  {
> >  	struct fsync_iocb *req = container_of(work, struct fsync_iocb, work);
> > +	struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, fsync);
> > +	struct file *file = req->file;
> >  	int ret;
> >  
> >  	ret = vfs_fsync(req->file, req->datasync);
> > -	fput(req->file);
> > -	aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
> > +	if (aio_complete(iocb, ret, 0, 0))
> > +		fput(file);
> 
> IDGI.
> 	1) can aio_complete() ever return false here?
> 	2) do we ever have aio_kiocb that would not have an associated
> struct file * that needs to be dropped on successful aio_complete()?  AFAICS,
> rw, fsync and poll variants all have one, and I'm not sure what kind of
> async IO *could* be done without an opened file.

OK, hell with that.  I've tried to play with turning kiocb into a struct with
anon union in it, with poll and fsync parts folded into that sucker and ki_filp
lifted into common part.  Possible, but it's hairy as hell and can be done
afterwards.

However, doing that digging has turned up something really nasty.  Look:
in io_cancel(2) you have 
        spin_lock_irq(&ctx->ctx_lock);
        kiocb = lookup_kiocb(ctx, iocb, key);   
        if (kiocb) {
                if (kiocb->flags & AIO_IOCB_DELAYED_CANCEL) {
                        kiocb->flags |= AIO_IOCB_CANCELLED;
                } else {
                        ret = kiocb_cancel(kiocb);
                        kiocb = NULL;
                }
        }
        spin_unlock_irq(&ctx->ctx_lock);

Now, suppose two threads call io_cancel() on the same aio_poll in progress.
Both hit that code and *both* find the same kiocb.  Sure, the first one
will shortly do
        if (kiocb)
                ret = kiocb_cancel(kiocb);
which will remove it from the list.  Too late, though - you've already dropped
->ctx_lock, letting the second one find it.  Result: two aio_poll_cancel() in
parallel, with resulting double-free and double-fput().

You really need to remove it from the ->active_reqs before dropping the lock.
free_ioctx_users() does it correctly, io_cancel(2) fucks it up.

I'd add something like
struct aio_kiocb *kiocb_cancel_locked(struct aio_kiocb *kiocb)
{
	if (!kiocb)
		return ERR_PTR(-EINVAL);
	if (kiocb->flags & AIO_IOCB_DELAYED_CANCEL) {
		list_del(&kiocb->ki_list);
		kiocb->flags |= AIO_IOCB_CANCELLED;
		return kiocb;
	} else {
		return ERR_PTR(kiocb_cancel(kiocb));
	}
}

with 
        spin_lock_irq(&ctx->ctx_lock);
        while (!list_empty(&ctx->active_reqs)) {
                req = list_first_entry(&ctx->active_reqs,
                                       struct aio_kiocb, ki_list);
		req = kiocb_cancel_locked(req);
		if (!IS_ERR_OR_NULL(req))
                        list_add_tail(&req->ki_list, &list);
        }
        spin_unlock_irq(&ctx->ctx_lock);

in free_ioctx_users() and
        spin_lock_irq(&ctx->ctx_lock);
        kiocb = kiocb_cancel_locked(lookup_kiocb(ctx, iocb, key));
        spin_unlock_irq(&ctx->ctx_lock);
	ret = IS_ERR_OR_NULL(kiocb) ? PTR_ERR(kiocb) : kiocb_cancel(kiocb);
in io_cancel(2)...