[RFC][PATCH] UDF filesystem uid fix

[RFC][PATCH] UDF filesystem uid fix

[Peter Osterlund]

Hi,

I would appreciate if someone with file system knowledge could review
this patch from Phillip Susi.


From: Phillip Susi <psusi@cfl.rr.com>

The UDF filesystem refused to update the file's uid and gid on the
disk if the in memory inode's id matched the values in the uid= and
gid= mount options.  This was causing the owner to change from the
desktop user to root when the volume was ejected and remounted.  I
changed this so that if the inode's id matches the mount option, it
writes a -1 to disk, because when the filesystem reads a -1 from disk,
it uses the mount option for the in memory inode.  This allows you to
use the uid/gid mount options in the way you would expect.
---

 fs/udf/inode.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 395e582..f892948 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1337,9 +1337,13 @@ udf_update_inode(struct inode *inode, in
 
 	if (inode->i_uid != UDF_SB(inode->i_sb)->s_uid)
 		fe->uid = cpu_to_le32(inode->i_uid);
+	else
+		fe->uid = cpu_to_le32(-1);
 
 	if (inode->i_gid != UDF_SB(inode->i_sb)->s_gid)
 		fe->gid = cpu_to_le32(inode->i_gid);
+	else
+		fe->gid = cpu_to_le32(-1);
 
 	udfperms =	((inode->i_mode & S_IRWXO)     ) |
 			((inode->i_mode & S_IRWXG) << 2) |

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [RFC][PATCH] UDF filesystem uid fix

[Pekka Enberg]

Hi,

On 12 Feb 2006 19:17:38 +0100, Peter Osterlund <petero2@telia.com> wrote:
> The UDF filesystem refused to update the file's uid and gid on the
> disk if the in memory inode's id matched the values in the uid= and
> gid= mount options.  This was causing the owner to change from the
> desktop user to root when the volume was ejected and remounted.  I
> changed this so that if the inode's id matches the mount option, it
> writes a -1 to disk, because when the filesystem reads a -1 from disk,
> it uses the mount option for the in memory inode.  This allows you to
> use the uid/gid mount options in the way you would expect.

The UDF code really seems broken. It fails for new inodes and some
chown cases, when the mount options are being used. Phillip's patch
does not look like a complete fix, though, as it will store invalid
uid/gid (-1) for some cases where we probably should be storing the
real uid/gid. For example, doing chown <user> when the same user is
passed as mount option, we'll get -1 on disk, instead of user's uid.

I think the semantics you want is: "if uid/gid is invalid on disk,
leave it that way unless we explicitly change it via chown; otherwise
we can always overwrite it." Hmm?

                                  Pekka

Re: [RFC][PATCH] UDF filesystem uid fix

[Phillip Susi]

Pekka Enberg wrote:
> The UDF code really seems broken. It fails for new inodes and some
> chown cases, when the mount options are being used. Phillip's patch
> does not look like a complete fix, though, as it will store invalid
> uid/gid (-1) for some cases where we probably should be storing the
> real uid/gid. For example, doing chown <user> when the same user is
> passed as mount option, we'll get -1 on disk, instead of user's uid.
>   

Could you be more specific about what test cases fail?  It worked fine 
for me so far.

Also the storage of -1 id is very much intentional.  Prior to this 
patch, if you mount with uid=yourid then you create a file, it would 
appear to be owned by you.  If you unmounted and remounted the volume 
however, it would suddenly be owned by root.  Clearly that is not 
acceptable. With this patch applied, the file appears to be owned by you 
both before and after the remount.  The actual value written to disk is 
-1, which on a read is translated to the value from the mount option, 
giving the appearance that the file is owned by the user specified in 
the mount, which is the expected behavior. 

Should the desktop user insert this media in another machine where they 
have a different uid ( that is specified in the mount options ) then 
they would still have access to their files, as the -1 will be 
translated to the correct uid on that system. 

Should you chown files to a user other than the one given in the mount 
option, then that actual uid is stored on the disk.  Also if you do not 
specify a uid/gid when you mount, then the actual uid is stored. 
> I think the semantics you want is: "if uid/gid is invalid on disk,
> leave it that way unless we explicitly change it via chown; otherwise
> we can always overwrite it." Hmm?
>   

No, the semantics is if the uid/gid on disk is invalid, then use the id 
specified in the mount options.  That was the case before, however when 
you created new files or chowned files to you ( and you gave your id in 
the mount options ), an id of 0 was written to disk instead.  Now -1 is 
written, which when read, is mapped back to your id specified in the 
mount options. 

Re: [RFC][PATCH] UDF filesystem uid fix

[Pekka J Enberg]

On Mon, 13 Feb 2006, Phillip Susi wrote:
> Could you be more specific about what test cases fail?  It worked fine for me
> so far.

I don't haven an UDF partition to test on so I am only reading the code. 
With your patch, every time an in-memory inode has the same uid/gid as the 
one you passed as mount option, the id on disk will become -1, no? So for 
example, doing chown 100 for a file writes -1 to disk if you passed 100 
as uid mount option. Am I missing something here?

On Mon, 13 Feb 2006, Phillip Susi wrote:
> > I think the semantics you want is: "if uid/gid is invalid on disk,
> > leave it that way unless we explicitly change it via chown; otherwise
> > we can always overwrite it." Hmm?
> 
> No, the semantics is if the uid/gid on disk is invalid, then use the id
> specified in the mount options.  That was the case before, however when you
> created new files or chowned files to you ( and you gave your id in the mount
> options ), an id of 0 was written to disk instead.  Now -1 is written, which
> when read, is mapped back to your id specified in the mount options. 

Yes, I agree that the current code is broken. I was talking about what the 
semantics should be and that your patch doesn't quite get us there. Do you 
disagree with that? The UDF specification I am looking at [1] says that -1 
is used by operating systems that do not support uid/gid to denote an 
invalid id (although ECMA-167 doesn't seem to have such rule), which is  
why I think it's an bad idea for Linux to ever write it on disk. Instead, 
we should always write the proper id on disk unless it was invalid in the 
first place and we did not explicity change it (via chown, for example).

  1. http://www.osta.org/specs/pdf/udf260.pdf

				Pekka

Re: [RFC][PATCH] UDF filesystem uid fix

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

On Tue, 14 Feb 2006 09:28:30 +0200 (EET) Pekka J Enberg wrote:

> Yes, I agree that the current code is broken. I was talking about what the 
> semantics should be and that your patch doesn't quite get us there. Do you 
> disagree with that? The UDF specification I am looking at [1] says that -1 
> is used by operating systems that do not support uid/gid to denote an 
> invalid id (although ECMA-167 doesn't seem to have such rule), which is  
> why I think it's an bad idea for Linux to ever write it on disk. Instead, 
> we should always write the proper id on disk unless it was invalid in the 
> first place and we did not explicity change it (via chown, for example).

Storing uid/gid values on the filesystem is not always good.  Imagine
that you need to work with the same removable media on different
machines, where you have accounts with different uids; in this case
uid/gid values stored on one machine have no meaning everywhere else.
It would be good to have a mount option for UDF which turns off the
uid/gid handling completely and shows all files on the filesystem with
uid/gid specified by mount options.

See also the recent thread "Filesystem for mobile hard drive":

http://lkml.org/lkml/2006/2/12/64

>   1. http://www.osta.org/specs/pdf/udf260.pdf

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC][PATCH] UDF filesystem uid fix

[Phillip Susi]

Pekka J Enberg wrote:
> I don't haven an UDF partition to test on so I am only reading the code. 
> With your patch, every time an in-memory inode has the same uid/gid as the 
> one you passed as mount option, the id on disk will become -1, no? So for 
> example, doing chown 100 for a file writes -1 to disk if you passed 100 
> as uid mount option. Am I missing something here?
>
>   
That is exactly correct. 

> Yes, I agree that the current code is broken. I was talking about what the 
> semantics should be and that your patch doesn't quite get us there. Do you 
> disagree with that? The UDF specification I am looking at [1] says that -1 
> is used by operating systems that do not support uid/gid to denote an 
> invalid id (although ECMA-167 doesn't seem to have such rule), which is  
> why I think it's an bad idea for Linux to ever write it on disk. Instead, 
> we should always write the proper id on disk unless it was invalid in the 
> first place and we did not explicity change it (via chown, for example).
Sometimes you DON'T want a valid UID written to the disk.  In the case 
of a typical desktop user, there is only one uid that is ever going to 
access the disk, but that uid may be different on each computer, even 
though it's the same person.  Thus they want to be able to take the disc 
from computer to computer, and have access to their files.  Since the 
existing uid/gid override mount options only apply if the on disk id is 
-1, it seemed a simple and appropriate thing to store -1 in the case 
where the id matches the mount option.  The only use case that this 
patch changes is where the id matches the mount option.  In that case, 
the user expected behavior is for the files on the disc to appear to be 
owned by the specified uid, with the added benefit that this will hold 
true if you remount with another uid specified, possibly even on another 
machine.  This seems to meet user expectations much better than the 
previous behavior of changing ownership to root when unmounted. 

If you want real IDs to be stored, then do nothing.  Simply continue to 
use the system just like before, where you did not specify a uid as a 
mount option. 

Re: [RFC][PATCH] UDF filesystem uid fix

[Pekka Enberg]

On 2/14/06, Phillip Susi <psusi@cfl.rr.com> wrote:
> This seems to meet user expectations much better than the
> previous behavior of changing ownership to root when unmounted.

Reading mount man pages, the definition for uid/gid mount option for
udf is "default user/group." I am pretty convinced that their original
intent was to provide meaningful id for inodes that don't have one.
Now what you're looking for sounds more like "mount whole filesystem
as user/group" which is something different.

The ownership changing to root thing is a bug caused by explicit
memset() straight after we read the inode followed by flawed logic
that forgets to set the uid. Your patch doesn't really fix that
either, but it masks it. Unfortunately, now combining uid/git options
with filesystem in which some inodes _have_ proper id yields to
strange results. I don't see how it's reasonable for a filesystem to
write invalid id on disk if I change the owner to myself even if I did
use the mount options.

So I don't think your patch is a proper fix for the ownership changing
to root bug, nor do I think it is sufficient to provide the "mount fs
as user" thing (which does sound useful). Now if you want to change
uid/gid option semantics to "mount fs as user", please explain why you
don't think my case matters, that is using uid/gid to provide id for
inodes that don't have one but still expecting chown to work?

                                        Pekka

Re: [RFC][PATCH] UDF filesystem uid fix

[Phillip Susi]

Pekka Enberg wrote:
> Reading mount man pages, the definition for uid/gid mount option for
> udf is "default user/group." I am pretty convinced that their original
> intent was to provide meaningful id for inodes that don't have one.
> Now what you're looking for sounds more like "mount whole filesystem
> as user/group" which is something different.
>   

Not quite, it's a bit of a compromise.  It only applies the id as a 
default for on disk inodes which don't have an id ( it is -1 ).  My 
patch just fixes it so that when writing IDs back to disk, it stores -1 
only in the case where the id matches the mount option.  If a file is 
owned by any other id, then that is what gets written to the disk. 

Generally speaking, this option is used to make sure that the desktop 
user has access to files on removable media which normally would have 
incorrect ownerships ( root ).  With this patch, the ownerships are 
maintained in the form the desktop user expects, which is to say, they 
own the files, not root or nobody. 
> The ownership changing to root thing is a bug caused by explicit
> memset() straight after we read the inode followed by flawed logic
> that forgets to set the uid. Your patch doesn't really fix that
> either, but it masks it. Unfortunately, now combining uid/git options
> with filesystem in which some inodes _have_ proper id yields to
> strange results. I don't see how it's reasonable for a filesystem to
> write invalid id on disk if I change the owner to myself even if I did
> use the mount options.
>
>   

What strange results?  If the filesystem has proper IDs on it they will 
be maintained.  The only case this patch effects is when the ID matches 
that given on the mount options, in which case, the user can not tell 
the difference; the file always looks like it is owned by him, unless 
you remount with a different mount option. 

If you create a file on the disk and you gave your id in the mount 
options, what difference does it make if the media gets a -1 written to 
it?  You still see the files as owned by you. 

> So I don't think your patch is a proper fix for the ownership changing
> to root bug, nor do I think it is sufficient to provide the "mount fs
> as user" thing (which does sound useful). Now if you want to change
> uid/gid option semantics to "mount fs as user", please explain why you
> don't think my case matters, that is using uid/gid to provide id for
> inodes that don't have one but still expecting chown to work?

IIRC, the file changed ownership to root because the existing code only 
saved the id to disk if it did NOT match the mount option.  I suppose I 
could have simply removed the test and always stored the id, but then 
the mount option would be meaningless.  Thus it seemed more logical to 
handle the other case, and store a value ( -1 ) such that when 
remounted, the mount option would have meaning.

Also, chown does still work.  If you chown a file to an id other than 
the one in the mount option, that id will be preserved on disk.  If you 
chown a file to the id in the mount option, then it will appear to work 
correctly, so long as you don't change the mount option.  If you do 
change the mount option, it is likely because you are logging in on 
another machine where your id is different, or you have given the disc 
to someone else and want them to be able to access it.  In that case, 
you will be treated as the same user, which is a desired result. 


In the general case, it would be nice to have two options; one that 
specifies defaults that only apply if there is no owner info, and 
another option that overrides any owner info that does exist.  In the 
specific case of removable media though, having an option to not bother 
storing ownerships is handy because most of the time, you don't care 
about that info and it just gets in the way. 


Maybe I should amend the patch to work like this:

uid/gid : specify default id when -1 is on disk
uid/gid = force : ignore ids on disk
uid/gid = [no]save : do [not] save actual id to disk ( save -1 instead )

Possibly with nosave being the default.  Would this be more acceptable?

I thought about doing this but decided to go with the simpler patch 
because I really can't think of any case where storing uids on removable 
media makes any kind of sense, and udf was made for removable media, and 
ubuntu at least, auto mounts removable media via hal and pmount with the 
uid/gid options already. 

Re: [RFC][PATCH] UDF filesystem uid fix

[Pekka Enberg]

On Wed, 2006-02-15 at 10:55 -0500, Phillip Susi wrote:
> Maybe I should amend the patch to work like this:
> 
> uid/gid : specify default id when -1 is on disk
> uid/gid = force : ignore ids on disk
> uid/gid = [no]save : do [not] save actual id to disk ( save -1 instead )
> 
> Possibly with nosave being the default.  Would this be more acceptable?

Yeah, sounds much better to me. However, I am wondering if we can
actually drop the nosave/save cases completely. Wouldn't we get the same
semantics by letting uid/gid specify the default id and make the ignore
case look like we're always reading -1 from disk, and never writing out
any ids? So as a desktop user, you mount with "uid=", "gid=", and
"force" passed as mount option and it works as expected.

			Pekka

Re: [RFC][PATCH] UDF filesystem uid fix

[Phillip Susi]

Pekka Enberg wrote:
> Yeah, sounds much better to me. However, I am wondering if we can
> actually drop the nosave/save cases completely. Wouldn't we get the same
> semantics by letting uid/gid specify the default id and make the ignore
> case look like we're always reading -1 from disk, and never writing out
> any ids? So as a desktop user, you mount with "uid=", "gid=", and
> "force" passed as mount option and it works as expected.

True, that would work.  It would require the addition of another mount 
option though, so I wonder, is that really needed?  What problem with 
the current patch would this solve?  Is there really a need to save real 
ids to the disk with the current uid option and no force?  Keep in mind 
that udf is meant for removable media where the uids aren't going to 
make any sense in another system.


Maybe I'm just being lazy though... I'll dig back into it and try to 
submit a new patch with the force option by this weekend.

Re: [RFC][PATCH] UDF filesystem uid fix

[Pekka Enberg]

On Wed, 2006-02-15 at 13:48 -0500, Phillip Susi wrote:
> True, that would work.  It would require the addition of another mount 
> option though, so I wonder, is that really needed?  What problem with 
> the current patch would this solve?  Is there really a need to save real 
> ids to the disk with the current uid option and no force?  Keep in mind 
> that udf is meant for removable media where the uids aren't going to 
> make any sense in another system.

Yeah, but even for removable media proper uid/gid makes sense. For
instance, if I am sharing an usb stick with someone else on the same
computer, uid/gid is useful. Anyway, I prefer that force thing and if
everyone else disagrees with me, now would be a good time to say so.

			Pekka

Re: [RFC][PATCH] UDF filesystem uid fix

[Phillip Susi]

Finally got around to completing that patch.  Take a look and let me know if it looks good.  I posted it in the thread entitled "[PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options".  

Phillip Susi wrote:
> Pekka Enberg wrote:
>> Yeah, sounds much better to me. However, I am wondering if we can
>> actually drop the nosave/save cases completely. Wouldn't we get the same
>> semantics by letting uid/gid specify the default id and make the ignore
>> case look like we're always reading -1 from disk, and never writing out
>> any ids? So as a desktop user, you mount with "uid=", "gid=", and
>> "force" passed as mount option and it works as expected.
> 
> True, that would work.  It would require the addition of another mount 
> option though, so I wonder, is that really needed?  What problem with 
> the current patch would this solve?  Is there really a need to save real 
> ids to the disk with the current uid option and no force?  Keep in mind 
> that udf is meant for removable media where the uids aren't going to 
> make any sense in another system.
> 
> 
> Maybe I'm just being lazy though... I'll dig back into it and try to 
> submit a new patch with the force option by this weekend.
>