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. 


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/