Page MenuHomeFreeBSD

mount/unmount events to devd
Needs ReviewPublic

Authored by rozhuk.im-gmail.com on Mar 23 2019, 4:09 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Send events to devd on mount new file system or unmount existing.

Test Plan
  1. Apply patch, rebuild and install kernell, reboot.
  2. run: cat /var/run/devd.pipe
  3. mount some fs, unmount some fs
  1. mount_msdosfs /dev/ada0p3 /media
  2. umount /media

!system=kernel subsystem=vfs type=mount fs=msdosfs path="/media"
!system=kernel subsystem=vfs type=unmount fs=msdosfs path="/media"

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

rozhuk.im-gmail.com retitled this revision from mount/unmount event to devd to mount/unmount events to devd.Mar 23 2019, 4:11 PM
mjg added a subscriber: mjg.Mar 25 2019, 12:22 PM

What's the motivation for this functionality?

/usr/src/sys/kern/vfs_mount.c
842

This is 1296 bytes on the stack which is a non-starter for the kernel. Also 256 is a rather arbitrary number, should be based on the format string instead.

946

This is done too early - should be moved past unlocking the vnode one line below.

Why the device is not added?

There is a serious problem with the format: valid pathnames can contain ", spaces, tabs, newlines and other nasty chars which interfere. in general devctl, as it is, is not very well suited for this data

Note that in certain cases jailed root can mount filesystems and unprivileged users can do the same. This means that a nasty jailed root can try to feed devctl maliciously formatted data to cause trouble.

Same for the unmount case.

This is alternate to kqueue EVFILT_FS, but there is no need to keep prev state from getmntinfo() and compare with new to find changes.
Also user can use shell scripts to perform additional actions.

/usr/src/sys/kern/vfs_mount.c
842

Is 256->16 ok?

946

I place core near to EVENTHANDLER_DIRECT_INVOKE(), to not break something.
I dont know where to get device.
What can I do with nasty chars? urlencode?

mjg added inline comments.Mar 27 2019, 11:53 PM
/usr/src/sys/kern/vfs_mount.c
842

Note the most important problem is that the buffer comes from the stack and it is very big. This should be malloc'ed instead.

256 -> 16 does not address the other problem - sizing is completely detached from how much memory is going to be needed. 256 is more than necessary for the current format string, but may become too small at some point. This is just error prone.

946

EVENTHANDLER code passes the vnode as an argument and it keeps it locked. Whether that's necessary or not is another question.

Neither snprintf nor devctl_notify deal with the vnode thus there is no reason to call them with it locked. In general you don't want to do stuff with a lock held if you can avoid it (sometimes performance and other times correctness reasons).

The biggest problem though is that devctl is just not prepared to handle user-supplied data. I don't have a definitive answer how to remedy this problem, but it's definitely a lot of work and it is highly unlikely to be worth the effort for this particular purpose. You did not provide an actual usecase for this functionality.

imp added inline comments.Apr 2 2019, 10:01 PM
/usr/src/sys/kern/vfs_mount.c
946

devctl is supposed to filter things properly for the protocols involved.
But you need to use devctl_safe_quote_sb to make that happen, which you'd use in conjunction with making the string for buf, which requires a statbuf, which is arguably better than using a stack variable anyway...

imp added inline comments.Apr 2 2019, 10:06 PM
/usr/src/sys/kern/vfs_mount.c
1484

should likely do the sb stuff here too

trasz added a subscriber: trasz.Apr 10 2019, 2:48 PM

Please consider adding it to devd.conf(5) man page.