Differential D19690 Diff 55387 /usr/src/sys/kern/vfs_mount.c

Changeset View

Standalone View

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

Context not available.
	#include <sys/systm.h>	#include <sys/systm.h>
	#include <sys/vnode.h>	#include <sys/vnode.h>
	#include <vm/uma.h>	#include <vm/uma.h>
		#include <sys/bus.h>

	#include <geom/geom.h>	#include <geom/geom.h>

Context not available.
	struct mount *mp;	struct mount *mp;
	struct vnode *newdp;	struct vnode *newdp;
	int error, error1;	int error, error1;
		char buf[MNAMELEN + MFSNAMELEN + 256];
		mjgUnsubmitted Not Done Inline Actions 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. mjg: This is 1296 bytes on the stack which is a non-starter for the kernel. Also 256 is a rather…
		rozhuk.im-gmail.comAuthorUnsubmitted Done Inline Actions Is 256->16 ok? rozhuk.im-gmail.com: Is 256->16 ok?
		mjgUnsubmitted Not Done Inline Actions 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. mjg: Note the most important problem is that the buffer comes from the stack and it is very big.

	ASSERT_VOP_ELOCKED(vp, __func__);	ASSERT_VOP_ELOCKED(vp, __func__);
	KASSERT((fsflags & MNT_UPDATE) == 0, ("MNT_UPDATE shouldn't be here"));	KASSERT((fsflags & MNT_UPDATE) == 0, ("MNT_UPDATE shouldn't be here"));
Context not available.
	vn_lock(newdp, LK_EXCLUSIVE \| LK_RETRY);	vn_lock(newdp, LK_EXCLUSIVE \| LK_RETRY);
	VOP_UNLOCK(vp, 0);	VOP_UNLOCK(vp, 0);
	EVENTHANDLER_DIRECT_INVOKE(vfs_mounted, mp, newdp, td);	EVENTHANDLER_DIRECT_INVOKE(vfs_mounted, mp, newdp, td);
		snprintf(buf, sizeof(buf), "fs=%s path=\"%s\"",
		mjgUnsubmitted Not Done Inline Actions 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. mjg: This is done too early - should be moved past unlocking the vnode one line below. Why the…
		rozhuk.im-gmail.comAuthorUnsubmitted Done Inline Actions 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? rozhuk.im-gmail.com: I place core near to EVENTHANDLER_DIRECT_INVOKE(), to not break something. I dont know where to…
		mjgUnsubmitted Not Done Inline Actions 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. mjg: EVENTHANDLER code passes the vnode as an argument and it keeps it locked. Whether that's…
		impUnsubmitted Not Done Inline Actions 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: devctl is supposed to filter things properly for the protocols involved. But you need to use…
		mp->mnt_stat.f_fstypename, mp->mnt_stat.f_mntonname);
		devctl_notify("kernel", "vfs", "mount", buf);
	VOP_UNLOCK(newdp, 0);	VOP_UNLOCK(newdp, 0);
	mountcheckdirs(vp, newdp);	mountcheckdirs(vp, newdp);
	vrele(newdp);	vrele(newdp);
Context not available.
	int error;	int error;
	uint64_t async_flag;	uint64_t async_flag;
	int mnt_gen_r;	int mnt_gen_r;
		char buf[MNAMELEN + MFSNAMELEN + 256];

	if ((coveredvp = mp->mnt_vnodecovered) != NULL) {	if ((coveredvp = mp->mnt_vnodecovered) != NULL) {
	mnt_gen_r = mp->mnt_gen;	mnt_gen_r = mp->mnt_gen;
Context not available.
	TAILQ_REMOVE(&mountlist, mp, mnt_list);	TAILQ_REMOVE(&mountlist, mp, mnt_list);
	mtx_unlock(&mountlist_mtx);	mtx_unlock(&mountlist_mtx);
	EVENTHANDLER_DIRECT_INVOKE(vfs_unmounted, mp, td);	EVENTHANDLER_DIRECT_INVOKE(vfs_unmounted, mp, td);
		snprintf(buf, sizeof(buf), "fs=%s path=\"%s\"",
		impUnsubmitted Not Done Inline Actions should likely do the sb stuff here too imp: should likely do the sb stuff here too
		mp->mnt_stat.f_fstypename, mp->mnt_stat.f_mntonname);
		devctl_notify("kernel", "vfs", "unmount", buf);
	if (coveredvp != NULL) {	if (coveredvp != NULL) {
	coveredvp->v_mountedhere = NULL;	coveredvp->v_mountedhere = NULL;
	VOP_UNLOCK(coveredvp, 0);	VOP_UNLOCK(coveredvp, 0);
Context not available.