Page MenuHomeFreeBSD

Add mount options to prevent covering
ClosedPublic

Authored by sef on Aug 28 2019, 8:06 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision is now accepted and ready to land.Aug 29 2019, 7:57 AM
sef updated this revision to Diff 61460.Aug 29 2019, 5:07 PM

Update the man page dates per reminder.

This revision now requires review to proceed.Aug 29 2019, 5:07 PM
sef updated this revision to Diff 61519.Sep 1 2019, 3:00 AM

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

allanjude accepted this revision.Sep 6 2019, 6:30 PM

Reviewed By: allanjude

sys/kern/vfs_mount.c
715 ↗(On Diff #61519)

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

1153 ↗(On Diff #61519)

Likely you meant to clean this up

This revision is now accepted and ready to land.Sep 6 2019, 6:30 PM
sef marked 2 inline comments as done.Sep 6 2019, 6:36 PM
sef added inline comments.
sys/kern/vfs_mount.c
715 ↗(On Diff #61519)

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.

1153 ↗(On Diff #61519)

Yes, done.

sef updated this revision to Diff 61753.Sep 6 2019, 6:37 PM
sef marked 2 inline comments as done.

Feedback from Alan.

This revision now requires review to proceed.Sep 6 2019, 6:37 PM
sef added a comment.Sep 11 2019, 11:15 PM

Ping, anyone?

kib added a subscriber: kib.Sep 13 2019, 6:24 PM
kib added inline comments.
sys/kern/vfs_mount.c
868 ↗(On Diff #61753)

error != 0

1210 ↗(On Diff #61753)

!= 0

sys/kern/vfs_subr.c
5363 ↗(On Diff #61753)

style(9): no initialization in declaration.

5365 ↗(On Diff #61753)

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

5366 ↗(On Diff #61753)

Style: void * (space before start).

5379 ↗(On Diff #61753)

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.

5386 ↗(On Diff #61753)

`if (error != 0)

break;

`
then unindent the rest of the loop body.

5390 ↗(On Diff #61753)

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

5393 ↗(On Diff #61753)

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

5400 ↗(On Diff #61753)

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

5412 ↗(On Diff #61753)

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

5419 ↗(On Diff #61753)

This line should not be committed.

5426 ↗(On Diff #61753)

The check is not needed.

sys/sys/vnode.h
926 ↗(On Diff #61753)

No need for the blank line.

sef marked 10 inline comments as done.Sep 13 2019, 7:16 PM

Updated diff coming in a few seconds.

sys/kern/vfs_mount.c
1210 ↗(On Diff #61753)

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
5363 ↗(On Diff #61753)

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.

5365 ↗(On Diff #61753)

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

5379 ↗(On Diff #61753)

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

5390 ↗(On Diff #61753)

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

5393 ↗(On Diff #61753)

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.

5400 ↗(On Diff #61753)

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.

5412 ↗(On Diff #61753)

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

5426 ↗(On Diff #61753)

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

sys/sys/vnode.h
926 ↗(On Diff #61753)

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 updated this revision to Diff 62058.Sep 13 2019, 7:16 PM
sef marked 6 inline comments as done.

Feedback from kib.

kib added inline comments.Sep 13 2019, 7:48 PM
sys/kern/vfs_mount.c
1213 ↗(On Diff #62058)

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

sys/kern/vfs_subr.c
5358 ↗(On Diff #62058)

This var is not very useful.

5362 ↗(On Diff #62058)

Put eof declaration together with error. -1 line.

5369 ↗(On Diff #62058)

endp = dirent + 1; ?

5375 ↗(On Diff #62058)

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

5379 ↗(On Diff #62058)

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

5380 ↗(On Diff #62058)

Extra blank line.

5387 ↗(On Diff #62058)

Extra ().

5379 ↗(On Diff #61753)

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

sef marked 7 inline comments as done.Sep 13 2019, 11:35 PM

New update coming in.

sys/kern/vfs_subr.c
5358 ↗(On Diff #62058)

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

5369 ↗(On Diff #62058)

... 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.

5379 ↗(On Diff #62058)

Again, holdover from before I added the td argument.

sef updated this revision to Diff 62077.Sep 13 2019, 11:36 PM
sef marked 2 inline comments as done.

More changes due to feedback from kib. Thanks!

kib accepted this revision.Sep 14 2019, 8:59 AM
kib added inline comments.
sys/kern/vfs_subr.c
5353 ↗(On Diff #62077)

Remove the empty line.

5355 ↗(On Diff #62077)

I still suggest to remove the td argument.

This revision is now accepted and ready to land.Sep 14 2019, 8:59 AM
sef marked an inline comment as done.Sep 14 2019, 5:51 PM
sef added inline comments.
sys/kern/vfs_subr.c
5355 ↗(On Diff #62077)

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 updated this revision to Diff 62100.Sep 14 2019, 5:51 PM

Small change from kib feedback.

This revision now requires review to proceed.Sep 14 2019, 5:51 PM
kib added inline comments.Sep 14 2019, 7:37 PM
sys/kern/vfs_subr.c
5355 ↗(On Diff #62077)

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
926 ↗(On Diff #61753)

So now there is one useless blank line more.

sef added inline comments.Sep 14 2019, 7:40 PM
sys/kern/vfs_subr.c
5355 ↗(On Diff #62077)

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 added inline comments.Sep 14 2019, 8:06 PM
sys/kern/vfs_subr.c
5355 ↗(On Diff #62077)

Risky in which way ?

sef added inline comments.Sep 14 2019, 8:10 PM
sys/kern/vfs_subr.c
5355 ↗(On Diff #62077)

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 updated this revision to Diff 62134.Sep 15 2019, 9:01 PM

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

sef added a comment.Sep 21 2019, 6:19 PM

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 added a comment.Sep 21 2019, 6:37 PM
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 added a comment.Sep 21 2019, 6:40 PM

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 updated this revision to Diff 62394.Sep 21 2019, 6:42 PM

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

kib accepted this revision.Sep 21 2019, 6:53 PM
kib added inline comments.
sys/kern/vfs_subr.c
5355 ↗(On Diff #62077)

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.

This revision is now accepted and ready to land.Sep 21 2019, 6:53 PM