[{"id":1766708,"web_url":"http://patchwork.ozlabs.org/comment/1766708/","msgid":"<20170912064500.GC16554@quack2.suse.cz>","list_archive_url":null,"date":"2017-09-12T06:45:00","subject":"Re: [PATCH v2 3/5] ext4: add sanity check for encryption + DAX","submitter":{"id":363,"url":"http://patchwork.ozlabs.org/api/people/363/","name":"Jan Kara","email":"jack@suse.cz"},"content":"On Mon 11-09-17 23:05:24, Ross Zwisler wrote:\n> We prevent DAX from being used on inodes which are using ext4's built in\n> encryption via a check in ext4_set_inode_flags(). We do have what appears\n> to be an unsafe transition of S_DAX in ext4_set_context(), though, where\n> S_DAX can get disabled without us doing a proper writeback + invalidate.\n> \n> There are also issues with mm-level races when changing the value of S_DAX,\n> as well as issues with the VM_MIXEDMAP flag:\n> \n> https://www.spinics.net/lists/linux-xfs/msg09859.html\n> \n> I actually think we are safe in this case because of the following:\n> \n> 1) You can't encrypt an existing file. Encryption can only be set on an\n> empty directory, with new inodes in that directory being created with\n> encryption turned on, so I don't think it's possible to turn encryption on\n> for a file that has open DAX mmaps or outstanding I/Os.\n> \n> 2) There is no way to turn encryption off on a given file. Once an inode\n> is encrypted, it stays encrypted for the life of that inode, so we don't\n> have to worry about the case where we turn encryption off and S_DAX\n> suddenly turns on.\n> \n> 3) The only way we end up in ext4_set_context() to turn on encryption is\n> when we are creating a new file in the encrypted directory. This happens\n> as part of ext4_create() before the inode has been allowed to do any I/O.\n> Here's the call tree:\n> \n> ext4_create()\n> __ext4_new_inode()\n> \t ext4_set_inode_flags() // sets S_DAX\n> \t fscrypt_inherit_context()\n> \t\tfscrypt_get_encryption_info();\n> \t\text4_set_context() // sets EXT4_INODE_ENCRYPT, clears S_DAX\n> \n> So, I actually think it's safe to transition S_DAX in ext4_set_context()\n> without any locking, writebacks or invalidations. I've added a\n> WARN_ON_ONCE() sanity check to make sure that we are notified if we ever\n> encounter a case where we are encrypting an inode that already has data,\n> in which case we need to add code to safely transition S_DAX.\n> \n> Signed-off-by: Ross Zwisler \n> CC: stable@vger.kernel.org\n\nLooks good to me - and frankly I think we can drop the stable CC here...\nAnyway, you can add:\n\nReviewed-by: Jan Kara \n\n\t\t\t\t\t\t\t\tHonza\n\n> ---\n> fs/ext4/super.c | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/fs/ext4/super.c b/fs/ext4/super.c\n> index 4251e50..c090780 100644\n> --- a/fs/ext4/super.c\n> +++ b/fs/ext4/super.c\n> @@ -1159,6 +1159,9 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,\n> \tif (inode->i_ino == EXT4_ROOT_INO)\n> \t\treturn -EPERM;\n> \n> +\tif (WARN_ON_ONCE(IS_DAX(inode) && i_size_read(inode)))\n> +\t\treturn -EINVAL;\n> +\n> \tres = ext4_convert_inline_data(inode);\n> \tif (res)\n> \t\treturn res;\n> -- \n> 2.9.5\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrwK71m9Wz9s81\n\tfor ;\n\tTue, 12 Sep 2017 16:45:15 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751289AbdILGpD (ORCPT );\n\tTue, 12 Sep 2017 02:45:03 -0400","from mx2.suse.de ([195.135.220.15]:49345 \"EHLO mx1.suse.de\"\n\trhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP\n\tid S1751251AbdILGpC (ORCPT );\n\tTue, 12 Sep 2017 02:45:02 -0400","from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254])\n\tby mx1.suse.de (Postfix) with ESMTP id 8C21BABB0;\n\tTue, 12 Sep 2017 06:45:00 +0000 (UTC)","by quack2.suse.cz (Postfix, from userid 1000)\n\tid 154DB1E3442; Tue, 12 Sep 2017 08:45:00 +0200 (CEST)"],"X-Virus-Scanned":"by amavisd-new at test-mx.suse.de","Date":"Tue, 12 Sep 2017 08:45:00 +0200","From":"Jan Kara ","To":"Ross Zwisler ","Cc":"Theodore Ts'o , Jan Kara ,\n\tlinux-kernel@vger.kernel.org, Andreas Dilger ,\n\tChristoph Hellwig , Dan Williams ,\n\tDave Chinner , linux-ext4@vger.kernel.org,\n\tlinux-nvdimm@lists.01.org, stable@vger.kernel.org","Subject":"Re: [PATCH v2 3/5] ext4: add sanity check for encryption + DAX","Message-ID":"<20170912064500.GC16554@quack2.suse.cz>","References":"<20170912050526.7627-1-ross.zwisler@linux.intel.com>\n\t<20170912050526.7627-4-ross.zwisler@linux.intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170912050526.7627-4-ross.zwisler@linux.intel.com>","User-Agent":"Mutt/1.5.24 (2015-08-30)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1767169,"web_url":"http://patchwork.ozlabs.org/comment/1767169/","msgid":"<20170912153921.GB5000@linux.intel.com>","list_archive_url":null,"date":"2017-09-12T15:39:21","subject":"Re: [PATCH v2 3/5] ext4: add sanity check for encryption + DAX","submitter":{"id":46514,"url":"http://patchwork.ozlabs.org/api/people/46514/","name":"Ross Zwisler","email":"ross.zwisler@linux.intel.com"},"content":"On Tue, Sep 12, 2017 at 08:45:00AM +0200, Jan Kara wrote:\n> On Mon 11-09-17 23:05:24, Ross Zwisler wrote:\n> > We prevent DAX from being used on inodes which are using ext4's built in\n> > encryption via a check in ext4_set_inode_flags(). We do have what appears\n> > to be an unsafe transition of S_DAX in ext4_set_context(), though, where\n> > S_DAX can get disabled without us doing a proper writeback + invalidate.\n> > \n> > There are also issues with mm-level races when changing the value of S_DAX,\n> > as well as issues with the VM_MIXEDMAP flag:\n> > \n> > https://www.spinics.net/lists/linux-xfs/msg09859.html\n> > \n> > I actually think we are safe in this case because of the following:\n> > \n> > 1) You can't encrypt an existing file. Encryption can only be set on an\n> > empty directory, with new inodes in that directory being created with\n> > encryption turned on, so I don't think it's possible to turn encryption on\n> > for a file that has open DAX mmaps or outstanding I/Os.\n> > \n> > 2) There is no way to turn encryption off on a given file. Once an inode\n> > is encrypted, it stays encrypted for the life of that inode, so we don't\n> > have to worry about the case where we turn encryption off and S_DAX\n> > suddenly turns on.\n> > \n> > 3) The only way we end up in ext4_set_context() to turn on encryption is\n> > when we are creating a new file in the encrypted directory. This happens\n> > as part of ext4_create() before the inode has been allowed to do any I/O.\n> > Here's the call tree:\n> > \n> > ext4_create()\n> > __ext4_new_inode()\n> > \t ext4_set_inode_flags() // sets S_DAX\n> > \t fscrypt_inherit_context()\n> > \t\tfscrypt_get_encryption_info();\n> > \t\text4_set_context() // sets EXT4_INODE_ENCRYPT, clears S_DAX\n> > \n> > So, I actually think it's safe to transition S_DAX in ext4_set_context()\n> > without any locking, writebacks or invalidations. I've added a\n> > WARN_ON_ONCE() sanity check to make sure that we are notified if we ever\n> > encounter a case where we are encrypting an inode that already has data,\n> > in which case we need to add code to safely transition S_DAX.\n> > \n> > Signed-off-by: Ross Zwisler \n> > CC: stable@vger.kernel.org\n> \n> Looks good to me - and frankly I think we can drop the stable CC here...\n\nSure, I'm fine to drop the CC to stable.\n\n> Anyway, you can add:\n> \n> Reviewed-by: Jan Kara \n> \n> \t\t\t\t\t\t\t\tHonza\n> \n> > ---\n> > fs/ext4/super.c | 3 +++\n> > 1 file changed, 3 insertions(+)\n> > \n> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c\n> > index 4251e50..c090780 100644\n> > --- a/fs/ext4/super.c\n> > +++ b/fs/ext4/super.c\n> > @@ -1159,6 +1159,9 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,\n> > \tif (inode->i_ino == EXT4_ROOT_INO)\n> > \t\treturn -EPERM;\n> > \n> > +\tif (WARN_ON_ONCE(IS_DAX(inode) && i_size_read(inode)))\n> > +\t\treturn -EINVAL;\n> > +\n> > \tres = ext4_convert_inline_data(inode);\n> > \tif (res)\n> > \t\treturn res;\n> > -- \n> > 2.9.5\n> > \n> -- \n> Jan Kara \n> SUSE Labs, CR","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xs89W2vyPz9ryr\n\tfor ;\n\tWed, 13 Sep 2017 01:39:27 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751514AbdILPj0 (ORCPT );\n\tTue, 12 Sep 2017 11:39:26 -0400","from mga11.intel.com ([192.55.52.93]:33831 \"EHLO mga11.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751404AbdILPjZ (ORCPT );\n\tTue, 12 Sep 2017 11:39:25 -0400","from orsmga002.jf.intel.com ([10.7.209.21])\n\tby fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t12 Sep 2017 08:39:24 -0700","from theros.lm.intel.com (HELO linux.intel.com) ([10.232.112.77])\n\tby orsmga002.jf.intel.com with ESMTP; 12 Sep 2017 08:39:22 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.42,383,1500966000\"; d=\"scan'208\";a=\"134510755\"","Date":"Tue, 12 Sep 2017 09:39:21 -0600","From":"Ross Zwisler ","To":"Jan Kara ","Cc":"Ross Zwisler ,\n\tTheodore Ts'o , linux-kernel@vger.kernel.org,\n\tAndreas Dilger ,\n\tChristoph Hellwig , Dan Williams ,\n\tDave Chinner , linux-ext4@vger.kernel.org,\n\tlinux-nvdimm@lists.01.org, stable@vger.kernel.org","Subject":"Re: [PATCH v2 3/5] ext4: add sanity check for encryption + DAX","Message-ID":"<20170912153921.GB5000@linux.intel.com>","References":"<20170912050526.7627-1-ross.zwisler@linux.intel.com>\n\t<20170912050526.7627-4-ross.zwisler@linux.intel.com>\n\t<20170912064500.GC16554@quack2.suse.cz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170912064500.GC16554@quack2.suse.cz>","User-Agent":"Mutt/1.8.3 (2017-05-23)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}}]