Bug 13861

Summary: CIFS mounts ignore uid argument (ok in 2.6.30.3)
Product: File System Reporter: bugzilla.kernel.org
Component: CIFSAssignee: Jeff Layton (jlayton)
Status: CLOSED CODE_FIX    
Severity: high CC: akpm, jlayton, rjw, sfrench
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.31_2.6.31-020631rc4 from ubuntu mainline PPA Subsystem:
Regression: Yes Bisected commit-id:
Bug Depends on:    
Bug Blocks: 13615    

Description bugzilla.kernel.org 2009-07-28 21:39:24 UTC
When my 32bit Ubuntu 9.04 client uses 2.6.31rc4 to mount a CIFS share (from a Fedora server) using the 'uid=' option, the specified uid is ignored, and the uid on the server shows up locally instead of the specified one.
Rebooting into an earlier kernel (such as 2.6.30.3) the uid= argument is honoured, so this isn't a Samba bug...

Example fstab line:
//server.house/username /home/username/smb4k/server-username cifs noexec,credentials=/home/username/.cifsPassword,uid=username 0 0

Username is uid 500 remotely, but 1000 local.

Correct output from .30:
$ ls -lah ~/smb4k/server-username/|head
total 235M
drwxr-xr-x 72 username    500    0 2009-07-28 22:00 .
drwxr-xr-x  6 username username 4.0K 2009-04-07 10:15 ..
drwxr-xr-x  2 username    500    0 2005-09-24 15:19 aFile

Under .31 something more like:
$ ls -lah ~/smb4k/server-username/|head
total 235M
drwxr-xr-x 72 500    500    0 2009-07-28 22:00 .
drwxr-xr-x  6 500 500 4.0K 2009-04-07 10:15 ..
drwxr-xr-x  2 500    500    0 2005-09-24 15:19 aFile

and obviously I can't write to my remote home directory anymore :-(
Comment 1 Jeff Layton 2009-07-29 01:55:00 UTC
This is an expected behavior change. It's unfortunate, but there was no way to fix this problem in a backward-compatible way. To get the legacy behavior when unix extensions are enabled, you'll need to add the "forceuid" and/or the "forcegid" mount options.
Comment 2 bugzilla.kernel.org 2009-07-29 06:26:17 UTC
So the option has been renamed for some reason ? Google doesn't turn up much of any sense for me right now, and I'm obviously not 'in the lopp' on what the Samba people are up to.

Assuming this really does have to be done, could we not log a warning along the lines of 'old mount option used, change to forceXXX' - that would solve a lot of head scratching !
Comment 3 Jeff Layton 2009-07-29 10:14:23 UTC
The option wasn't renamed. There were new ones added. The reasoning is in this patch description:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=4ae1507f6d266d0cc3dd36e474d83aad70fec9e4

IOW, you'll still need to use the "uid=" option but if you want the ownership information provided by the server to be overridden then you need to add the forceuid option.

The solution doesn't lend itself well to printk warnings at mount time either given that uid= is still a valid option. I don't like making "silent" behavior changes, but I believe we had no choice but to do this to fix the problems with the current behavior.
Comment 4 bugzilla.kernel.org 2009-07-29 17:11:29 UTC
Well, I can't argue much with both IBM and RedHat can I :-)

I don't see why "not possible to have a mount by an unprivileged
user that shows the server's file ownership info." is such a problem that we would be prepared to introduce a silent change of behaviour, breaking peoples (long time) working set ups, and all without any warning. 
There is obviously a use case I've missed, because I think if you specify a particular local user to use, that should override what the server sends, after all I obviously know what I am doing :-)

If the idea is to add the feature that local non-root users can mount without override, why not add a new option (useserverownership=) that defaults to 'no' the old behaviour, but can be set 'yes' for users who have mounts in /etc/fstab and want the new behaviour. 
Or alternatively, default the new forceuid= option to what it needs to be to not break existing installs (which logically has the same effect as my hypothetical 'useserverownership'). People who need the new feature can turn it the other way in fstab.

That would mean everyone* caries on as normal, and people who want a different behaviour can go turn it on. I just don't understand why the default has to change, I guess.

Could we really not print a warning like "uid= without forceuid=, this may no longer do what you want [url]" ? With a good URL (not the commit message, which is fairly unclear) that would mitigate a lot of the pain.
Comment 5 Jeff Layton 2009-07-29 17:31:22 UTC
Apologies if the patch description was unclear. Adding Steve to the discussion here since he has a stake in this as well...

My inclination is not to clutter up the logs with printk spam, but I'm willing to listen to arguments to the contrary.

I think changing the default behavior of the uid= option was the right thing to do. The existing behavior made *absolutely* no sense. When the mount was done by root (uid = 0) then we trusted that the ownership information from the server was correct. When done by an unprivileged user (uid != 0), we clobbered all of the ownership info from the server.

