Date: Tue, 26 Jul 2011 06:11:39 GMT From: Ilya Putsikau <ilya@FreeBSD.org> To: Perforce Change Reviews <perforce@FreeBSD.org> Subject: PERFORCE change 196727 for review Message-ID: <201107260611.p6Q6BdxD043955@skunkworks.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://p4web.freebsd.org/@@196727?ac=10 Change 196727 by ilya@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)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201107260611.p6Q6BdxD043955>