Linus, what do you think about it? It makes things simpler in
sys_?truncate(), sys_?chown() and their nfsd counterparts. Penalty for
notify_change() is minimal. If you have objections against this variant
I can roll one that will only affect fs/nfsd/vfs.c, but IMHO this code
duplication is unnecessary and evil.
Cheers,
Al
Patch (against 2.1.129-4) follows:
--- fs/attr.c.old Mon Nov 16 07:03:55 1998
+++ fs/attr.c Mon Nov 16 08:45:29 1998
@@ -78,18 +78,77 @@
mark_inode_dirty(inode);
}
+/*
+ * Treatment of chown (POSIX is braindead. As usual...)
+ * a) If we change UID we MUST drop SUID. OK.
+ * b) If we change GID we MUST drop SGID. Also fine.
+ * c) POSIX says that we MUST drop S{U|G}ID even if new {U|G}ID
+ * is equal to the old one. Hmm... OK, that may be reasonable.
+ * d) POSIX doesn't specify the effect of chown when {U|G}ID is -1.
+ * e) Standard convention being that {U|G}ID == -1 means "don't change
+ * {U|G}ID".
+ * And here the ambiguity begins: should chown(foo,bar,-1) drop SGID?
+ * If we consider it as equivalent of chown(foo,bar,current_GID_of_foo) it
+ * should. But we consider passing -1 _not_ as syntax sugar we'ld better
+ * keep S{U|G}ID as is - after all, we have been explicitly asked not to
+ * mess with {U|G}ID at all.
+ * Different OSes have different behaviour here. Moreover, in Linux
+ * we have _two_ different variants - one for normal chown() and other for
+ * chown request passed to knfsd. chown() considers -1's as syntax sugar,
+ * knfsd doesn't. Worse yet, knfsd doesn't drop S{U|G}ID even if we had
+ * explicitly asked to set {U|G}ID to their current values.
+ * IMHO behaviour should be at least consistent. Proposal: let's add
+ * sysctlable parameter controlling whether we drop S?ID when ?ID is -1
+ * or not. Right now all chown/s?id functionality is moved into the function
+ * below. It reproduces old behaviour unless CHOWN_BRAINDAMAGE_FIX is defined.
+ */
+static void chown_sxid(struct inode * inode, struct iattr * attr)
+{
+ unsigned int ia_valid = attr->ia_valid;
+
+ if (!(ia_valid & ATTR_MODE))
+ attr->ia_mode = inode->i_mode;
+#ifndef CHOWN_BRAINDAMAGE_FIX
+ if (!(ia_valid & ATTR_UID))
+ attr->ia_uid = inode->i_uid;
+ if (!(ia_valid & ATTR_GID))
+ attr->ia_gid = inode->i_gid;
+ if (attr->ia_mode & S_ISUID) {
+ attr->ia_mode &= ~S_ISUID;
+ attr->ia_valid |= ATTR_MODE;
+ }
+ if ((attr->ia_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
+ attr->ia_mode &= ~S_ISGID;
+ attr->ia_valid |= ATTR_MODE;
+ }
+#else
+ if ((ia_valid & ATTR_UID) && (attr->ia_mode & S_ISUID)) {
+ attr->ia_mode &= ~S_ISUID;
+ attr->ia_valid |= ATTR_MODE;
+ }
+ if ((ia_valid & ATTR_GID) &&
+ ((attr->ia_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
+ attr->ia_mode &= ~S_IGUID;
+ attr->ia_valid |= ATTR_MODE;
+ }
+#endif
+}
+
int notify_change(struct dentry * dentry, struct iattr * attr)
{
struct inode *inode = dentry->d_inode;
int error;
time_t now = CURRENT_TIME;
unsigned int ia_valid = attr->ia_valid;
+ unsigned long size = attr->ia_size;
attr->ia_ctime = now;
if (!(ia_valid & ATTR_ATIME_SET))
attr->ia_atime = now;
if (!(ia_valid & ATTR_MTIME_SET))
attr->ia_mtime = now;
+ if (ia_valid & (ATTR_UID|ATTR_GID))
+ chown_sxid(inode, attr);
if (inode->i_sb && inode->i_sb->s_op &&
inode->i_sb->s_op->notify_change)
@@ -98,6 +157,11 @@
error = inode_change_ok(inode, attr);
if (!error)
inode_setattr(inode, attr);
+ }
+ if (!error && (ia_valid & ATTR_SIZE)) {
+ vmtruncate(inode, size);
+ if (inode->i_op && inode->i_op->truncate)
+ inode->i_op->truncate(inode);
}
return error;
}
--- fs/open.c.old Mon Nov 16 07:04:02 1998
+++ fs/open.c Mon Nov 16 08:16:11 1998
@@ -77,12 +77,6 @@
newattrs.ia_size = length;
newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
error = notify_change(dentry, &newattrs);
- if (!error) {
- /* truncate virtual mappings of this file */
- vmtruncate(inode, length);
- if (inode->i_op && inode->i_op->truncate)
- inode->i_op->truncate(inode);
- }
up(&inode->i_sem);
return error;
}
@@ -517,30 +511,14 @@
error = -EPERM;
if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
goto out;
- if (user == (uid_t) -1)
- user = inode->i_uid;
- if (group == (gid_t) -1)
- group = inode->i_gid;
- newattrs.ia_mode = inode->i_mode;
- newattrs.ia_uid = user;
- newattrs.ia_gid = group;
- newattrs.ia_valid = ATTR_UID | ATTR_GID | ATTR_CTIME;
- /*
- * If the owner has been changed, remove the setuid bit
- */
- if (inode->i_mode & S_ISUID) {
- newattrs.ia_mode &= ~S_ISUID;
- newattrs.ia_valid |= ATTR_MODE;
+ newattrs.ia_valid = ATTR_CTIME;
+ if (user != (uid_t) -1) {
+ newattrs.ia_uid = user;
+ newattrs.ia_valid |= ATTR_UID;
}
- /*
- * If the group has been changed, remove the setgid bit
- *
- * Don't remove the setgid bit if no group execute bit.
- * This is a file marked for mandatory locking.
- */
- if (((inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))) {
- newattrs.ia_mode &= ~S_ISGID;
- newattrs.ia_valid |= ATTR_MODE;
+ if (group != (gid_t) -1) {
+ newattrs.ia_gid = group;
+ newattrs.ia_valid |= ATTR_GID;
}
error = DQUOT_TRANSFER(dentry, &newattrs);
out:
--- fs/nfsd/vfs.c.old Mon Nov 16 07:04:37 1998
+++ fs/nfsd/vfs.c Mon Nov 16 07:13:02 1998
@@ -232,6 +232,12 @@
dentry = fhp->fh_dentry;
inode = dentry->d_inode;
+ imode = inode->i_mode;
+ if (iap->ia_valid & ATTR_MODE) {
+ iap->ia_mode &= S_IALLUGO;
+ imode = iap->ia_mode |= (imode & ~S_IALLUGO);
+ }
+
/* The size case is special... */
if (iap->ia_valid & ATTR_SIZE) {
if (!S_ISREG(inode->i_mode))
@@ -241,43 +247,22 @@
if (err != 0)
goto out;
}
- err = get_write_access(inode);
- if (err)
- goto out_nfserr;
- /* N.B. Should we update the inode cache here? */
- inode->i_size = iap->ia_size;
- if (inode->i_op && inode->i_op->truncate)
- inode->i_op->truncate(inode);
- mark_inode_dirty(inode);
- put_write_access(inode);
- iap->ia_valid &= ~ATTR_SIZE;
iap->ia_valid |= ATTR_MTIME;
- iap->ia_mtime = CURRENT_TIME;
- }
-
- imode = inode->i_mode;
- if (iap->ia_valid & ATTR_MODE) {
- iap->ia_mode &= S_IALLUGO;
- imode = iap->ia_mode |= (imode & ~S_IALLUGO);
- }
-
- /* Revoke setuid/setgid bit on chown/chgrp */
- if ((iap->ia_valid & ATTR_UID) && (imode & S_ISUID)
- && iap->ia_uid != inode->i_uid) {
- iap->ia_valid |= ATTR_MODE;
- iap->ia_mode = imode &= ~S_ISUID;
- }
- if ((iap->ia_valid & ATTR_GID) && (imode & S_ISGID)
- && iap->ia_gid != inode->i_gid) {
- iap->ia_valid |= ATTR_MODE;
- iap->ia_mode = imode &= ~S_ISGID;
}
/* Change the attributes. */
if (iap->ia_valid) {
iap->ia_valid |= ATTR_CTIME;
- iap->ia_ctime = CURRENT_TIME;
- err = notify_change(dentry, iap);
+ if (iap->ia_valid & ATTR_SIZE) {
+ err = get_write_access(inode);
+ if (err)
+ goto out_nfserr;
+ fh_lock(fhp);
+ err = notify_change(dentry, iap);
+ fh_unlock(fhp);
+ put_write_access(inode);
+ } else
+ err = notify_change(dentry, iap);
if (err)
goto out_nfserr;
if (EX_ISSYNC(fhp->fh_export))
@@ -737,11 +722,6 @@
newattrs.ia_size = size;
newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
err = notify_change(dentry, &newattrs);
- if (!err) {
- vmtruncate(inode, size);
- if (inode->i_op && inode->i_op->truncate)
- inode->i_op->truncate(inode);
- }
put_write_access(inode);
DQUOT_DROP(inode);
fh_unlock(fhp);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/