[saturnero@freesbie.org: Weird behaviour of mount_unionfs with executables]

[Bruce Evans]

On Mon, 4 Jul 2005, Dario Freni wrote:

> On Sun, Jul 03, 2005 at 08:43:01PM -0400, Stephan Uphoff wrote:
>> YES - and it looks like just specifying noatime for the union mount
>> should fix the copy problem for FreeSBIE.
>
> Correct. Using -o noatime when mounting the unionfs resolves all above
> problems. Thank you very much.

Hmm.  This shouldn't work, but helps avoid the copy due to a layering
violation.  In kern_execve(), the test of MNT_NOATIME is supposed to
be just an optimization to avoid making null VOP_SETATTR(), but it
only looks at the mount flag for the top layer so it doesn't work
right if the mount flag is different in lower layers.  This gives the
bizarre behaviour that the noatime option for unionsfs "works" for
exec() but not for read().

I could only see the duplication problem for mount_unionfs -b.  There
are lots of bugs in that case:
= no copy is made for the modification caused by read(), despite
   mount_unionfs(8) claiming that:

      Any other operation which would ultimately require modification to the
      ^^^
      lower layer fails with EROFS.

- the error for failure is often not EROFS (but this may be a side effect).
- exec() makes a bad copy. It fails to copy the access times and doesn't
   even copy the permissions; in particular, it loses the x bits so the
   file becomes non-executable.
- touch(1) makes a bad copy like exec() and then fails to actually touch
   the file except for the clobbered times in the non-copy being current.
   (touch tries utimes(2) twice, first with the current times and then with
   a null times pointer.  Both calls fail with EACCES.  I think a bad copy
   is made on the first call ...).  Subsequent touch(1)'s work normally
   (... touch tries utimes(2) only once, only with the current times, and
   since this works there seems to be a race for the earlier calls to fail
   (I tested as root, so there should be no permissions problems with either
   call, and since the middle utimes(path, NULL) call failed the bug can't
   be just a stale vp after making the copy in the first call).)

The comment in union_setattr() only claims to make a copy to handle
truncation, but a copy seems to be made when any attribute is set:

% static int
% union_setattr(ap)
% ...
% 	/*
% 	 * Handle case of truncating lower object to zero size
% 	 * by creating a zero length upper object.  This is to
% 	 * handle the case of open with O_TRUNC and O_CREAT.
% 	 */
% 	if (un->un_uppervp == NULLVP && (un->un_lowervp->v_type == VREG)) {
% 		error = union_copyup(un, (ap->a_vap->va_size != 0),
% 			    ap->a_cred, ap->a_td);
% 		if (error)
% 			return (error);
% 	}

Copying on any change to an attribute seems right, but it doesn't
actually handle implicit changes that don't go through setattr --
mainly setting of atimes on read().  Copying on read() wouldn't be
right.

% 
% 	/*
% 	 * Try to set attributes in upper layer,
% 	 * otherwise return read-only filesystem error.
% 	 */
% 	error = EROFS;
% 	if ((uppervp = union_lock_upper(un, td)) != NULLVP) {
% 		error = VOP_SETATTR(un->un_uppervp, ap->a_vap,
% 					ap->a_cred, ap->a_td);
% 		if ((error == 0) && (ap->a_vap->va_size != VNOVAL))
% 			union_newsize(ap->a_vp, ap->a_vap->va_size, VNOVAL);
% 		union_unlock_upper(uppervp, td);
% 	}
% 	return (error);
% }

The second clause in the comment doesn't match the code here -- the error
is very rarely EROFS since it is usually the error returned by VOP_SETATTR()
and we have avoided getting this far in most cases where VOP_SETATTR()
would return EROFS.  The comment was closer to matching the code in rev.1.1
where decideding EROFS was left to lower layers.

Bruce