Follow on patches for uid=/gid= behavior changes

[Jeff Layton-2]

If the patch I posted this morning looks OK, here's a proposed README
patch, and a patch to the mount.cifs manpage.

Finally, here's a third patch that adds a warning about the change for the
default behavior for 2.6.33.

Thoughts on all 3?

--
Jeff Layton <jlayton@...>

---

From 200b7f2817aef7a33060b6bd6068b16175fa1379 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@...>
Date: Fri, 31 Jul 2009 09:34:26 -0400
Subject: [PATCH] cifs: add warning messages about change in default uid=/gid= behavior

Eventually, we'd like to change the meaning of the uid=/gid= options so
that they only specify file ownership when the server does not provide
that information. Changing this abruptly would be considered a regression,
so add a printk that warns about the upcoming change.

To silence the warning, admins can add the appropriate mount options to
clarify their intent when using these options.

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

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 1f3345d..a375356 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1361,15 +1361,31 @@ cifs_parse_mount_options(char *options, const char *devname,
  if (vol->UNCip == NULL)
  vol->UNCip = &vol->UNC[2];
 
- if (uid_specified)
+ if (uid_specified) {
  vol->override_uid = override_uid;
- else if (override_uid == 1)
+ if (override_uid == -1)
+ printk(KERN_WARNING "CIFS: the default behavior when "
+    "uid= is specified will change in "
+    "2.6.33 to only specify the owner "
+    "when the server does not provide "
+    "one. Add the forceuid or "
+    "noforceuid options to silence "
+    "this warning.\n");
+ } else if (override_uid == 1)
  printk(KERN_NOTICE "CIFS: ignoring forceuid mount option "
    "specified with no uid= option.\n");
 
- if (gid_specified)
+ if (gid_specified) {
  vol->override_gid = override_gid;
- else if (override_gid == 1)
+ if (override_gid == -1)
+ printk(KERN_WARNING "CIFS: the default behavior when "
+    "gid= is specified will change in "
+    "2.6.33 to only specify the group "
+    "when the server does not provide "
+    "one. Add the forcegid or "
+    "noforcegid options to silence "
+    "this warning.\n");
+ } else if (override_gid == 1)
  printk(KERN_NOTICE "CIFS: ignoring forcegid mount option "
    "specified with no gid= option.\n");
 
--
1.6.0.6

---

---

From a4293786a63e7b7bea6666ba55d22577fed866ed Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@...>
Date: Fri, 31 Jul 2009 09:51:53 -0400
Subject: [PATCH] cifs: update the README with information about uid=/forceuid/noforceuid

...and the corresponding gid options. Add some detail about how the
union of these options is intended to work.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/README |   55 +++++++++++++++++--------------------------------------
 1 files changed, 17 insertions(+), 38 deletions(-)

diff --git a/fs/cifs/README b/fs/cifs/README
index ad92921..a0223f5 100644
--- a/fs/cifs/README
+++ b/fs/cifs/README
@@ -262,44 +262,23 @@ A partial list of the supported mount options follows:
  mount.
   domain Set the SMB/CIFS workgroup name prepended to the
  username during CIFS session establishment
-  forceuid Set the default uid for inodes based on the uid
- passed in. For mounts to servers
- which do support the CIFS Unix extensions, such as a
- properly configured Samba server, the server provides
- the uid, gid and mode so this parameter should  not be
- specified unless the server and clients uid and gid
- numbering differ.  If the server and client are in the
- same domain (e.g. running winbind or nss_ldap) and
- the server supports the Unix Extensions then the uid
- and gid can be retrieved from the server (and uid
- and gid would not have to be specifed on the mount.
- For servers which do not support the CIFS Unix
- extensions, the default uid (and gid) returned on lookup
- of existing files will be the uid (gid) of the person
- who executed the mount (root, except when mount.cifs
- is configured setuid for user mounts) unless the "uid="
- (gid) mount option is specified.  For the uid (gid) of newly
- created files and directories, ie files created since
- the last mount of the server share, the expected uid
- (gid) is cached as long as the inode remains in
- memory on the client.   Also note that permission
- checks (authorization checks) on accesses to a file occur
- at the server, but there are cases in which an administrator
- may want to restrict at the client as well.  For those
- servers which do not report a uid/gid owner
- (such as Windows), permissions can also be checked at the
- client, and a crude form of client side permission checking
- can be enabled by specifying file_mode and dir_mode on
- the client.  Note that the mount.cifs helper must be
- at version 1.10 or higher to support specifying the uid
- (or gid) in non-numeric form.
-  forcegid (similar to above but for the groupid instead of uid)
-  uid Set the default uid for inodes, and indicate to the
- cifs kernel driver which local user mounted . If the server
- supports the unix extensions the default uid is
- not used to fill in the owner fields of inodes (files)
- unless the "forceuid" parameter is specified.
-  gid Set the default gid for inodes (similar to above).
+  uid Set the ownership for all inodes, and indicate to the
+ cifs kernel driver which local user initiated the mount.
+ If the server supports the unix extensions, the owner provided
+ by the server will be overridden in favor of the one provided
+ by this option. To disable this behavior use "noforceuid".
+  noforceuid Disable overriding ownership information from the server. With
+ this option, the value given in the uid= option will only be
+ used if the server does not provide one.
+  forceuid Reverse a previously specified noforceuid option.
+  gid Set the group ownership for all inodes. If the server supports
+ the unix extensions, the group provided by the server will be
+ overridden in favor of the one provided by this option. To
+ disable this behavior use "noforcegid".
+  noforcegid Disable overriding group ownership information from the server.
+ With this option, the value given in the gid= option will only
+ be used if the server does not provide one.
+  forcegid Reverse a previously specified noforcegid option.
   file_mode     If CIFS Unix extensions are not supported by the server
  this overrides the default mode for file inodes.
   dir_mode      If CIFS Unix extensions are not supported by the server
--
1.6.0.6

---

---

From a7708d4775a7f5789742baf832d25e4dfd45be59 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@...>
Date: Fri, 31 Jul 2009 09:19:32 -0400
Subject: [PATCH] manpage: update the mount.cifs manpage to reflect changes when uid= or gid= is specified

The change to not override ownership information when uid= is specified
was considered a regression so the older default behavior had to be
restored. Update the manpage to reflect the current situation
in-kernel.

Signed-off-by: Jeff Layton <jlayton@...>
---
 docs-xml/manpages-3/mount.cifs.8.xml |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/docs-xml/manpages-3/mount.cifs.8.xml b/docs-xml/manpages-3/mount.cifs.8.xml
index 9383f3f..b8b7b50 100644
--- a/docs-xml/manpages-3/mount.cifs.8.xml
+++ b/docs-xml/manpages-3/mount.cifs.8.xml
@@ -128,8 +128,7 @@ credentials file properly.
  <listitem>
 
  <para>sets the uid that will own all files or directories on the
-mounted filesystem when the server does not provide ownership
-information. It may be specified as either a username or a numeric uid.
+mounted filesystem. It may be specified as either a username or a numeric uid.
 When not specified, the default is uid 0.  The mount.cifs helper must be
 at version 1.10 or higher to support specifying the uid in non-numeric
 form. See the section on FILE AND DIRECTORY OWNERSHIP AND PERMISSIONS below for more
@@ -152,8 +151,7 @@ be the value of the uid= option. See the section on FILE AND DIRECTORY OWNERSHIP
  <listitem>
 
  <para>sets the gid that will own all files or
-directories on the mounted filesystem when the server does not provide
-ownership information.  It may be specified as either a groupname or a
+directories on the mounted filesystem.  It may be specified as either a groupname or a
 numeric gid.  When not specified, the default is gid 0. The mount.cifs
 helper must be at version 1.10 or higher to support specifying the gid
 in non-numeric form. See the section on FILE AND DIRECTORY OWNERSHIP AND
@@ -534,7 +532,8 @@ uid= or gid= options are set, and will have permissions set to the
 default file_mode and dir_mode for the mount. Attempting to change these
 values via chmod/chown will return success but have no effect.</para>
 
- <para>When the client and server negotiate unix extensions,
+ <para>When the client and server negotiate unix extensions
+and the uid= and gid= options are not specified,
 files and directories will be assigned the uid, gid, and mode provided
 by the server. Because CIFS mounts are generally single-user, and the
 same credentials are used no matter what user accesses the mount, newly
@@ -542,11 +541,15 @@ created files and directories will generally be given ownership
 corresponding to whatever credentials were used to mount the
 share.</para>
 
- <para>If the uid's and gid's being used do not match on the
-client and server, the forceuid and forcegid options may be helpful.
-Note however, that there is no corresponding option to override the
-mode. Permissions assigned to a file when forceuid or forcegid are in
-effect may not reflect the the real permissions.</para>
+ <para>If the uid= or gid= options are provided then the
+ownership of all files and directories on the mount will be overridden.
+To prevent the client from clobbering file ownership information on
+these mounts, use the "noforceuid" and "noforcegid" options. Note that
+file modes are not overriden in this situation. Since the default
+behavior is to override ownership when the uid= and gid= options are in
+effect but file and directory modes are preserved, one should be cautious
+when using these options since the resulting permissions may grant
+unintended privileges.</para>
 
  <para>When unix extensions are not negotiated, it's also
 possible to emulate them locally on the server using the "dynperm" mount
--
1.6.0.6

---

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

[Steve French-2]

The README update seems ok (although a little long, I didn't see an obvious way to shrink it).   I am not convinced that we want to add the warn about kernel behavior change (may be able to do this in mount.cifs more sensibly, or not change the behavior if users turn out to prefer this way)

On Fri, Jul 31, 2009 at 9:06 AM, Jeff Layton <jlayton@...> wrote:

If the patch I posted this morning looks OK, here's a proposed README
patch, and a patch to the mount.cifs manpage.

Finally, here's a third patch that adds a warning about the change for the
default behavior for 2.6.33.

Thoughts on all 3?

--
Jeff Layton <jlayton@...>

--
Thanks,

Steve

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

[Jeff Layton-2]

On Fri, 31 Jul 2009 10:04:07 -0500
Steve French <smfrench@...> wrote:

> The README update seems ok (although a little long, I didn't see an obvious
> way to shrink it).   I am not convinced that we want to add the warn about
> kernel behavior change (may be able to do this in mount.cifs more sensibly,
> or not change the behavior if users turn out to prefer this way)
>

Yeah, that's really my question at this point. Should we change the
default behavior?

I think it ultimately makes more sense to not clobber the ownership
unless it's specifically requested. As my evidence I submit the manpage
update -- there are more "ifs" and special cases when explaining the
the option behavior.

OTOH, it's a lot more effort to change the default. Maybe it's not
worth it.

Another idea: would it be better to dispense with forceuid/forcegid and
just add a new default_uid= or default_gid= options to specify what the
owner should be when one isn't provided (and when uid=/gid= aren't
specified).

Given that my first pass at this made everyone unhappy, I'd appreciate
some feedback.

--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Suresh Jayaraman-2]

Jeff Layton wrote:

> On Fri, 31 Jul 2009 10:04:07 -0500
> Steve French <smfrench@...> wrote:
>
>> The README update seems ok (although a little long, I didn't see an obvious
>> way to shrink it).   I am not convinced that we want to add the warn about
>> kernel behavior change (may be able to do this in mount.cifs more sensibly,
>> or not change the behavior if users turn out to prefer this way)
>>
>
> Yeah, that's really my question at this point. Should we change the
> default behavior?

I think yes. As suggested by Andrew in the bugzilla (leave old behavior,
add new options -> let users start using new options -> deprecate old
behavior and your patches are already doing this I think). And yes,
tradionally breaking existing behavior is frowned upon by kernel
community but, perhaps for a good reason.

And I agree with Steve on adding Kernel warning. Do we really need to
add a Kernel warning? Would it be sufficient if we warn in mount.cifs? I
think we normally don't warn user about upcoming changes in dmesg.. (and
user may not look at dmesg for such behavioral changes)

> I think it ultimately makes more sense to not clobber the ownership
> unless it's specifically requested. As my evidence I submit the manpage
> update -- there are more "ifs" and special cases when explaining the
> the option behavior.
>
> OTOH, it's a lot more effort to change the default. Maybe it's not
> worth it.

This sounds like the correct thing to me.

> Another idea: would it be better to dispense with forceuid/forcegid and
> just add a new default_uid= or default_gid= options to specify what the
> owner should be when one isn't provided (and when uid=/gid= aren't
> specified).

Too many options, wouldn't confuse users more, than helping?

> Given that my first pass at this made everyone unhappy, I'd appreciate
> some feedback.
>


Thanks,

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