Re: jail getfsstat patches.

On Fri, 25 Jun 2004, Julian Elischer wrote:

> There are patches around to make 'df' and 'mount' show pretty much the
> exact right thing from a jail. 

A couple of notes based on a quick skim and prior discussions:

(1) These patches get confused by unusual contortions of the BSD VFS, as
    they rely on the strings for mounts, rather than actually determine if
    the file system is visible.  This is probably more acceptable than
    determining if the file system is actually visible in the jail, but
    should be kept in mind.  For example, consider the renaming of
    directories leading to mountpoints, and mounts legitimately created in
    chroots (but not jails).  This is a symptom of the general Problem
    With Paths in BSD (see a post of mine a while back on arch_at_ on the
    topic).

(2) This introduces the same dependency found in the mount system call
    that the user process submits a mostly useful path to compare against. 
    Same Big BSD Path Problem, and we've seen some unfortunate bugs in the
    user mount code where that assumption has problems -- for example, for
    a while it wasn't possible to unmount a file system if its mountpoint
    had been renamed since mount, as the mount command would scan the
    mount table and reject it rather than unmount by path.  This meant
    that if a low level directory was renamed, or you wanted to unmount in
    a chroot(), you were stuck.  Caution advised.

(3) There's a lot of common code added to many functions.  I'm sure that
    can be abstracted.  I.e.,

	if (jail_statfs_restructed & jailed(td->td_ucred)) {
		jail_statfs(td->td_ucred, mp);
		mtx_lock(&mountlist_mtx);
		nmp = TAILQ_NEXT(mp, mnt_list);
	}

    Likewise, the "is this path visible to this jail" is something that
    needs to be abstracted, as it will help make it clear when it needs to
    be called.  I would advise adding comments on the order of:

	/*
	 * We don't need to call jail_statfs_check() here because the
	 * process selected a file system by path, meaning that we already
	 * know it's in the process name space. 
	 */

(4) Should jail_statfs_restricted be per-jail?

(5) Is there any particular reason to mask the file system type on the
    root file system?  That's actually a somewhat useful piece of
    information to have...

(6) The style in there is somewhat wrong, and should be fixed (a lot of
    odd spacing, etc).

(7) If I might suggest, caching the suser result is OK, but I would avoid
    combining the jail statfs restricted munging of the path information
    in the same code block as the non-jail restriction on vsid exporting.
    It makes the access control logic less clear.

(8) There might well be small races with forceable unmount here in the
    presence of copyout's, etc.  These may not be a big deal.

(9) The word "hack" appears in the comments.  I wonder if we can find a
    way to remove the cause of that word, and hence the comment.  Or at
    least, find a way to make it not a hack (or revise the
    interpretation as a hack).  For example, strlen() might be used.
    Also, that use of strcpy() makes me nervous, since it's copying to and
    from the same buffer, and if the string lengths come out
    inconveniently, the two sections of the buffer may be overlapping.

(10) Could we assign constant names to the jail-statfs_restricted values?
     When I see ">=4", I have to admit it isn't clear to me why at any
     particular bit of the patch, that particular value was selected. 

(11) If we ever allow mounting in jail, this code will instantly break.

Given that the person maintaining those web pages is a FreeBSD committer
who's been working actively on Jail stuff, I suggest his opinion as to
when to merge them is most relevant. :-)

Robert N M Watson             FreeBSD Core Team, TrustedBSD Projects
robert_at_fledge.watson.org      Principal Research Scientist, McAfee Research


> 
> In both -current and 4.x
> 
> I propose to commit these.
> 
> http://garage.freebsd.pl/
> "jailfsstat - With this kernel module process in jail can only see file
> systems mounted inside."
> 
> for 4.x
> 
> and
> 
>     http://sources.zabbadoz.net/freebsd/jail.html
> for 5.x
> 
> with possible small changes..
> 
> e.g. the 4.x version would not be a module 
> but would have a sysclt to turn it on 
> (off by default)
> 
> and the 5.x version may require osme small work too..
> 
> 
> Does anyone violently object to these?
> 
> The fact that df or mount shows so much not only confuses the hell
> out of users, it makes scripts fail in odd ways.
> (and bugs the hell out of me too).
> 
> julian
> 
> 
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"
>