kern/coredump_vnode.c: avoid dumping to the mount point we suspended

[kib]

Summary

PR:     299095
Reported by:    Rick Richard <[e-mail]>

[rick_sloservers.com]

growfs still hangs with this. When there's no core file yet, it blocks
creating it, before the new check:

_sleep vn_start_write vn_open_cred core_vn_extend sigexit

With an existing core file the check returns EBUSY, but then vn_close()
blocks:

_sleep vn_start_write vn_close core_vn_extend sigexit

I think the dump has to be skipped before the open.

[kib]

Push suspend check into vn_open_cred().

[rick_sloservers.com]

Half-way there...

With an existing core file this works: growfs exits on the signal and the filesystem stays writable.

Without one, however, it panics in the create path:

panic: Fatal page fault at vn_open_cred+0x218: 0xa959a8662ff03acc
vn_open_cred() at vn_open_cred+0x218
core_vn_extend() at core_vn_extend+0x716

vp isn't set yet when vn_open_nosuspend() is called there. I think it should check ndp->ni_dvp instead.

[kib]

Use dvp for nosuspend check.

[rick_sloservers.com]

That fixed it. growfs now just dies from the signal whether or not a core file is already there, and the filesystem stays usable. Thanks!

[kib]

There is also D60285 somewhat related to this.

[rick_sloservers.com]

I have a question about vn_open_nosuspend.

It reads mnt_kern_flag and mnt_susp_owner without MNT_ILOCK, is it possible for a core dump to start and see (MNTK_SUSPEND | MNTK_SUSPENDED) before mnt_susp_owner is set (a possible but unlikely NULL ptr dereference here)?

If a core dump and a fs snapshot start at the same time, for example?

[kib]

In D60283#1383798, @rick_sloservers.com wrote:

I have a question about vn_open_nosuspend.

It reads mnt_kern_flag and mnt_susp_owner without MNT_ILOCK, is it possible for a core dump to start and see (MNTK_SUSPEND | MNTK_SUSPENDED) before mnt_susp_owner is set (a possible but unlikely NULL ptr dereference here)?

If a core dump and a fs snapshot start at the same time, for example?

So yes there is a race with deref os mnt_susp_owner which must be compared against NULL before deref.
But otherwise it is safe to check for MNTK_SUSPEND* and mnt_susp_owner because the process must be single-threaded when dumping core, so other thread from the same process (checked by the condition) cannot start the suspension when we are opening core file for write.

[kib]

Ensure that mnt_susp_owner is not NULL.

[rick_sloservers.com]

Thanks!

[kib]

Add comment.

[kib]

Elaborate more about locking.

[kib]

Add assert in addition to stating the locking mode for mnt_susp_owner

[markj]

sys/kern/vfs_vnops.c

247

266

How is it possible to have v_mount == NULL? In the caller, we dereference ndp->ni_dvp->v_mount without such a check in the O_NAMEDATTR case.

[kib]

Apply even more atomicity to loads.
Drop the mp != NULL check.

[markj]

sys/kern/vfs_vnops.c

252

The name VN_OPEN_NOTSUSPENDED implies that the flag is general, but here we are explicitly assuming that the current thread is dumping core. Maybe we should just have a VN_OPEN_COREDUMP flag instead. VN_OPEN_NAMECACHE is also only used by coredump code. (And I don't see why we want to create a namecache entry for coredumps, commit 8ee9765a9d947 does not explain the reason.)

[kib]

sys/kern/vfs_vnops.c

252

The open(2) syscall does not create namecache entry on lookup/create. As I understand, it is so to not pollute the cache in situations like large tar file extraction etc.

Simultaneously, coredump with the rotating name template re-lookups the same name on each dump. To use the cache in this case, the flag was added AFAIR.

[kib]

Rename flags.

[markj]

sys/kern/vfs_vnops.c

422

We could be leaking VI_FOPENING here.

[markj]

sys/kern/vfs_vnops.c

266

Hmm, should this be using VOP_GETWRITEMOUNT? What if vp is a nullfs vnode?

[kib]

Calculate mp by VOP_GETWRITEMOUNT()
Do not leak VI_FOPENING