Re: cvs commit: src/sbin/umount umount.c

In message <20031118154259.GA70896_at_fit.vutbr.cz>, Rudolf Cejka writes:
>>   If the unmount by file system ID fails, don't warn before retrying
>>   a non-fsid unmount if the file system ID is all zeros. This is a
...
>Hello and thanks for fixing this! I had a plan to report this, but you
>were faster :o) I'm interested in this area - please, can you tell, what
>do you plan to do in your more complete fix?
>
>When I looked at this issue, I thought about some things:
>
>* Why is f_fsid zeroed for non-root users at all? Is there any reason?

As I understand it, the main reason for hiding file system IDs from
non-root users is beacuse file system IDs are used as part of NFS
file handles on an NFS server, so hiding them makes it harder to
guess a valid file handle. If you know the file system ID and an
inode number, then you would only need to guess the 32-bit inode
generation number.

OpenBSD started zeroing out file system IDs for non-root users a
long time ago, and while FreeBSD mostly followed suit, I think it
was only with Kirk's 64-bit statfs changes a few days ago that we
have started doing this consistently (we had missed getfsstat()
before).

I was planning to return a filesystem ID of {st_dev, 0} to non-root
users, where st_dev is the device number that is already returned
by the stat(2) system call. This requires a few changes, because
currently st_dev comes from va_fsid in struct vattr, which is not
directly accessible at the time a file system is mounted. Since
many userland applications depend on st_dev being persistent and
unique, I think it makes more sense to have it as part of struct
mount instead of struct vattr.

Some additional changes are required to guarantee the uniqueness
of st_dev's and file system IDs (including {st_dev, 0} ones), and
then unmount(2) needs to accept these user-visible IDs. In fact,
maybe {st_dev, 0} could be returned to root too, but that might
possibly break some NFS-related utilities.

>* There are small typos in umount.c:

Thanks - fixed locally, but there's no urgency to commit before
5.2.

>* Do you understand, why there is line in umount.c:376
>  getmntentry(NULL, NULL, &sfs->f_fsid, REMOVE)
>  ? I'm not sure, but if it is needed for some reason,
>  I think that there should be used different getmntentry() according
>  to the used unmount() method, like in this patch:

I think umount(8) first gets a list of all mounted file systems and
then uses that list to resolve a mountpoint or device node into a
a struct statfs. When unmounting all file systems, it needs to
ignore any file systems that it has already unmounted, or it might
attempt to unmount the same file system twice. If the unmount call
fails, it should still do the REMOVE operation so that it will at
least attempt an unmount on each file system.

You're right that this will not work correctly with zeroed file
system IDs (it worked before Kirk's commit last week, but wasn't
supposed to). In practice can it ever make things worse than the
uniqueness problems caused by non-root users not having no file
system ID? I can't think of any examples offhand.

>* /usr/src/sbin/mount/mount.c: If user uses mount -v, it prints false
>  zeroed fsids - isn't it better to print just non-zero fsids, so that
>  nobody is "mystified"? I have created two patches - I do not know
>  which do you consider as a better:

Yes, I guess now that getfsstat(2) also zeros the IDs for non-root,
there isn't much point in printing them.

Ian