From owner-freebsd-fs@FreeBSD.ORG Mon Jun 2 14:56:04 2008 Return-Path: Delivered-To: freebsd-fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0B9E31065671 for ; Mon, 2 Jun 2008 14:56:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx10.syd.optusnet.com.au (fallbackmx10.syd.optusnet.com.au [211.29.132.251]) by mx1.freebsd.org (Postfix) with ESMTP id 92BD68FC26 for ; Mon, 2 Jun 2008 14:56:03 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by fallbackmx10.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m527opq6008585 for ; Mon, 2 Jun 2008 17:50:56 +1000 Received: from c220-239-252-11.carlnfd3.nsw.optusnet.com.au (c220-239-252-11.carlnfd3.nsw.optusnet.com.au [220.239.252.11]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m527ojOk011104 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 2 Jun 2008 17:50:47 +1000 Date: Mon, 2 Jun 2008 17:50:45 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Ighighi In-Reply-To: <48438687.1080606@gmail.com> Message-ID: <20080602163212.G2551@delplex.bde.org> References: <48438687.1080606@gmail.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-fs@FreeBSD.org, bug-followup@FreeBSD.org Subject: Re: kern/122047: [ext2fs] incorrect handling of UF_IMMUTABLE / UF_APPEND, flag on EXT2FS (maybe others) X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 02 Jun 2008 14:56:04 -0000 On Mon, 2 Jun 2008, Ighighi wrote: > On Linux, only the root user may set/clear the immutable/append flags > on ext2 filesystems... Shouldn't FreeBSD do this too, as a POLA? I think it should do just that... > Anyway the attached patch extends the previous one by making it possible > to follow the current Linux convention by setting the sysctl to 0. > Setting it to 1, allows normal users to set them as well, and setting it > to -1 preserves current (though erroneous) FreeBSD behavior. ... but nothing more. Why have complications provide more control over Linux file systems than Linux does? The current behaviour seems to be just a bug from not understanding that Linux has no user immutable/append flags. % --- src/sys/gnu/fs/ext2fs/ext2_inode_cnv.c.orig 2005-06-14 22:36:10.000000000 -0400 % +++ src/sys/gnu/fs/ext2fs/ext2_inode_cnv.c 2008-06-02 00:35:34.658524358 -0430 % @@ -30,11 +30,19 @@ % #include % #include % #include % +#include % +#include % % #include % #include % #include % % +SYSCTL_DECL(_vfs_e2fs); % + % +static int userflags = -1; % +SYSCTL_INT(_vfs_e2fs, OID_AUTO, userflags, CTLFLAG_RW, % + &userflags, 0, "Users may set/clear filesystem flags"); % + % void % ext2_print_inode( in ) % struct inode *in; The existence of vfs sysctls is another bug. They should be mount options, or perhaps system-wide, or not exist at all. ext2fs has only one vfs sysctl now: - vfs.e2fs.dircheck. This sysctl is less broken than in ffs, where the corresponding sysctl is spelled debug.dircheck and a comment still says that the corresponding static kernel variable `dirchk' is changed by "patching". The kernel variable is spelled differently to the sysctl to confuse me when grepping for dircheck. - the debug.doasyncfree sysctl is in dead code (under the non-option FANCY_REALLOC. Block reallocation is also dead in ext2fs). This sysctl is less broken in ffs. There it is named vfs.ffs.doasyncfree. OTOH, perhaps these sysctls really did belong under debug or vfs.debug. It is not very useful to restrict them to just ffs or ext2fs when have many mounted file systems. This bug should not be extended. % @@ -83,8 +91,37 @@ ext2_ei2i(ei, ip) % ip->i_mtime = ei->i_mtime; % ip->i_ctime = ei->i_ctime; % ip->i_flags = 0; % - ip->i_flags |= (ei->i_flags & EXT2_APPEND_FL) ? APPEND : 0; % - ip->i_flags |= (ei->i_flags & EXT2_IMMUTABLE_FL) ? IMMUTABLE : 0; I think it would work to just map EXT2*_FL to SF_*. % + switch (userflags) { % + case 0: % + /* % + * Only the superuser may set/clear these flags. % + * This is the current behavior on Linux. % + */ % + if (ei->i_flags & EXT2_APPEND_FL) % + ip->i_flags |= SF_APPEND; % + if (ei->i_flags & EXT2_IMMUTABLE_FL) % + ip->i_flags |= SF_IMMUTABLE; % + break; % + case 1: % + /* % + * Users may set/clear these flags on files they own. % + */ % + if (ei->i_flags & EXT2_APPEND_FL) % + ip->i_flags |= UF_APPEND; % + if (ei->i_flags & EXT2_IMMUTABLE_FL) % + ip->i_flags |= UF_IMMUTABLE; % + break; For administration it can be convenient for the file system to behave a little differently to native mode, but I letting root change the (system) immutable flags is enough. % + case -1: % + default: % + /* % + * Default behavior on FreeBSD % + */ % + if (ei->i_flags & EXT2_APPEND_FL) % + ip->i_flags |= APPEND; % + if (ei->i_flags & EXT2_IMMUTABLE_FL) % + ip->i_flags |= IMMUTABLE; % + break; % + } I think the current behaviour is too buggy to keep. (Your original PR describes the bugs -- FreeBSD makes a mess by setting 2 flags in the in-core inode and allowing these flags to be changed independently, then cannot merge the flags properly when writing to the on-disk inode.) [conversion to the on-disk inode] Similarly. There is a problem in ext2_vnops.c that is not touched by these patches. Even in the simple version that only support SF_*, ext2_setattr() needs changes to disallow setting of UF_* -- otherwise FreeBSD still makes a mess, with stat() showing changes to UF_* succeeding while the inode is in-core but going away when the inode is flushed from the inode/vnode cache. The fix is simply to remove the code that supports UF_*. (We could also globably replace IMMUTABLE and APPEND by SF_IMMUTABLE and SF_APPEND. This would be clearer but would increase divergence from ffs.) The fix to support all 3 sysctl modes is not so simple: - case 0: dynamically disallow attempts to set UF_*. - case 1: dynamically disallow attempts to set SF_*. - case -1: dynamically convert attempts to set SF_* or UF*_ into attempts to set both, and somehow relax the checks for setting SF_* so that this has a chance of succeeding for non-root. I don't want these complications. Note that the corresponding code in ffs is poorly organized and buggy (it doesn't allow preservation of flags in the right way). ext2_setattr() was once almost identical to ufs_settatr() but now has the following bitrot: - missing support for setting atimes on exec. - different comments about privilege though the code is the same. - different comments about truncate() on r/o file systems. Missing a critical fix for truncate(). - missing the comment expansion and cleanup for utimes(). I think there was a minor security-related fix in there too, but this is now null. % --- src/sys/gnu/fs/ext2fs/ext2_vnops.c.orig 2006-02-19 20:53:14.000000000 -0400 % +++ src/sys/gnu/fs/ext2fs/ext2_vnops.c 2008-05-28 07:58:02.189157441 -0430 % @@ -358,6 +358,8 @@ ext2_getattr(ap) % vap->va_mtime.tv_nsec = ip->i_mtimensec; % vap->va_ctime.tv_sec = ip->i_ctime; % vap->va_ctime.tv_nsec = ip->i_ctimensec; % + vap->va_birthtime.tv_sec = 0; % + vap->va_birthtime.tv_nsec = 0; % vap->va_flags = ip->i_flags; % vap->va_gen = ip->i_gen; % vap->va_blocksize = vp->v_mount->mnt_stat.f_iosize; This is unrelated and should be handled centrally. Almost all file systems get this wrong. Most fail to set va_birthtime, so stat() returns kernel stack garbage for st_birthtime. ffs1 does the same as the above. msdosfs does the above correctly, by setting tv_sec to (time_t)-1 in unsupported cases. Bruce