[PATCH] [CIFS] set to fix ephemeral modes and implement "dynperm" mount option

Hi Steve,

Here's a respun set of the ephemeral modes patches that allows for the
legacy behavior conditional on the "dynperm" option. The main
difference between this set and the one sent yesterday is that this set
makes it so that we do our best to clear and set ATTR_READONLY on
appropriate mode changes when the dynperm option is specified.

I've given it some cursory testing this morning against win2k3. Let me
know what you think.

Thanks,
--
Jeff Layton <jlayton@...>

[0001-CIFS-on-non-posix-shares-clear-write-bits-in-mode.patch]

---

>From 94dff9392585dda79d158a4ec1984a772de290bc Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@...>
Date: Tue, 13 May 2008 06:47:19 -0700
Subject: [PATCH 1/4] [CIFS] on non-posix shares, clear write bits in mode when ATTR_READONLY is set

When mounting a share with posix extensions disabled, cifs_get_inode_info
turns off all the write bits in the mode for regular files if ATTR_READONLY
is set. Directories and other inode types, however, can also have
ATTR_READONLY set, but the mode gives no indication of this.

This patch makes this apply to other inode types besides regular files.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/inode.c   |   81 ++++++++++++++++++++++++----------------------------
 fs/cifs/readdir.c |   71 +++++++++++++++++++++++++---------------------
 2 files changed, 76 insertions(+), 76 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index fcbdbb6..aa8a10c 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -391,6 +391,7 @@ int cifs_get_inode_info(struct inode **pinode,
  char *buf = NULL;
  bool adjustTZ = false;
  bool is_dfs_referral = false;
+ umode_t default_mode;
 
  pTcon = cifs_sb->tcon;
  cFYI(1, ("Getting info on %s", search_path));
@@ -512,52 +513,44 @@ try_again_CIFSSMBQPathInfo:
  inode->i_mtime.tv_sec += pTcon->ses->server->timeAdj;
  }
 
- /* set default mode. will override for dirs below */
- if (atomic_read(&cifsInfo->inUse) == 0)
- /* new inode, can safely set these fields */
- inode->i_mode = cifs_sb->mnt_file_mode;
- else /* since we set the inode type below we need to mask off
-     to avoid strange results if type changes and both
-     get orred in */
+ /* get default inode mode */
+ if (attr & ATTR_DIRECTORY)
+ default_mode = cifs_sb->mnt_dir_mode;
+ else
+ default_mode = cifs_sb->mnt_file_mode;
+
+ /* set permission bits */
+ if (atomic_read(&cifsInfo->inUse) == 0 ||
+    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DYNPERM) == 0)
+ inode->i_mode = default_mode;
+ else {
+ /* just reenable write bits if !ATTR_READONLY */
+ if ((inode->i_mode & S_IWUGO) == 0 &&
+    (attr & ATTR_READONLY) == 0)
+ inode->i_mode |= (S_IWUGO & default_mode);
+
  inode->i_mode &= ~S_IFMT;
-/* if (attr & ATTR_REPARSE)  */
- /* We no longer handle these as symlinks because we could not
-   follow them due to the absolute path with drive letter */
- if (attr & ATTR_DIRECTORY) {
- /* override default perms since we do not do byte range locking
-   on dirs */
- inode->i_mode = cifs_sb->mnt_dir_mode;
- inode->i_mode |= S_IFDIR;
- } else if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) &&
-   (cifsInfo->cifsAttrs & ATTR_SYSTEM) &&
-   /* No need to le64 convert size of zero */
-   (pfindData->EndOfFile == 0)) {
- inode->i_mode = cifs_sb->mnt_file_mode;
- inode->i_mode |= S_IFIFO;
-/* BB Finish for SFU style symlinks and devices */
- } else if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) &&
-   (cifsInfo->cifsAttrs & ATTR_SYSTEM)) {
- if (decode_sfu_inode(inode,
- le64_to_cpu(pfindData->EndOfFile),
- search_path,
- cifs_sb, xid))
- cFYI(1, ("Unrecognized sfu inode type"));
-
- cFYI(1, ("sfu mode 0%o", inode->i_mode));
+ }
+
+ /* clear write bits if ATTR_READONLY is set */
+ if (attr & ATTR_READONLY)
+ inode->i_mode &= ~S_IWUGO;
+
+ /* set inode type */
+ if ((attr & ATTR_SYSTEM) &&
+    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL)) {
+ /* no need to fix endianness on 0 */
+ if (pfindData->EndOfFile == 0)
+ inode->i_mode |= S_IFIFO;
+ else if (decode_sfu_inode(inode,
+ le64_to_cpu(pfindData->EndOfFile),
+ search_path, cifs_sb, xid))
+ cFYI(1, ("unknown SFU file type\n"));
  } else {
- inode->i_mode |= S_IFREG;
- /* treat the dos attribute of read-only as read-only
-   mode e.g. 555 */
- if (cifsInfo->cifsAttrs & ATTR_READONLY)
- inode->i_mode &= ~(S_IWUGO);
- else if ((inode->i_mode & S_IWUGO) == 0)
- /* the ATTR_READONLY flag may have been */
- /* changed on server -- set any w bits */
- /* allowed by mnt_file_mode */
- inode->i_mode |= (S_IWUGO &
-  cifs_sb->mnt_file_mode);
- /* BB add code here -
-   validate if device or weird share or device type? */
+ if (attr & ATTR_DIRECTORY)
+ inode->i_mode |= S_IFDIR;
+ else
+ inode->i_mode |= S_IFREG;
  }
 
  spin_lock(&inode->i_lock);
diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 34ec321..3ec32d9 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -132,6 +132,7 @@ static void fill_in_inode(struct inode *tmp_inode, int new_buf_type,
  __u32 attr;
  __u64 allocation_size;
  __u64 end_of_file;
+ umode_t default_mode;
 
  /* save mtime and size */
  local_mtime = tmp_inode->i_mtime;
@@ -187,48 +188,54 @@ static void fill_in_inode(struct inode *tmp_inode, int new_buf_type,
  if (atomic_read(&cifsInfo->inUse) == 0) {
  tmp_inode->i_uid = cifs_sb->mnt_uid;
  tmp_inode->i_gid = cifs_sb->mnt_gid;
- /* set default mode. will override for dirs below */
- tmp_inode->i_mode = cifs_sb->mnt_file_mode;
- } else {
- /* mask off the type bits since it gets set
- below and we do not want to get two type
- bits set */
+ }
+
+ if (attr & ATTR_DIRECTORY)
+ default_mode = cifs_sb->mnt_dir_mode;
+ else
+ default_mode = cifs_sb->mnt_file_mode;
+
+ /* set initial permissions */
+ if ((atomic_read(&cifsInfo->inUse) == 0) ||
+    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DYNPERM) == 0)
+ tmp_inode->i_mode = default_mode;
+ else {
+ /* just reenable write bits if !ATTR_READONLY */
+ if ((tmp_inode->i_mode & S_IWUGO) == 0 &&
+    (attr & ATTR_READONLY) == 0)
+ tmp_inode->i_mode |= (S_IWUGO & default_mode);
+
  tmp_inode->i_mode &= ~S_IFMT;
  }
 
- if (attr & ATTR_DIRECTORY) {
- *pobject_type = DT_DIR;
- /* override default perms since we do not lock dirs */
- if (atomic_read(&cifsInfo->inUse) == 0)
- tmp_inode->i_mode = cifs_sb->mnt_dir_mode;
- tmp_inode->i_mode |= S_IFDIR;
- } else if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) &&
-   (attr & ATTR_SYSTEM)) {
+ /* clear write bits if ATTR_READONLY is set */
+ if (attr & ATTR_READONLY)
+ tmp_inode->i_mode &= ~S_IWUGO;
+
+ /* set inode type */
+ if ((attr & ATTR_SYSTEM) &&
+    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL)) {
  if (end_of_file == 0)  {
- *pobject_type = DT_FIFO;
  tmp_inode->i_mode |= S_IFIFO;
+ *pobject_type = DT_FIFO;
  } else {
- /* rather than get the type here, we mark the
- inode as needing revalidate and get the real type
- (blk vs chr vs. symlink) later ie in lookup */
- *pobject_type = DT_REG;
+ /*
+ * trying to get the type can be slow, so just call
+ * this a regular file for now, and mark for reval
+ */
  tmp_inode->i_mode |= S_IFREG;
+ *pobject_type = DT_REG;
  cifsInfo->time = 0;
  }
-/* we no longer mark these because we could not follow them */
-/*        } else if (attr & ATTR_REPARSE) {
- *pobject_type = DT_LNK;
- tmp_inode->i_mode |= S_IFLNK; */
  } else {
- *pobject_type = DT_REG;
- tmp_inode->i_mode |= S_IFREG;
- if (attr & ATTR_READONLY)
- tmp_inode->i_mode &= ~(S_IWUGO);
- else if ((tmp_inode->i_mode & S_IWUGO) == 0)
- /* the ATTR_READONLY flag may have been changed on   */
- /* server -- set any w bits allowed by mnt_file_mode */
- tmp_inode->i_mode |= (S_IWUGO & cifs_sb->mnt_file_mode);
- } /* could add code here - to validate if device or weird share type? */
+ if (attr & ATTR_DIRECTORY) {
+ tmp_inode->i_mode |= S_IFDIR;
+ *pobject_type = DT_DIR;
+ } else {
+ tmp_inode->i_mode |= S_IFREG;
+ *pobject_type = DT_REG;
+ }
+ }
 
  /* can not fill in nlink here as in qpathinfo version and Unx search */
  if (atomic_read(&cifsInfo->inUse) == 0)
