From patchwork Sat Oct 29 17:47:58 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 9403995 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 847E060588 for ; Sat, 29 Oct 2016 17:48:12 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7A5E529139 for ; Sat, 29 Oct 2016 17:48:12 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6EB5D2913D; Sat, 29 Oct 2016 17:48:12 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RCVD_IN_SORBS_SPAM,T_DKIM_INVALID,T_TVD_MIME_EPI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9F8CB2913A for ; Sat, 29 Oct 2016 17:48:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752480AbcJ2RsC (ORCPT ); Sat, 29 Oct 2016 13:48:02 -0400 Received: from mail-oi0-f42.google.com ([209.85.218.42]:32978 "EHLO mail-oi0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751292AbcJ2RsA (ORCPT ); Sat, 29 Oct 2016 13:48:00 -0400 Received: by mail-oi0-f42.google.com with SMTP id y2so163608723oie.0; Sat, 29 Oct 2016 10:48:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=pPODMio7y/m14B/fO/w2y7yGYTjcbohYqa3jqvfJWtA=; b=aYF2dpNvRATGf8FYWeGoe6ecxgWG/cchQuJ2J8LFIBWc6N5MHUDKXJirer8djbAvpO GWMliJ1k4Nye/AXLbSm6HUViYWZ7ZKAQoOAlPfTfATncHpNkzovNStabISARomxWIJKS dR1CoHZ7y7OQYqHeZWkLxYT/tsUB3qI/KOVn4PcoNWB7rvyQ+PNe6QnsDAaPrz8ldd1v bAYveNcK1zFA9LhKJp3RF/iYFe0A37EJad6w6ERPxTFFzVT1uCMdyyZnpbK4pqp98Ddz vnk6Y9MqeQ+ff+6yWBopERSTVtDarRQlnoN52vax0SCkCyzQtcZ4SWtqo7VjMFgGDaHH UAIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=pPODMio7y/m14B/fO/w2y7yGYTjcbohYqa3jqvfJWtA=; b=SvZzKvrWspVnCJGYOXJJiGzHDNjZreYAUkU9ITSXeP7jd6TsOgrN+8oJUQ2Q+g3ZaT 1h5hTQ6JcXul9+QlJSCHlyUcHVEujYiNW5wjtD5VUOrqjHUCuODgYB1GkiRxWALLpxmX GxVWg7XnVi4uZ60D3yYhWpWq7V0D4TwgbZ3tYZiVlVG19xCJw/pT0c9jj562jA9NiHfc WvjSUslcj3ZEOqSW7yZ7AnBFM1dsYbB+syWabqAqTtbTXOOVaKXQ5YXIGvVlpOwx07QF LUzUcmSSLPiVOwxkct5h4BRwZqTe5+TE/hWplhi3AgjMmP4nbDBwRYxvKOqku+pI2gXI hhbQ== X-Gm-Message-State: ABUngvf14MW+aQkb/dRQIUnZO+p6G3R5SJWUCRYXJ1eksg2tcJnha83XuJkug6a24z5Z6xRcWb0hnaugs+tVKA== X-Received: by 10.202.241.213 with SMTP id p204mr19743239oih.116.1477763279655; Sat, 29 Oct 2016 10:47:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.142.104 with HTTP; Sat, 29 Oct 2016 10:47:58 -0700 (PDT) In-Reply-To: <20161029152017.GA7388@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> From: Linus Torvalds Date: Sat, 29 Oct 2016 10:47:58 -0700 X-Google-Sender-Auth: xHNEmxssCgeHMqso_TiSaecnUvY Message-ID: Subject: Re: [PATCH] aio: fix a user triggered use after free (and fix freeze protection of aio writes) To: Christoph Hellwig Cc: Al Viro , Jan Kara , Dmitry Monakhov , Jeff Moyer , linux-fsdevel , linux-aio@kvack.org, Linux Kernel Mailing List , stable Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Sat, Oct 29, 2016 at 8:20 AM, Christoph Hellwig wrote: > > We can't as that would not fix the use after free (at least for the lockdep > case - otherwise the call is a no-op). Once iter_op returns aio_complete > might have dropped our reference to the file, and another thread might > have closed the fd so that the fput from aio_complete was the last one. I don't concpetually mind the patch per se, but the repeated if (rw == WRITE) { .. } if (rw == WRITE) { .. } is just insane and makes the code less legible than it should be. Also, honestly, make it use a helper: "aio_file_start_write()" and "aio_file_end_write()" that has the comments and the lockdep games. Because that patch is just too effing ugly. Does something like the attached work for you guys? Linus fs/aio.c | 33 +++++++++++++++++++++++++++++---- include/linux/fs.h | 1 + 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/fs/aio.c b/fs/aio.c index 1157e13a36d6..3f66331ef90c 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -1066,6 +1066,27 @@ static struct kioctx *lookup_ioctx(unsigned long ctx_id) return ret; } +/* + * We do file_start_write/file_end_write() to make sure + * we have filesystem freeze protection over the whole + * AIO write sequence, but to make sure that lockdep does + * not complain about the held lock when we return to + * userspace, we tell it that we release and reaquire the + * lock. + */ +static void aio_file_start_write(struct file *file) +{ + file_start_write(file); + __sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE); +} + +static void aio_file_end_write(struct file *file) +{ + __sb_writers_acquired(file_inode(file)->i_sb, SB_FREEZE_WRITE); + file_end_write(file); +} + + /* aio_complete * Called when the io request on the given iocb is complete. */ @@ -1078,6 +1099,9 @@ static void aio_complete(struct kiocb *kiocb, long res, long res2) unsigned tail, pos, head; unsigned long flags; + if (kiocb->ki_flags & IOCB_WRITE) + aio_file_end_write(kiocb->ki_filp); + /* * Special case handling for sync iocbs: * - events go directly into the iocb for fast handling @@ -1460,13 +1484,14 @@ static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode, return ret; } - if (rw == WRITE) - file_start_write(file); + if (rw == WRITE) { + /* aio_complete() will end the write */ + req->ki_flags |= IOCB_WRITE; + aio_file_start_write(file); + } ret = iter_op(req, &iter); - if (rw == WRITE) - file_end_write(file); kfree(iovec); break; diff --git a/include/linux/fs.h b/include/linux/fs.h index 16d2b6e874d6..db600e9bb1b4 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -321,6 +321,7 @@ struct writeback_control; #define IOCB_HIPRI (1 << 3) #define IOCB_DSYNC (1 << 4) #define IOCB_SYNC (1 << 5) +#define IOCB_WRITE (1 << 6) struct kiocb { struct file *ki_filp;