On Sun, Mar 3, 2019 at 7:18 AM Al Viro wrote: > > > Maybe unrelated to this bug, but... What's to prevent a wakeup > > that happens just after we'd been added to a waitqueue by ->poll() > > triggering aio_poll_wake(), which gets to aio_poll_complete() > > with its fput() *before* we'd reached the end of ->poll() instance? I'm assuming you're talking about the second vfs_poll() in aio_poll_complete_work()? The one we call before we check for "rew->cancelled" properly under the spinlock? > 1) io_submit(2) allocates aio_kiocb instance and passes it to aio_poll() > 2) aio_poll() resolves the descriptor to struct file by > req->file = fget(iocb->aio_fildes) [...] So honestly, the whole filp handling in aio looks overly complicated to me. All the different operations do that silly fget/fput() dance, although aio_read/write at least tried to make a common helper function for handling it. But as far as I can tell, they *all* could do: - look up file in aio_submit() when allocating and creating the aio_kiocb - drop the filp in 'iocb_put()' (which happens whether things complete successfully or not). and we'd have avoided a lot of complexity, and we'd have avoided this bug. Your patch fixes the poll() case, but it does so by just letting the existing complexity remain, and adding a second fget/fput pair in the poll logic. It would seem like it would be much better to rip all the complexity out entirely, and replace it with sane, simple and obviously correct code. Hmm? In other words, why wouldn't something like the attached work instead? TOTALLY UNTESTED! It builds, and it looks sane, but maybe I'm overlooking some obvious problem with it. But doesn't it look nice to see 2 files changed, 41 insertions(+), 50 deletions(-) with actual code reduction, and a fundamental simplification in handling of the file pointer? Linus