--
1.5.4.5

---

[0002-CIFS-disable-most-mode-changes-on-non-unix-non-cif.patch]

---

>From 7d4ccff7c7e3ee051ea7385184ac12f5f21b6c88 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@...>
Date: Tue, 13 May 2008 07:05:11 -0700
Subject: [PATCH 2/4] [CIFS] disable most mode changes on non-unix/non-cifsacl mounts

CIFS currently allows you to change the mode of an inode on a share that
doesn't have unix extensions enabled, and isn't using cifsacl. The inode
in this case *only* has its mode changed in memory on the client. This
is problematic since it can change any time the inode is purged from the
cache.

This patch makes cifs_setattr silently ignore most mode changes when
unix extensions and cifsacl support are not enabled, and when the share
is not mounted with the "dynperm" option. The exceptions are:

When a mode change would remove all write access to an inode we turn on
the ATTR_READONLY bit on the server and remove all write bits from the
inode's mode in memory.

When a mode change would add a write bit to an inode that previously had
them all turned off, it turns off the ATTR_READONLY bit on the server,
and resets the mode back to what it would normally be (generally, the
file_mode or dir_mode of the share).

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/inode.c |   38 ++++++++++++++++++++++++++------------
 1 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index aa8a10c..c64db5c 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1543,7 +1543,7 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  attrs->ia_valid &= ~ATTR_MODE;
 
  if (attrs->ia_valid & ATTR_MODE) {
- cFYI(1, ("Mode changed to 0x%x", attrs->ia_mode));
+ cFYI(1, ("Mode changed to 0%o", attrs->ia_mode));
  mode = attrs->ia_mode;
  }
 
@@ -1558,18 +1558,18 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
 #ifdef CONFIG_CIFS_EXPERIMENTAL
  if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL)
  rc = mode_to_acl(inode, full_path, mode);
- else if ((mode & S_IWUGO) == 0) {
-#else
- if ((mode & S_IWUGO) == 0) {
+ else
 #endif
- /* not writeable */
- if ((cifsInode->cifsAttrs & ATTR_READONLY) == 0) {
- set_dosattr = true;
- time_buf.Attributes =
- cpu_to_le32(cifsInode->cifsAttrs |
-    ATTR_READONLY);
- }
- } else if (cifsInode->cifsAttrs & ATTR_READONLY) {
+ if (((mode & S_IWUGO) == 0) &&
+    (cifsInode->cifsAttrs & ATTR_READONLY) == 0) {
+ set_dosattr = true;
+ time_buf.Attributes = cpu_to_le32(cifsInode->cifsAttrs |
+  ATTR_READONLY);
+ /* fix up mode if we're not using dynperm */
+ if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DYNPERM) == 0)
+ attrs->ia_mode = inode->i_mode & ~S_IWUGO;
+ } else if ((mode & S_IWUGO) &&
+   (cifsInode->cifsAttrs & ATTR_READONLY)) {
  /* If file is readonly on server, we would
  not be able to write to it - so if any write
  bit is enabled for user or group or other we
@@ -1580,6 +1580,20 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  /* Windows ignores set to zero */
  if (time_buf.Attributes == 0)
  time_buf.Attributes |= cpu_to_le32(ATTR_NORMAL);
+
+ /* reset local inode permissions to normal */
+ if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DYNPERM)) {
+ attrs->ia_mode &= ~(S_IALLUGO);
+ if (S_ISDIR(inode->i_mode))
+ attrs->ia_mode |=
+ cifs_sb->mnt_dir_mode;
+ else
+ attrs->ia_mode |=
+ cifs_sb->mnt_file_mode;
+ }
+ } else if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DYNPERM)) {
+ /* ignore mode change - ATTR_READONLY hasn't changed */
+ attrs->ia_valid &= ~ATTR_MODE;
  }
  }
 
--
1.5.4.5

---

[0003-CIFS-when-creating-new-inodes-use-file_mode-dir_m.patch]

---

>From b6a5ccdaf7960bfd131db2a714afe9ada7cc20b4 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@...>
Date: Tue, 13 May 2008 07:05:20 -0700
Subject: [PATCH 3/4] [CIFS] when creating new inodes, use file_mode/dir_mode exclusively on mount without unix extensions

