From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754278AbcJ3Gc1 (ORCPT ); Sun, 30 Oct 2016 02:32:27 -0400 Received: from verein.lst.de ([213.95.11.211]:40868 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752830AbcJ3GcZ (ORCPT ); Sun, 30 Oct 2016 02:32:25 -0400 Date: Sun, 30 Oct 2016 07:32:22 +0100 From: Christoph Hellwig To: Al Viro Cc: Christoph Hellwig , torvalds@linux-foundation.org, jack@suse.cz, dmonakhov@openvz.org, jmoyer@redhat.com, linux-fsdevel@vger.kernel.org, linux-aio@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH] aio: fix a user triggered use after free (and fix freeze protection of aio writes) Message-ID: <20161030063222.GB1683@lst.de> References: <1477727070-18806-1-git-send-email-hch@lst.de> <1477727070-18806-2-git-send-email-hch@lst.de> <20161029122451.GQ19539@ZenIV.linux.org.uk> <20161029152017.GA7388@lst.de> <20161029161230.GR19539@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161029161230.GR19539@ZenIV.linux.org.uk> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Oct 29, 2016 at 05:12:30PM +0100, Al Viro wrote: > Eww... IOW, as soon as we'd submitted an async iocb, nobody can so much as > look at struct file *or* iocb, right? Or underlying inode, or any fs-private > data structures attached to it... Yeah. > I certainly agree that it's a bug, but IMO you are just papering over it. > Just look at e.g. > written = mapping->a_ops->direct_IO(iocb, &data); > > /* > * Finally, try again to invalidate clean pages which might have been > * cached by non-direct readahead, or faulted in by get_user_pages() > * if the source of the write was an mmap'ed region of the file > * we're writing. Either one is a pretty crazy thing to do, > * so we don't support it 100%. If this invalidation > * fails, tough, the write still worked... > */ > if (mapping->nrpages) { > invalidate_inode_pages2_range(mapping, > pos >> PAGE_SHIFT, end); > } > in generic_file_direct_write() - who said that mapping doesn't point into > freed memory by that point? True, somewhat unlike due to inode caching, but for sure possible and something that needs to be deal with. > Why do we play that kind of insane games, anyway? Why not e.g. refcount > the (async) iocb and have kiocb_free() drop the reference, with io_submit_one() > holding one across the call of aio_run_iocb()? Cacheline bouncing issues? > Anything more subtle? There shouldn't really be a need to refcount the iocb itself - nothing worth looking at. The one we consider was struct file, and I didn't like it because of the cacheline bouncing if we decrement if from both the submitter and completion context. But it starts to sounds like the sane version more and more.