While it means some minor pain for people depending on the legacy behavior, I believe it means a cleaner set of mount options going forward.
Comment 6 Jeff Layton 2009-07-29 17:47:06 UTC
Oh, and you certainly can argue with Red Hat and IBM, but since this comes down to a matter of default behavior, it's probably best to discuss it among a wider audience than bugzilla.

The usual forum is here:

https://lists.samba.org/mailman/listinfo/linux-cifs-client
Comment 7 Andrew Morton 2009-07-29 22:33:02 UTC
Breaking existing setups was very very bad of us.  I didn't know about this.

It would have been better to leave the uid= behaviour unaltered and to add *new* mount options for the new functionality.  Then migrate userspace over to stop using uid= altogether.  Then deprecate and finally remove uid= support.

To aid in this process we could print a warning if userspace uses uid=.

At the very very least the kernel should be changed (immediately!) to emit a printk when it detects uid=, directing operators to the documentation which will allow them to unbreak their machines.  But this is a worse solution.


Please don't let us release 2.6.31 with this regression(!) in place in its current form.
Comment 8 Jeff Layton 2009-07-29 22:52:16 UTC
Fair enough. I dislike saddling people that have what I consider to be more "correct" setups with extra mount options. But, if it's too costly to break existing setups this way I'm ok with reversing the default for now to ease the transition. Here's what I propose:

We'll back out the existing patch and add new "noforceuid" and "noforcegid" options instead.

Once that is done, the question becomes whether we should begin a transition to a different default. I argue yes. The existing default is more confusing IMO.

To bring that about we'll add a printk that warns that the default is going to change in the future (maybe in 2.6.33) and that the user's preferences will need to be declared by adding either forceuid or noforceuid to the options (i.e., the printk will only fire when neither is present). Ditto for forcegid.

We'll leave that printk in place for 2 releases, and then switch the default.

Sound reasonable? If so, I'll code up some patches and post them for review in the next day or so.
Comment 9 Andrew Morton 2009-07-29 22:57:28 UTC
Sounds good, thanks.
Comment 10 Rafael J. Wysocki 2009-07-29 23:11:54 UTC
Well, I'm not quite sure what Linus is going to say to this and the "kdesu broken" thread on the LKML is a good example of what happens when existing user space is broken.  FWIW.
Comment 11 Steve French 2009-07-29 23:54:42 UTC
I don't like the idea of warning about upcoming behavior changes in the kernel dmesg (seems like that could result in printk spam), but I could certainly live with new mount parms to accomplish Jeff's goal of making the uid behavior for unpriv mounts more sensible.
Comment 12 Steve French 2009-07-29 23:57:45 UTC
(clarifying my previous comment) I don't mind going back to the old uid behavior (but adding new mount parms to accomplish Jeff's goal) also am interested if adding warnings in user space (mount.cifs) would be possible
Comment 13 Jeff Layton 2009-07-30 00:55:56 UTC
What I need clarification on is what you think the default should (eventually) be. Should we transition this code so that server provided uid's are the default even when uid= is specified? Or should we leave the default as-is?

If we're planning to leave the default as-is, we can forgo warnings altogether (and maybe can change the parameter name to "serveruids" or maybe "servergids" or something).

If we want to change the default eventually, then we need to add a warning to make it clear when and how it'll be changing. Doing this is userspace is tough. How will mount.cifs know what the kernel's default is? I think the only reasonable way to do that is from in-kernel with a printk.

The good news is that it needn't throw printk spam forever. We'll only throw the warning when someone specifies uid= and doesn't add forceuid or noforceuid. If the admin adds those options then we can silence it.
Comment 14 Steve French 2009-07-30 02:21:16 UTC
I will discuss options with Jeff tomorrow, but this reminds me that for 2.6.32 or 2.6.33 we need to investigate:

1) how to detect the case where uids on server and client don't match (not always easy to do, but there are some special cases where we can tell)
2) Uid mapping (multiple ways to do this)
Comment 15 Steve French 2009-07-30 22:55:58 UTC
I reviewed Jeff's proposed patch, and am ok with it with minor corrections (a warning message that was left out).
Comment 16 Jeff Layton 2009-07-31 11:47:09 UTC
Patch posted to LKML, linux-fsdevel and linux-cifs-client this morning. Testing and/or review would be appreciated before I ask Steve to push this to Linus.
Comment 17 Rafael J. Wysocki 2009-08-02 21:14:39 UTC
Handled-By : Jeff Layton <jlayton@redhat.com>
Patch : http://patchwork.kernel.org/patch/38498/
Comment 18 Steve French 2009-08-03 02:36:57 UTC
Merged into cifs-2.6.git

Will request upstream merge within a few days.
Comment 19 Rafael J. Wysocki 2009-08-09 23:05:40 UTC
Fixed by commit 9b9d6b2434fe942895c341b9a982f158529788ec .