Received: by 2002:a25:868d:0:0:0:0:0 with SMTP id z13csp176543ybk; Tue, 19 May 2020 19:03:23 -0700 (PDT) X-Google-Smtp-Source: ABdhPJx1RJXiA56mkRazAaENJfZi1EaRAw7P2V/VA77e1tedWwMPfO9mKBBb1uSJB8NN/JW85e1U X-Received: by 2002:a17:906:6a92:: with SMTP id p18mr1851333ejr.233.1589940203841; Tue, 19 May 2020 19:03:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1589940203; cv=none; d=google.com; s=arc-20160816; b=Uli2J2kpXuU4JheJcJsPSVYYskHsJyokIIN3ejGQL9BJryTRXNF5CSGjkZGrJJLhqW MV4Kt2wY3BsEDbMF8dbNOkCj6r5x+b38dYaKe0TSbzRr+nKXfrQ+4q16HB+XCi44LDca Rme97yI291NRa7Y97TpulrtsCoZ5TT517g6YX3/OGi3TmFwC78cgl/mGG49y0/iI/O14 dhSjoamv4Egwxz5a58H9CoM6iRbVR0Qn1LjpfGh2WI7PAPHaIuMXGvieMXea8IsVqm5r aSly6jGv2kFTcwwB3QKxZjYANsFVM0HtWCamySmzFRwqsdeZByMmhGLXiakMlwoyLWxv uREQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:ironport-sdr:ironport-sdr; bh=BtGe2qwnV4xHei3I+GiZWl0WI0mSxKIpykH3NEsME+s=; b=ZgUzfhmPP9a0u0uonTDu4b3hiR/G0y10y65SAUOXvRunaQUoJFoqwVFEluXaZIunJb PnNLqQrG2zWNV5eyC/SlRfkhvTceP4zMGGHITqraehyllr42lR+QwzCRvzixwWbwRbXb xlJJEEHuQXwwlRm23unDxa+MhvnlRbEZQP7QRdgVC3wZBmvshxASXtd803hRLsZm9DAt ywgYhhhbMLCqdCZI8WeLjbN8VEoLQVzXjjkWMGrw3kMK5mgECbc8E9181KAXOWCtY40p bMfyEEA9NFvWkyi+s4sL7E1b8F3oEwKuZYgn8ZpAwUtZrpfC78P5dm3mVVOjpIo1D258 +tig== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-ext4-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id f8si655068edy.149.2020.05.19.19.02.51; Tue, 19 May 2020 19:03:23 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-ext4-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728305AbgETCCe (ORCPT + 99 others); Tue, 19 May 2020 22:02:34 -0400 Received: from mga02.intel.com ([134.134.136.20]:11305 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726379AbgETCCe (ORCPT ); Tue, 19 May 2020 22:02:34 -0400 IronPort-SDR: KXF6qd43oAx8/UnDSaK4asVuHJ+/bSNwS+SENonGqW++3WS/7MnzVhNci1siOL6aCqWtPKxamD w17MzyKZycIQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 May 2020 19:02:34 -0700 IronPort-SDR: m/fhxx6jPbq7qZ0zgedcE0xpHxfN3zP7A9oZnB302QnVzCHxMgMIzOf8NbcKm47m7FnUyO55hO +uQHaEVufyJw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,412,1583222400"; d="scan'208";a="282519935" Received: from iweiny-desk2.sc.intel.com ([10.3.52.147]) by orsmga002.jf.intel.com with ESMTP; 19 May 2020 19:02:33 -0700 Date: Tue, 19 May 2020 19:02:33 -0700 From: Ira Weiny To: Eric Biggers Cc: linux-ext4@vger.kernel.org, Andreas Dilger , "Theodore Y. Ts'o" , Jan Kara , Al Viro , Dan Williams , Dave Chinner , Christoph Hellwig , Jeff Moyer , "Darrick J. Wong" , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX Message-ID: <20200520020232.GA3470571@iweiny-DESK2.sc.intel.com> References: <20200513054324.2138483-1-ira.weiny@intel.com> <20200513054324.2138483-4-ira.weiny@intel.com> <20200516020253.GG1009@sol.localdomain> <20200518050315.GA3025231@iweiny-DESK2.sc.intel.com> <20200518162447.GA954@sol.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200518162447.GA954@sol.localdomain> User-Agent: Mutt/1.11.1 (2018-12-01) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Mon, May 18, 2020 at 09:24:47AM -0700, Eric Biggers wrote: > On Sun, May 17, 2020 at 10:03:15PM -0700, Ira Weiny wrote: First off... OMG... I'm seeing some possible user pitfalls which are complicating things IMO. It probably does not matter because most users don't care and have either enabled DAX on _every_ mount or _not_ enabled DAX on _every_ mount. And have _not_ used verity nor encryption while using DAX. Verity is a bit easier because verity is not inherited and we only need to protect against setting it if DAX is on. However, it can be weird for the user thusly: 1) mount _without_ DAX 2) enable verity on individual inodes 3) unmount/mount _with_ DAX Now the verity files are not enabled for DAX without any indication... This is still true with my patch. But at least it closes the hole of trying to change the DAX flag after the fact (because verity was set). Also both this check and the verity need to be maintained to keep the mount option working as it was before... For encryption it is more complicated because encryption can be set on directories and inherited so the IS_DAX() check does nothing while '-o dax' is used. Therefore users can: 1) mount _with_ DAX 2) enable encryption on a directory 3) files created in that directory will not have DAX set And I now understand why the WARN_ON() was there... To tell users about this craziness. ... > > This is, AFAICS, not going to affect correctness. It will only be confusing > > because the user will be able to set both DAX and encryption on the directory > > but files there will only see encryption being used... :-( > > > > Assuming you are correct about this call path only being valid on directories. > > It seems this IS_DAX() needs to be changed to check for EXT4_DAX_FL in > > "fs/ext4: Introduce DAX inode flag"? Then at that point we can prevent DAX and > > encryption on a directory. ... and at this point IS_DAX() could be removed at > > this point in the series??? > > I haven't read the whole series, but if you are indeed trying to prevent a > directory with EXT4_DAX_FL from being encrypted, then it does look like you'd > need to check EXT4_DAX_FL, not S_DAX. > > The other question is what should happen when a file is created in an encrypted > directory when the filesystem is mounted with -o dax. Actually, I think I > missed something there. Currently (based on reading the code) the DAX flag will > get set first, and then ext4_set_context() will see IS_DAX() && i_size == 0 and > clear the DAX flag when setting the encrypt flag. I think you are correct. > > So, the i_size == 0 check is actually needed. > Your patch (AFAICS) just makes creating an encrypted file fail > when '-o dax'. Is that intended? Yes that is what I intended but it is more complicated I see now. The intent is that IS_DAX() should _never_ be true on an encrypted or verity file... even if -o dax is specified. Because IS_DAX() should be a result of the inode flags being checked. The order of the setting of those flags is a bit odd for the encrypted case. I don't really like that DAX is set then un-set. It is convoluted but I'm not clear right now how to fix it. > If not, maybe you should change it to check > S_NEW instead of i_size == 0 to make it clearer? The patch is completely unnecessary. It is much easier to make (EXT4_ENCRYPT_FL | EXT4_VERITY_FL) incompatible with EXT4_DAX_FL when it is introduced later in the series. Furthermore this mutual exclusion can be done on directories in the encrypt case. Which I think will be nicer for the user if they get an error when trying to set one when the other is set. Ira