From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2130.oracle.com ([156.151.31.86]:56476 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932759AbeGCQq5 (ORCPT ); Tue, 3 Jul 2018 12:46:57 -0400 Date: Tue, 3 Jul 2018 09:46:49 -0700 From: "Darrick J. Wong" To: Al Viro Cc: Trond Myklebust , "linux-ext4@vger.kernel.org" , "linux-fsdevel@vger.kernel.org" Subject: Re: [RFC] open_by_handle() vs. EA inodes Message-ID: <20180703164649.GA5353@magnolia> References: <20180629181900.GP30522@ZenIV.linux.org.uk> <701debe87eff185064f3cc31f9a9b425cdf8fe61.camel@hammerspace.com> <20180629195732.GQ30522@ZenIV.linux.org.uk> <20180629201548.GR30522@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180629201548.GR30522@ZenIV.linux.org.uk> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri, Jun 29, 2018 at 09:15:48PM +0100, Al Viro wrote: > On Fri, Jun 29, 2018 at 08:57:32PM +0100, Al Viro wrote: > > On Fri, Jun 29, 2018 at 07:38:30PM +0000, Trond Myklebust wrote: > > > On Fri, 2018-06-29 at 19:19 +0100, Al Viro wrote: > > > > On ea_inode-enabled ext4 open_by_handle() (as well as knfsd, > > > > etc.) > > > > can get to EA inodes as long as it knows their inumbers - just pass > > > > it > > > > an fhandle with zeroed version bytes and the right inumber in it. > > > > > > > > AFAICS, it's Not Nice(tm), especially since you can write to > > > > those, > > > > whether they are shared or not. > > > > > > > > Should we make ext4_nfs_get_inode() check for EXT4_EA_INODE_FL > > > > and fail if it's set? > > > > > > handle_to_path() requires CAP_DAC_READ_SEARCH capabilities. Isn't that > > > sufficiently restrictive for open_by_handle()? > > > > > > Concerning knfsd, people can in theory enable subtree checking to > > > enforce checking whether or not you are in an exported subtree. In > > > practice that breaks rename, so people are strongly encouraged to > > > disable subtree checking, and only to export complete filesystems. > > > > Umm... Do we ever want those accessed via fhandles, capabilities or > > no capabilities? IOW, is there any reason for ext4 ->fh_to_dentry() > > to give access to such inodes? Those are implementation internals, > > same as e.g. journal inode... > > Note, BTW, that journal inode will be rejected by > if (ino < EXT4_FIRST_INO(sb) && ino != EXT4_ROOT_INO) > return ERR_PTR(-EFSCORRUPTED); > in ext4_iget_normal(); EA inodes won't be, since their numbers > are in the normal range. And looking at the commit that has > introduced ext4_iget_normal(), I'd say that EA inodes fit the > description in > commit f4bb2981024fc91b23b4d09a8817c415396dbabb > Author: Theodore Ts'o > Date: Sun Oct 5 22:56:00 2014 -0400 > > ext4: add ext4_iget_normal() which is to be used for dir tree lookups > > If there is a corrupted file system which has directory entries that > point at reserved, metadata inodes, prohibit them from being used by > treating them the same way we treat Boot Loader inodes --- that is, > mark them to be bad inodes. This prohibits them from being opened, > deleted, or modified via chmod, chown, utimes, etc. > > In particular, this prevents a corrupted file system which has a > directory entry which points at the journal inode from being deleted > and its blocks released, after which point Much Hilarity Ensues. > > anyway. IOW, it might be better to have that check done not in > ext4_nfs_get_inode() but in ext4_iget_normal(), as in > struct inode *ext4_iget_normal(struct super_block *sb, unsigned long ino) > { > struct inode *res; > if (ino < EXT4_FIRST_INO(sb) && ino != EXT4_ROOT_INO) > return ERR_PTR(-EFSCORRUPTED); > res = ext4_iget(sb, ino); > if (IS_ERR(res) || likely(!(EXT4_I(res)->i_flags & EXT4_EA_INODE_FL))) > return res; > iput(res); > return ERR_PTR(-EFSCORRUPTED); > } > > Objections? None here. :) Based on my reading of how ea inodes work, these special inodes should never be directly reachable by userspace, ever. Only the ext4 xattr code should touch them. --D