Add mount options to prevent covering

[sef]

Summary

I ran into a situation where a ZFS system replicated to a second system, and then to a third system, resulted in all of the boot environments being mounted on /mnt at the same time. This was not what I had desired. I discussed with Alan Jude to make sure I didn't miss anything (it still happens), and we discussed a couple of attacks. That leads to this proposed change, which adds to mount options, neither of which apply to MNT_UPDATE:

• nocover -- fail with EBUSY if the requested mount point is already the root of a mount point.
• emptydir -- fail with ENOTEMPTY if the requested mount point directory is not empty.

(As a note: I first implemented the latter using a new VOP, but while that would have been faster with ZFS, it would have been about the same with UFS.)

The next step in this would be to add support for ZFS to have one or more of these as a default, but that'd be done via changes to ZFS.

[sef]

Update the man page dates per reminder.

[sef]

Add support for having "cover" and "noemptydir" to the options parsing.

[sef]

Ping?

[allanjude]

Reviewed By: allanjude

sys/kern/vfs_mount.c

757–769

Is there a reason you didn't move this vfs_freeopt() call to do_freeopt=1?

1199

Likely you meant to clean this up

[sef]

sys/kern/vfs_mount.c

757–769

Because it wasn't a line I changed and it ended up completely invisible to me as a result :). I just verified it was the only one, and have fixed it.

1199

Yes, done.

[sef]

Feedback from Alan.

[sef]

Ping, anyone?

[kib]

sys/kern/vfs_mount.c

910

error != 0

1256

!= 0

sys/kern/vfs_subr.c

5554

style(9): no initialization in declaration.

5556

It is undesirable to call malloc() under a vnode lock, because pagedaemon might need the vnode lock to clean pages.

5557

Style: void * (space before start).

5570

This is a wrong test, VOP_ISLOCKED() might return LK_EXCLOTHER or LK_SHARED. But it does not matter much because vnode is asserted exclusively locked at line 863. So this block must be removed.

5577

`if (error != 0)

break;

`
then unindent the rest of the loop body.

5581

I do not see why do you need new line there.

5584

Why whiteout is not qualified for non-emptiness ? I do not argue, but want to understand the cause of the decision.

5591

If you initialize error to zero before starting the loop, wouldn't bare 'break' work there ? You can remove 'done' label and gotos then.

5603

I would list all three conditions in one if separated by '||', this saves ten lines.

5610

This line should not be committed.

5617

The check is not needed.

sys/sys/vnode.h

936

No need for the blank line.

[sef]

Updated diff coming in a few seconds.

sys/kern/vfs_mount.c

1256

I'll change them, but I'll also point out there are places in the code that do NOT do that. See sys_mount for an example of both.

sys/kern/vfs_subr.c

5554

style(9) says, "Be careful to not obfuscate the code" and "Use this feature only thoughtfully." Again, there are already instances in the file of that -- sysctl_ftry-reclaim_vnode, sched_sync, speedup_syncer. I don't see how it obfuscates the code; it makes it clear those are initial values.

5556

I based the code on other uses of readdir in the kernel (blech). I'm open to suggestions there!

5570

But that's a different function, and I can anticipate other callers. I've changed it, but I feel concern.

5581

Indentation and 80 columns. No longer indented an extra layer, so I joined a couple of the lines together, and it looks better.

5584

A white-out entry isn't *there*, at that level anyway, so it looks like an empty directory. I wasn't entirely sure about that, but that was the logic.

5591

No, it's breaking out of two levels. Hm. Now, I *could* change it to do a "if error != 0 break" after the for loop, and I think that does what I want. (Or, as I just changed it, change the while loop to have eof ==0 and error == 0.)

I _think_ I had the "error = 0;" statement because of how I was thinking about flow while writing the code, but I don't think I need it -- if the VOP_READDIR gets an error, then it can propagate up, and I think that's the correct behaviour. Please feel free to check my logic.

5603

Same format is in UFS dirempty function. (Although it doesn't do whiteout checks there, because it's checking due to rmdir.)

5617

Holdover from xnu days where malloc with M_WAITOK could still return NULL.

sys/sys/vnode.h

936

There are other blank lines right around it. I did not see any particular grouping, so I put my addition in its own grouping.

[sef]

Feedback from kib.

[kib]

sys/kern/vfs_mount.c

1259

return (EBUSY); Assigning to error is not useful.

sys/kern/vfs_subr.c

5549

This var is not very useful.

5553

Put eof declaration together with error. -1 line.

5560

endp = dirent + 1; ?

5566

Style only allows C comments /* */. That said, the comment above is rather non-informative.

5570

Then add ASSERT_VOP_LOCKED(). It is redundant because pre-condition for VOP_READDIR() asserts that anyway.

5570

Why not td ? But I strongly suggest to remove the td arg at all, it is pointless.

5571

Extra blank line.

5578

Extra ().

[sef]

New update coming in.

sys/kern/vfs_subr.c

5549

Holdover -- I realized I could add the td argument to it, and forgot to get rid of all the vestiges from before that.

5560

... duh. That is so much clearer.

Except, it's not needed at all, since I set endp in the for loop below before checking it.

5570

Again, holdover from before I added the td argument.

[sef]

More changes due to feedback from kib. Thanks!

[kib]

sys/kern/vfs_subr.c

5544

Remove the empty line.

5546

I still suggest to remove the td argument.

[sef]

sys/kern/vfs_subr.c

5546

But you still haven't said why. It's used -- by the uio, which admittedly may not use it, and by VOP_READDIR(), which may (at least for the thread credential).

[sef]

Small change from kib feedback.

[kib]

sys/kern/vfs_subr.c

5546

Because there is high desire to drop trivial td arguments.

The reason why td was used and passed around is long time gone. Now the code makes an impression that it can work with borrowed context of arbitrary thread, but it cannot. More, the special cases where it should be able to work with td != curthread is very hard to identify.

For that reasons, td is slowly removed from kernel, and new functions should not take the arg unless there is some reason.

sys/sys/vnode.h

936

So now there is one useless blank line more.

[sef]

sys/kern/vfs_subr.c

5546

Hm, ok. So I should just use curthread and curthread's cred? That strikes me as risky, but then again, I'm not sure where it wouldn't be valid, correct?

[kib]

sys/kern/vfs_subr.c

5546

Risky in which way ?

[sef]

sys/kern/vfs_subr.c

5546

Inherent distrust of global state. As I said, I am not sure where that global state *wouldn't* be valid. (That is, I was trying to follow my logic through, and didn't see a case where relying on curthread was unsafe. So my inherent distrust is wrong, correct?)

[sef]

I forgot to add the assertion kib suggested. (NB, still doesn't have the td changes we discussed.)

[sef]

Any further comments on this? I'd gotten an OOB message from Josh about this. If not, I'll make sure it's still functional and plan on committing this weekend or Monday.

[kib]

In D21458#474367, @sef wrote:

Any further comments on this? I'd gotten an OOB message from Josh about this. If not, I'll make sure it's still functional and plan on committing this weekend or Monday.

The td arg is still there.

[sef]

The td arg is still there.

I had asked for clarification from you, and you didn't respond; I assumed you didn't care about it enough at that point.

[sef]

Remove thread argument to vfs_emptydir. Also remove a blank line. Per kib.

[kib]

sys/kern/vfs_subr.c

5546

curthread is always valid.

Well, there are cases of early initialization and context switch where curthread handling is delicate, but it is definitely not in any code that touches vnodes or executes in the syscall context.