From owner-freebsd-arch@FreeBSD.ORG Fri Apr 19 17:43:52 2013 Return-Path: Delivered-To: arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 3C881DFA; Fri, 19 Apr 2013 17:43:52 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id F0F985EA; Fri, 19 Apr 2013 15:57:28 +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 mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 7A1061041552; Fri, 19 Apr 2013 22:53:51 +1000 (EST) Date: Fri, 19 Apr 2013 22:53:50 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Kenneth D. Merry" Subject: Re: patches to add new stat(2) file flags In-Reply-To: <20130418184951.GA18777@nargothrond.kdm.org> Message-ID: <20130419215624.L1262@besplex.bde.org> References: <20130307000533.GA38950@nargothrond.kdm.org> <20130307222553.P981@besplex.bde.org> <20130308232155.GA47062@nargothrond.kdm.org> <20130310181127.D2309@besplex.bde.org> <20130409190838.GA60733@nargothrond.kdm.org> <20130418184951.GA18777@nargothrond.kdm.org> 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=A8I0pNqG c=1 sm=1 a=n2O7wv11oSwA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=YOiZBDKP_E4A:10 a=QuKyM733q63FOVycHlwA:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: arch@FreeBSD.org, fs@FreeBSD.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Apr 2013 17:43:52 -0000 On Thu, 18 Apr 2013, Kenneth D. Merry wrote: > On Tue, Apr 09, 2013 at 13:08:38 -0600, Kenneth D. Merry wrote: >> ... >> Okay, I think these issues should now be fixed. We now refuse to change >> attributes only on the root directory. And I updatd deupdat() to do the >> same. >> >> When a directory is created or a file is added, the archive bit is not >> changed on the directory. Not sure if we need to do that or not. (Simply >> changing msdosfs_mkdir() to set ATTR_ARCHIVE was not enough to get the >> archive bit set on directory creation.) > > Bruce, any comment on this? I didn't get around to looking at it closely. Just had a quick look at the msdosfs parts. Apparently we are already doing the same as WinXP for ATTR_ARCHIVE on directories. Not the right thing, but: - don't set it on directory creation - don't set it on directory modification - allow setting and clearing it (with your changes). @ *** src/lib/libc/sys/chflags.2.orig @ --- src/lib/libc/sys/chflags.2 @ *************** @ *** 112,137 **** @ ... @ --- 112,170 ---- @ ... @ + .It Dv UF_IMMUTABLE @ + The file may not be changed. @ + Filesystems may use this flag to maintain compatibility with the DOS, Windows @ + and CIFS FILE_ATTRIBUTE_READONLY attribute. msdosfs doesn't use this yet. It uses ATTR_READONLY, and doesn't map this to or from UF_IMMUTABLE. I think I want ATTR_READONLY to be a flag and not affect the file permissions (just like immutable flags normally don't affect the file permissions. Does CIFS FILE_ATTRIBUTE_READONLY have exactly the same semantics as IMMUTABLE? That is, does it prevent all operations on the file and the file's metadata except read()? For IMMUTABLE, the other operations that it disallows include setattr(), rename() and unlink(). Well it doesn't in WinXP using Cygwin. I made a directory with attributes +R, and this didn't prevent creating files in the directory or rmdir of the directory. Even attributes +R +H +S didn't prevent these operations. Maybe +R isn't really used for directories, like +A. Then for a file with +R +H +S: - rm asked before deleting it (+R changed its fake permissions from rw-r--r-- to r--r--r--). - touching it succeeded - attrib on it succeeded - writing it failed. So it seems that in WinXP, ATTR_READONLY is ignored for directories, and more like the !writeable permission than the immutable flag. @ *** src/sys/fs/msdosfs/msdosfs_denode.c.orig @ --- src/sys/fs/msdosfs/msdosfs_denode.c @ *************** @ *** 300,307 **** @ if ((dep->de_flag & DE_MODIFIED) == 0) @ return (0); @ dep->de_flag &= ~DE_MODIFIED; @ ! if (dep->de_Attributes & ATTR_DIRECTORY) @ ! return (0); @ if (dep->de_refcnt <= 0) @ return (0); @ error = readde(dep, &bp, &dirp); @ --- 300,309 ---- @ if ((dep->de_flag & DE_MODIFIED) == 0) @ return (0); @ dep->de_flag &= ~DE_MODIFIED; @ ! /* Was: silently ignore attribute changes for all dirs. */ @ ! if (DETOV(dep)->v_vflag & VV_ROOT) @ ! return (EINVAL); @ ! /* Otherwise valid. */ Clean up the comments a bit. Say nothing, or that all attributes apply to all directories except the root directory. Perhaps the VV_ROOT case is unreachable because callers filter out this case. I have a debugger trap for it. @ if (dep->de_refcnt <= 0) @ return (0); @ error = readde(dep, &bp, &dirp); @ *** src/sys/fs/msdosfs/msdosfs_vnops.c.orig @ --- src/sys/fs/msdosfs/msdosfs_vnops.c @ *************** @ *** 398,403 **** @ --- 402,418 ---- @ if (vap->va_flags != VNOVAL) { @ if (vp->v_mount->mnt_flag & MNT_RDONLY) @ return (EROFS); @ + /* @ + * We don't allow setting attributes on the root directory, @ + * because according to Bruce Evans: "The special case for @ + * the root directory is because before FAT32, the root @ + * directory didn't have an entry for itself (and was @ + * otherwise special). With FAT32, the root directory is @ + * not so special, but still doesn't have an entry for itself." @ + */ @ + if (vp->v_vflag & VV_ROOT) @ + return (EINVAL); @ + @ if (cred->cr_uid != pmp->pm_uid) { @ error = priv_check_cred(cred, PRIV_VFS_ADMIN, 0); @ if (error) No need to give the source. I prefer the do this check after the permissions check, but if it is done early then it is best done as a single check for all attributes in msdosfs_settattr() and not just for flags. Currently there is: - no check for ownerships. We only allow null changes to ownerships. With no check like the above, we allow them even for the root directory, while the above disallows null changes to flags for the root directory. - for truncate(), the error is EISDIR for all directories. - for file times, we silently ignore changes for all directories, after doing permissions checks. Only the root directory should be special. - for file permissions, we handle directories as for file times. Now the only possible non-null change is of ATTR_READONLY, and since this apparently has no effect in WinXP, ignorig changing it for directories is best. Bruce