PERFORCE change 196727 for review
Ilya Putsikau
ilya at FreeBSD.org
Tue Jul 26 06:11:40 UTC 2011
http://p4web.freebsd.org/@@196727?ac=10
Change 196727 by ilya at ilya_triton2011 on 2011/07/26 06:11:21
Fix fuse device and file system data free race.
Fix typo that could result in panic
Affected files ...
.. //depot/projects/soc2011/ilya_fuse/fuse_module/fuse_device.c#14 edit
.. //depot/projects/soc2011/ilya_fuse/fuse_module/fuse_ipc.c#15 edit
.. //depot/projects/soc2011/ilya_fuse/fuse_module/fuse_ipc.h#16 edit
.. //depot/projects/soc2011/ilya_fuse/fuse_module/fuse_vfsops.c#24 edit
Differences ...
==== //depot/projects/soc2011/ilya_fuse/fuse_module/fuse_device.c#14 (text+ko) ====
@@ -87,8 +87,8 @@
FUSE_LOCK();
if (fuse_get_devdata(dev)) {
+ fdata_trydestroy(fdata);
FUSE_UNLOCK();
- fdata_destroy(fdata);
goto busy;
} else {
fdata->dataflags |= FSESS_OPENED;
@@ -108,48 +108,37 @@
fuse_device_close(struct cdev *dev, int fflag, int devtype, struct thread *td)
{
struct fuse_data *data;
+ struct fuse_ticket *tick;
- FUSE_LOCK();
data = fuse_get_devdata(dev);
- if (! data)
+ if (!data)
panic("no fuse data upon fuse device close");
KASSERT(data->dataflags | FSESS_OPENED,
("fuse device is already closed upon close"));
fdata_set_dead(data);
+
+ FUSE_LOCK();
data->dataflags &= ~FSESS_OPENED;
+
fuse_lck_mtx_lock(data->aw_mtx);
-
/* wakup poll()ers */
selwakeuppri(&data->ks_rsel, PZERO + 1);
+ /* Don't let syscall handlers wait in vain */
+ while ((tick = fuse_aw_pop(data))) {
+ fuse_lck_mtx_lock(tick->tk_aw_mtx);
+ fticket_set_answered(tick);
+ tick->tk_aw_errno = ENOTCONN;
+ wakeup(tick);
+ fuse_lck_mtx_unlock(tick->tk_aw_mtx);
+ FUSE_ASSERT_AW_DONE(tick);
+ fuse_ticket_drop(tick);
+ }
+ fuse_lck_mtx_unlock(data->aw_mtx);
- DEBUG("mntco %d\n", data->mntco);
- if (data->mntco > 0) {
- struct fuse_ticket *tick;
-
- /* Don't let syscall handlers wait in vain */
- while ((tick = fuse_aw_pop(data))) {
- fuse_lck_mtx_lock(tick->tk_aw_mtx);
- fticket_set_answered(tick);
- tick->tk_aw_errno = ENOTCONN;
- wakeup(tick);
- fuse_lck_mtx_unlock(tick->tk_aw_mtx);
- fuse_lck_mtx_unlock(data->aw_mtx);
- FUSE_ASSERT_AW_DONE(tick);
- fuse_ticket_drop(tick);
- fuse_lck_mtx_lock(data->aw_mtx);
- }
- fuse_lck_mtx_unlock(data->aw_mtx);
-
- FUSE_UNLOCK();
- goto out;
- }
dev->si_drv1 = NULL;
- fuse_lck_mtx_unlock(data->aw_mtx);
+ fdata_trydestroy(data);
FUSE_UNLOCK();
- fdata_destroy(data);
-
-out:
DEBUG("%s: device closed by thread %d.\n", dev->si_name, td->td_tid);
return(0);
}
==== //depot/projects/soc2011/ilya_fuse/fuse_module/fuse_ipc.c#15 (text+ko) ====
@@ -341,14 +341,11 @@
data = malloc(sizeof(struct fuse_data), M_FUSEMSG, M_WAITOK | M_ZERO);
- data->mpri = FM_NOMOUNTED;
data->fdev = fdev;
- data->dataflags = 0;
mtx_init(&data->ms_mtx, "fuse message list mutex", NULL, MTX_DEF);
STAILQ_INIT(&data->ms_head);
mtx_init(&data->aw_mtx, "fuse answer list mutex", NULL, MTX_DEF);
TAILQ_INIT(&data->aw_head);
- data->ticketer = 0;
data->daemoncred = crhold(cred);
data->daemon_timeout = FUSE_DEFAULT_DAEMON_TIMEOUT;
@@ -360,9 +357,23 @@
}
void
-fdata_destroy(struct fuse_data *data)
+fdata_trydestroy(struct fuse_data *data)
{
- debug_printf("data=%p, destroy.mntco = %d\n", data, data->mntco);
+ DEBUG("data=%p data.mp=%p data.fdev=%p data.flags=%04x\n",
+ data, data->mp, data->fdev, data->dataflags);
+
+ if (data->mp != NULL) {
+ MPASS(data->mp->mnt_data == data);
+ return;
+ }
+
+ if (data->fdev->si_drv1 != NULL) {
+ MPASS(data->fdev->si_drv1 == data);
+ return;
+ }
+
+ DEBUG("destroy: data=%p\n", data);
+ MPASS((data->dataflags & FSESS_OPENED) == 0);
/* Driving off stage all that stuff thrown at device... */
mtx_destroy(&data->ms_mtx);
@@ -381,19 +392,18 @@
{
debug_printf("data=%p\n", data);
- fuse_lck_mtx_lock(data->ms_mtx);
+ FUSE_LOCK();
if (fdata_get_dead(data)) {
- fuse_lck_mtx_unlock(data->ms_mtx);
+ FUSE_UNLOCK();
return;
}
+ fuse_lck_mtx_lock(data->ms_mtx);
data->dataflags |= FSESS_DEAD;
wakeup_one(data);
selwakeuppri(&data->ks_rsel, PZERO + 1);
+ wakeup(&data->ticketer);
fuse_lck_mtx_unlock(data->ms_mtx);
-
- FUSE_LOCK();
- wakeup(&data->ticketer);
FUSE_UNLOCK();
}
==== //depot/projects/soc2011/ilya_fuse/fuse_module/fuse_ipc.h#16 (text+ko) ====
@@ -127,8 +127,6 @@
struct cdev *fdev;
struct mount *mp;
struct vnode *vroot;
- enum mountpri mpri;
- int mntco;
struct ucred *daemoncred;
int dataflags;
@@ -179,15 +177,14 @@
struct fuse_data *
fuse_get_devdata(struct cdev *fdev)
{
- return (fdev->si_drv1);
+ return fdev->si_drv1;
}
static __inline__
struct fuse_data *
fuse_get_mpdata(struct mount *mp)
{
- struct fuse_data *data = mp->mnt_data;
- return (data->mpri == FM_PRIMARY ? data : NULL);
+ return mp->mnt_data;
}
static __inline__
@@ -251,7 +248,7 @@
{
struct fuse_ticket *ftick = NULL;
- mtx_assert(&ftick->tk_data->aw_mtx, MA_OWNED);
+ mtx_assert(&data->aw_mtx, MA_OWNED);
if ((ftick = TAILQ_FIRST(&data->aw_head))) {
fuse_aw_remove(ftick);
@@ -276,7 +273,7 @@
}
struct fuse_data *fdata_alloc(struct cdev *dev, struct ucred *cred);
-void fdata_destroy(struct fuse_data *data);
+void fdata_trydestroy(struct fuse_data *data);
void fdata_set_dead(struct fuse_data *data);
static __inline__
==== //depot/projects/soc2011/ilya_fuse/fuse_module/fuse_vfsops.c#24 (text+ko) ====
@@ -72,58 +72,18 @@
MALLOC_DEFINE(M_FUSEVFS, "fuse_filesystem", "buffer for fuse vfs layer");
-#define FUSE_FLAGOPT(fnam, fval) do { \
- vfs_flagopt(opts, #fnam, &mntopts, fval); \
- vfs_flagopt(opts, "__" #fnam, &__mntopts, fval); \
-} while (0)
-
static int
-fuse_vfsop_mount(struct mount *mp)
+fuse_getdevice(const char *fspec, struct thread *td, struct cdev **fdevp)
{
- int err = 0;
- int mntopts = 0;
- int __mntopts = 0;
- int max_read_set = 0;
- uint32_t max_read = ~0;
- int daemon_timeout;
-
- size_t len;
-
+ struct nameidata nd, *ndp = &nd;
+ struct vnode *devvp;
struct cdev *fdev;
- struct fuse_data *data;
- struct thread *td = curthread;
- char *fspec, *subtype = NULL;
- struct vnode *devvp;
- struct vfsoptlist *opts;
- struct nameidata nd, *ndp = &nd;
-
- fuse_trace_printf_vfsop();
-
- if (mp->mnt_flag & MNT_UPDATE)
- return EOPNOTSUPP;
-
- mp->mnt_flag |= MNT_SYNCHRONOUS;
- /* Get the new options passed to mount */
- opts = mp->mnt_optnew;
-
- if (!opts)
- return EINVAL;
-
- /* `fspath' contains the mount point (eg. /mnt/fuse/sshfs); REQUIRED */
- if (!vfs_getopts(opts, "fspath", &err))
- return err;
-
- /* `from' contains the device name (eg. /dev/fuse0); REQUIRED */
- fspec = vfs_getopts(opts, "from", &err);
- if (!fspec)
- return err;
-
- mp->mnt_data = NULL;
+ int err;
/*
* Not an update, or updating the name: look up the name
* and verify that it refers to a sensible disk device.
- */
+ */
NDINIT(ndp, LOOKUP, FOLLOW, UIO_SYSSPACE, fspec, td);
if ((err = namei(ndp)) != 0)
@@ -155,11 +115,12 @@
*/
#ifdef MAC
err = mac_check_vnode_open(td->td_ucred, devvp, VREAD|VWRITE);
- if (! err)
+ if (!err)
#endif
err = VOP_ACCESS(devvp, VREAD|VWRITE, td->td_ucred, td);
if (err) {
vrele(devvp);
+ dev_rel(fdev);
return err;
}
}
@@ -170,25 +131,67 @@
*/
vrele(devvp);
- FUSE_LOCK();
if (!fdev->si_devsw ||
strcmp("fuse", fdev->si_devsw->d_name)) {
- FUSE_UNLOCK();
+ dev_rel(fdev);
return ENXIO;
}
- data = fuse_get_devdata(fdev);
- if (data && data->dataflags & FSESS_OPENED) {
- data->mntco++;
- debug_printf("a.inc:mntco = %d\n", data->mntco);
- } else {
- FUSE_UNLOCK();
- dev_rel(fdev);
- return ENXIO;
- }
- FUSE_UNLOCK();
+ *fdevp = fdev;
+
+ return 0;
+}
+
+#define FUSE_FLAGOPT(fnam, fval) do { \
+ vfs_flagopt(opts, #fnam, &mntopts, fval); \
+ vfs_flagopt(opts, "__" #fnam, &__mntopts, fval); \
+} while (0)
+
+static int
+fuse_vfsop_mount(struct mount *mp)
+{
+ int err = 0;
+ int mntopts = 0;
+ int __mntopts = 0;
+ int max_read_set = 0;
+ uint32_t max_read = ~0;
+ int daemon_timeout;
+
+ size_t len;
+
+ struct cdev *fdev;
+ struct fuse_data *data;
+ struct thread *td = curthread;
+ char *fspec, *subtype = NULL;
+ struct vfsoptlist *opts;
+
+ fuse_trace_printf_vfsop();
+
+ if (mp->mnt_flag & MNT_UPDATE)
+ return EOPNOTSUPP;
+
+ mp->mnt_flag |= MNT_SYNCHRONOUS;
+ mp->mnt_data = NULL;
+ /* Get the new options passed to mount */
+ opts = mp->mnt_optnew;
+
+ if (!opts)
+ return EINVAL;
+
+ /* `fspath' contains the mount point (eg. /mnt/fuse/sshfs); REQUIRED */
+ if (!vfs_getopts(opts, "fspath", &err))
+ return err;
+
+ /* `from' contains the device name (eg. /dev/fuse0); REQUIRED */
+ fspec = vfs_getopts(opts, "from", &err);
+ if (!fspec)
+ return err;
+
+ err = fuse_getdevice(fspec, td, &fdev);
+ if (err != 0)
+ return err;
- /*
+ /*
* With the help of underscored options the mount program
* can inform us from the flags it sets by default
*/
@@ -215,40 +218,52 @@
subtype = vfs_getopts(opts, "subtype=", &err);
err = 0;
- if (fdata_get_dead(data))
- err = ENOTCONN;
- if (mntopts & FSESS_DAEMON_CAN_SPY)
- err = priv_check(td, PRIV_VFS_FUSE_ALLOWOTHER);
-
DEBUG2G("mntopts 0x%x\n", mntopts);
- /* Sanity + permission checks */
+ FUSE_LOCK();
+ data = fuse_get_devdata(fdev);
+ if (data == NULL || data->mp != NULL ||
+ (data->dataflags & FSESS_OPENED) == 0) {
+ DEBUG("invalid or not opened device: data=%p data.mp=%p\n",
+ data, data != NULL ? data->mp : NULL);
+ err = ENXIO;
+ FUSE_UNLOCK();
+ goto out;
+ } else {
+ DEBUG("set mp: data=%p mp=%p\n", data, mp);
+ data->mp = mp;
+ }
+ if (fdata_get_dead(data)) {
+ DEBUG("device is dead during mount: data=%p\n", data);
+ err = ENOTCONN;
+ FUSE_UNLOCK();
+ goto out;
+ }
+ /* Sanity + permission checks */
if (!data->daemoncred)
panic("fuse daemon found, but identity unknown");
-
- MPASS(data->mpri != FM_PRIMARY);
- if (td->td_ucred->cr_uid != data->daemoncred->cr_uid)
+ if (mntopts & FSESS_DAEMON_CAN_SPY)
+ err = priv_check(td, PRIV_VFS_FUSE_ALLOWOTHER);
+ if (err == 0 && td->td_ucred->cr_uid != data->daemoncred->cr_uid)
/* are we allowed to do the first mount? */
err = priv_check(td, PRIV_VFS_FUSE_MOUNT_NONUSER);
-
if (err) {
+ FUSE_UNLOCK();
goto out;
}
/* We need this here as this slot is used by getnewvnode() */
mp->mnt_stat.f_iosize = PAGE_SIZE;
-
mp->mnt_data = data;
-
data->mp = mp;
- data->mpri = FM_PRIMARY;
data->dataflags |= mntopts;
data->max_read = max_read;
#ifdef XXXIP
if (!priv_check(td, PRIV_VFS_FUSE_SYNC_UNMOUNT))
data->dataflags |= FSESS_CAN_SYNC_UNMOUNT;
#endif
+ FUSE_UNLOCK();
vfs_getnewfsid(mp);
mp->mnt_flag |= MNT_LOCAL;
@@ -266,11 +281,13 @@
out:
if (err) {
- data->mntco--;
FUSE_LOCK();
- if (data->mntco == 0 && ! (data->dataflags & FSESS_OPENED)) {
- fdev->si_drv1 = NULL;
- fdata_destroy(data);
+ if (data->mp == mp) {
+ /* Destroy device only if we acquired reference to it */
+ DEBUG("mount failed, destroy device: data=%p mp=%p err=%d\n",
+ data, mp, err);
+ data->mp = NULL;
+ fdata_trydestroy(data);
}
FUSE_UNLOCK();
dev_rel(fdev);
@@ -328,15 +345,10 @@
fdata_set_dead(data);
alreadydead:
- data->mpri = FM_NOMOUNTED;
- data->mntco--;
-
FUSE_LOCK();
+ data->mp = NULL;
fdev = data->fdev;
- if (data->mntco == 0 && !(data->dataflags & FSESS_OPENED)) {
- data->fdev->si_drv1 = NULL;
- fdata_destroy(data);
- }
+ fdata_trydestroy(data);
FUSE_UNLOCK();
MNT_ILOCK(mp);
@@ -347,7 +359,7 @@
dev_rel(fdev);
return 0;
-}
+}
static int
fuse_vfsop_root(struct mount *mp, int lkflags, struct vnode **vpp)
More information about the p4-projects
mailing list