From owner-freebsd-fs@FreeBSD.ORG Fri Mar 15 09:47:48 2013 Return-Path: Delivered-To: fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 2849E385; Fri, 15 Mar 2013 09:47:48 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail28.syd.optusnet.com.au (mail28.syd.optusnet.com.au [211.29.133.169]) by mx1.freebsd.org (Postfix) with ESMTP id A3007DCA; Fri, 15 Mar 2013 09:47:46 +0000 (UTC) Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail28.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r2F9lYsg010340 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 15 Mar 2013 20:47:38 +1100 Date: Fri, 15 Mar 2013 20:47:34 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pawel Jakub Dawidek Subject: Re: patches to add new stat(2) file flags In-Reply-To: <20130314232449.GC1446@garage.freebsd.pl> Message-ID: <20130315184014.A902@besplex.bde.org> References: <20130307000533.GA38950@nargothrond.kdm.org> <20130307214649.X981@besplex.bde.org> <20130314232449.GC1446@garage.freebsd.pl> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=JMpjKL2b c=1 sm=1 a=n2O7wv11oSwA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=YOiZBDKP_E4A:10 a=LVUDrmMsRTOz-s-3SHEA:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: arch@FreeBSD.org, "Kenneth D. Merry" , fs@FreeBSD.org X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Mar 2013 09:47:48 -0000 On Fri, 15 Mar 2013, Pawel Jakub Dawidek wrote: > On Thu, Mar 07, 2013 at 10:21:38PM +1100, Bruce Evans wrote: >> On Wed, 6 Mar 2013, Kenneth D. Merry wrote: >> >>> I have attached diffs against head for some additional stat(2) file flags. >>> >>> The primary purpose of these flags is to improve compatibility with CIFS, >>> both from the client and the server side. >>> ... >>> UF_IMMUTABLE: Command line name: "uchg", "uimmutable" >>> ZFS name: XAT_READONLY, ZFS_READONLY >>> Windows: FILE_ATTRIBUTE_READONLY >>> >>> This flag means that the file may not be modified. >>> This is not a new flag, but where applicable it is >>> mapped to the Windows readonly bit. ZFS and UFS >>> now both support the flag and enforce it. >>> >>> The behavior of this flag is compatible with MacOS X. >> >> This is incompatible with mapping the DOS read-only attribute to the >> non-writeable file permission in msdosfs. msdosfs does this mainly to >> get at least one useful file permission, but the semantics are subtly >> different from all of file permissions, UF_IMMUTABLE and SF_IMMUTABLE. >> I think it should be a new flag. > > I agree, especially that I saw some discussion recently on Illumos > mailing lists to not enforce this flag in ZFS, which would be confusing > to FreeBSD users if we forget to _not_ merge that change. However, I now think the READONLY attribute would map well to UF_IMMUTABLE in msdosfs, better than the current mapping of the READONLY attribute to the inverse of the write permissions bits. The permissions bits are also controlled by the permissions bits of the mount point, and this is the least worst way to control them for general files. When this is mixed with control by the READONLY attribute (which involves back-control of the READONLY attribute according to the permissions bits), the behaviour is confusing and might lead to the READONLY bit being set for too many files (e.g., for copies of man pages, since man pages are installed with the bogus permissions r--r--r-- although the owner (root) can write them (the r--r--r-- permissions only made sense when the owner was bin)). If the READONLY attribute is instead mapped only to UF_IMMUTABLE, its impact would be smaller since there aren't so many files which have a native READONLY attribute or a native UF_IMMUTABLE attribute. The READONLY attribute would interact badly with the permissions bits in a different way -- just like UF_IMMUTABLE interacts with them. It is confusing when ls -l shows writability for non-writable files. Further testing of possible confusion from UF_IMMUTABLE on a rw-r--r-- uchg file on ffs showed that: - eaccess(2) with flag W_OK used to work correctly, although this was not documented. It used to return the documented errno EACCES, but its man page didn't say anything about immutable attributes and said that this error means that the permissions bits indicate no access (or search permission is denied). - eaccess(2) with flag W_OK now returns the undocumented errno EPERM. Its man page doesn't seem to have changed significantly. Documentation for ACLs also seems to be missing. The old and new man pages point to more details in intro(2). The fine details are missing there too. There is just the usual weaselish "appropriate privilege" used in a generic way for EPERM. This can mean anything, but what it means is not documented in either man page. Actually, eaccess() used to work correctly because I fixed it locally. It seems to have always been broken in FreeBSD. The current version is: @ /* @ * If immutable bit set, nobody gets to write it. "& ~VADMIN_PERMS" @ * is here, because without it, * it would be impossible for the owner @ * to remove the IMMUTABLE flag. @ */ @ if ((accmode & (VMODIFY_PERMS & ~VADMIN_PERMS)) && @ (ip->i_flags & (IMMUTABLE | SF_SNAPSHOT))) @ return (EPERM); Bugs to be fixed here: - the first sentence in the comment is banal and doesn't even echo the code (the code actually handles several immutable bits (obfuscated by the IMMUTABLE macro), and also the snapshot bit) - the second sentence in the comment has a misplaced comment delimiter '*' in the middle of it. It also doesn't fully echo the code, but is not banal. - the "write" in the first sentence also doesn't even echo the code. It used to echo the code when the code was simpler. The code used to check only (accmode & VWRITE). But immutability prevents much more than writing, and the code now handles that. - wrong errno. ext2fs still uses the old ffs code here (except it doesn't use IMMUTABLE and checks explicitly for the only immutable flag that it supports). It duplicates the SF_SNAPSHOT check, but that is nonsense because ext2fs doesn't support snapshots. nandfs copies ffs for setattr, so it has immutabilty flags checks there, but it just uses vaccess() for access(), so it it is missing the above, so the immutable flags checks are either nonsense where they are made or missing here. tmpfs uses the old ffs code here (except for mangling the style, but it does remove the banal comment). I couldn't see exactly what zfs does here, but it mostly returns EPERM for immutable flags checks. Fixing foofs_access() hopefully also fixes open(2), unlink(2), ... Unfortunately, my fix is incompatible with dubious fixes that make the man pages bug for bug compatible with the code. POSIX of course doesn't document EPERM for open(2) (except in the general weasel section about appropriate privilege). FreeBSD didn't document it either in the version in which the above was fixed. But now FreeBSD documents in open.2 and other man pages that immutability gives EPERM, and the code always had this bug. The changes in the man pages have some style bugs: in open.2: - a comma splice in the reference to chflags(2) - this reference is only made in 1 of the descriptions of EPERM. These style bugs were cloned to most or all man pages that are affected by immutability or nounlink flags. ACLs still seem to be unmentioned in all these man pages. I don't use them, so I don't know what happens for them. However, the core vfs function vaccess() is careful to always return EACCES and EPERM as explicitly specified by POSIX. This means EACCES for all cases except VADMIN. VADMIN/EPERM apply to chmod(), chown(), ... but shouldn't apply to open(), unlink(), rename(), ... Bruce