When CIFS creates a new inode on a mount without unix extensions, it
temporarily assigns the mode that was passed to it in the create/mkdir
call. Eventually, when the inode is revalidated, it changes to have the
file_mode or dir_mode for the mount. This is confusing to users who
expect that the mode shouldn't change this way. It's also problematic
since only the mode is treated this way, not the uid or gid. Suppose you
have a CIFS mount that's mounted with:

uid=0,gid=0,file_mode=0666,dir_mode=0777

...if an unprivileged user comes along and does this on the mount:

mkdir -m 0700 foo
touch foo/bar

...there is a period of time where the touch will fail, since the dir
will initially be owned by root and have mode 0700. If the user waits
long enough, then "foo" will be revalidated and will get the correct
dir_mode permissions.

This patch changes cifs_mkdir and cifs_create to not overwrite the
mode found by the initial cifs_get_inode_info call after the inode is
created on the server. Legacy behavior can be reenabled with the
new "dynperm" mount option.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/dir.c   |    4 +++-
 fs/cifs/inode.c |    7 +++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index e4e0078..597a849 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -242,7 +242,9 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
  buf, inode->i_sb, xid,
  &fileHandle);
  if (newinode) {
- newinode->i_mode = mode;
+ if (cifs_sb->mnt_cifs_flags &
+    CIFS_MOUNT_DYNPERM)
+ newinode->i_mode = mode;
  if ((oplock & CIFS_CREATE_ACTION) &&
     (cifs_sb->mnt_cifs_flags &
      CIFS_MOUNT_SET_UID)) {
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index c64db5c..e120d3c 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -998,8 +998,11 @@ mkdir_get_info:
  CIFS_MOUNT_MAP_SPECIAL_CHR);
  }
  if (direntry->d_inode) {
- direntry->d_inode->i_mode = mode;
- direntry->d_inode->i_mode |= S_IFDIR;
+ if (cifs_sb->mnt_cifs_flags &
+     CIFS_MOUNT_DYNPERM)
+ direntry->d_inode->i_mode =
+ (mode | S_IFDIR);
+
  if (cifs_sb->mnt_cifs_flags &
      CIFS_MOUNT_SET_UID) {
  direntry->d_inode->i_uid =
--
1.5.4.5

---

[0004-CIFS-silently-ignore-ownership-changes-unless-unix.patch]

---

>From 15f56c11738b2a6da4107f34220dbd0181a606c2 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@...>
Date: Tue, 13 May 2008 07:05:21 -0700
Subject: [PATCH 4/4] [CIFS] silently ignore ownership changes unless unix extensions are enabled or we're faking uid changes

CIFS currently allows you to change the ownership of a file, but unless
unix extensions are enabled this change is not passed off to the server.

Have CIFS silently ignore ownership changes that can't be persistently
stored on the server unless the "setuids" option is explicitly
specified.

We could return an error here (-EOPNOTSUPP or something), but this is
how most disk-based windows filesystems on behave on Linux (e.g.  VFAT,
NTFS, etc). With cifsacl support and proper Windows to Unix idmapping
support, we may be able to do this more properly in the future.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/inode.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index e120d3c..fc6f69f 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1530,13 +1530,26 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  } else
  goto cifs_setattr_exit;
  }
- if (attrs->ia_valid & ATTR_UID) {
- cFYI(1, ("UID changed to %d", attrs->ia_uid));
- uid = attrs->ia_uid;
- }
- if (attrs->ia_valid & ATTR_GID) {
- cFYI(1, ("GID changed to %d", attrs->ia_gid));
- gid = attrs->ia_gid;
+
+ /*
+ * Without unix extensions we can't send ownership changes to the
+ * server, so silently ignore them. This is consistent with how
+ * local DOS/Windows filesystems behave (VFAT, NTFS, etc). With
+ * CIFSACL support + proper Windows to Unix idmapping, we may be
+ * able to support this in the future.
+ */
+ if (!pTcon->unix_ext &&
+    !(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID)) {
+ attrs->ia_valid &= ~(ATTR_UID | ATTR_GID);
+ } else {
+ if (attrs->ia_valid & ATTR_UID) {
+ cFYI(1, ("UID changed to %d", attrs->ia_uid));
+ uid = attrs->ia_uid;
+ }
+ if (attrs->ia_valid & ATTR_GID) {
+ cFYI(1, ("GID changed to %d", attrs->ia_gid));
+ gid = attrs->ia_gid;
+ }
  }
 
  time_buf.Attributes = 0;
--
1.5.4.5

---

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client