Discussion:
[developer] possible bug in zfsctl_snapdir_lookup() - uninitialized snapname
Youzhong Yang via illumos-developer
2014-09-15 17:31:48 UTC
Permalink
Hi all,

When I tried to troubleshoot an issue of unable to mount a snapshot, I came
across something weird. Inside zfsctl_snapdir_lookup(), there is a jump to
'domount', in which case leaves 'snapname' uninitialized.

Is this a bug? I saw junk data passed to the 2nd arg of
zfsctl_snapdir_lookup() when dtracing it.

Thanks,
-Youzhong

http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_ctldir.c#777

zfsctl_snapdir_lookup() {
.
.
.
char snapname[MAXNAMELEN];
.
.
.
mutex_enter(&sdp->sd_lock);
search.se_name = (char *)nm;
if ((sep = avl_find(&sdp->sd_snaps, &search, &where)) != NULL) {
*vpp = sep->se_root;
VN_HOLD(*vpp);
err = traverse(vpp);
if (err != 0) {
VN_RELE(*vpp);
*vpp = NULL;
} else if (*vpp == sep->se_root) {
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
goto domount;
} else {
/*
* VROOT was set during the traverse call. We need
* to clear it since we're pretending to be part
* of our parent's vfs.
*/
(*vpp)->v_flag &= ~VROOT;
}
mutex_exit(&sdp->sd_lock);
ZFS_EXIT(zfsvfs);
return (err);
}
.
.
.
domount:
mountpoint_len = strlen(refstr_value(dvp->v_vfsp->vfs_mntpt)) +
strlen("/.zfs/snapshot/") + strlen(nm) + 1;
mountpoint = kmem_alloc(mountpoint_len, KM_SLEEP);
(void) snprintf(mountpoint, mountpoint_len, "%s/.zfs/snapshot/%s",
refstr_value(dvp->v_vfsp->vfs_mntpt), nm);

margs.spec = snapname;
margs.dir = mountpoint;
margs.flags = MS_SYSSPACE | MS_NOMNTTAB;
margs.fstype = "zfs";
margs.dataptr = NULL;
margs.datalen = 0;
margs.optptr = NULL;
margs.optlen = 0;

err = domount("zfs", &margs, *vpp, kcred, &vfsp);
kmem_free(mountpoint, mountpoint_len);
.
.
.
}



-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Matthew Ahrens via illumos-zfs
2014-09-15 21:28:11 UTC
Permalink
On Mon, Sep 15, 2014 at 10:31 AM, Youzhong Yang via illumos-developer <
Post by Youzhong Yang via illumos-developer
Hi all,
When I tried to troubleshoot an issue of unable to mount a snapshot, I
came across something weird. Inside zfsctl_snapdir_lookup(), there is a
jump to 'domount', in which case leaves 'snapname' uninitialized.
Is this a bug? I saw junk data passed to the 2nd arg of
zfsctl_snapdir_lookup() when dtracing it.
Yes, this is a bug. It was reported back in 2012 but nobody pushed the fix
Post by Youzhong Yang via illumos-developer
It looks like in zfsctl_snapdir_lookup in the case marked with the
following
Post by Youzhong Yang via illumos-developer
comment
Post by Youzhong Yang via illumos-developer
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
'snapname' variable remains uninitialized and that likely would lead
to a
Post by Youzhong Yang via illumos-developer
failure in mount.
Agreed. If you have a fix I'd be happy to review it and help get it
integrated.

How does the following look to you?

--- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
+++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
@@ -1002,6 +1002,7 @@ zfsctl_snapdir_lookup(ap)
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
+ VERIFY(zfsctl_snapshot_zname(dvp, nm, MAXNAMELEN,
snapname) == 0);
goto domount;
} else {
/*

--matt
Post by Youzhong Yang via illumos-developer
Thanks,
-Youzhong
http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_ctldir.c#777
zfsctl_snapdir_lookup() {
.
.
.
char snapname[MAXNAMELEN];
.
.
.
mutex_enter(&sdp->sd_lock);
search.se_name = (char *)nm;
if ((sep = avl_find(&sdp->sd_snaps, &search, &where)) != NULL) {
*vpp = sep->se_root;
VN_HOLD(*vpp);
err = traverse(vpp);
if (err != 0) {
VN_RELE(*vpp);
*vpp = NULL;
} else if (*vpp == sep->se_root) {
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
goto domount;
} else {
/*
* VROOT was set during the traverse call. We need
* to clear it since we're pretending to be part
* of our parent's vfs.
*/
(*vpp)->v_flag &= ~VROOT;
}
mutex_exit(&sdp->sd_lock);
ZFS_EXIT(zfsvfs);
return (err);
}
.
.
.
mountpoint_len = strlen(refstr_value(dvp->v_vfsp->vfs_mntpt)) +
strlen("/.zfs/snapshot/") + strlen(nm) + 1;
mountpoint = kmem_alloc(mountpoint_len, KM_SLEEP);
(void) snprintf(mountpoint, mountpoint_len, "%s/.zfs/snapshot/%s",
refstr_value(dvp->v_vfsp->vfs_mntpt), nm);
margs.spec = snapname;
margs.dir = mountpoint;
margs.flags = MS_SYSSPACE | MS_NOMNTTAB;
margs.fstype = "zfs";
margs.dataptr = NULL;
margs.datalen = 0;
margs.optptr = NULL;
margs.optlen = 0;
err = domount("zfs", &margs, *vpp, kcred, &vfsp);
kmem_free(mountpoint, mountpoint_len);
.
.
.
}
*illumos-developer* | Archives
<https://www.listbox.com/member/archive/182179/=now>
<https://www.listbox.com/member/archive/rss/182179/21175174-cd73734d> |
Modify
<https://www.listbox.com/member/?&>
Your Subscription <http://www.listbox.com>
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Youzhong Yang via illumos-developer
2014-09-15 22:13:54 UTC
Permalink
Thanks Matt.

Your proposed fix looks good to me.

The particular snapshot which cannot be mounted on our server has v_count 1
in its vnode_t struct, which leads zfs_mount() to return EBUSY.

I don't know how the zfs_snapentry_t->se_root->v_count got leaked, do you
happen to know similar issue(s)? In one of your e-mails you mentioned a
potential fix for bug 5160 (https://www.illumos.org/issues/5160), but I
cannot relate it to what happened in our system.

By the way, would you please share the fix for bug 5160?

Thanks again,

-Youzhong
0xffffff3e779203d0::print -at zfs_snapentry_t
ffffff3e779203d0 zfs_snapentry_t {
ffffff3e779203d0 char *se_name = 0xffffff3375c2f470
"Bmvm.148164.pass.ja1"
ffffff3e779203d8 vnode_t *se_root = 0xffffff3332794e00
ffffff3e779203e0 avl_node_t se_node = {
ffffff3e779203e0 struct avl_node *[2] avl_child = [
0xffffff4d8d834cf8, 0 ]
ffffff3e779203f0 uintptr_t avl_pcb = 0xffffff4df5951ec0
}
}
0xffffff3332794e00::print -at vnode_t
ffffff3332794e00 vnode_t {
ffffff3332794e00 kmutex_t v_lock = {
ffffff3332794e00 void *[1] _opaque = [ 0 ]
}
ffffff3332794e08 uint_t v_flag = 0x100
ffffff3332794e0c uint_t v_count = 0x1
ffffff3332794e10 void *v_data = 0xffffff34d23f6700
ffffff3332794e18 struct vfs *v_vfsp = 0xffffff32c8cfede0
ffffff3332794e20 struct stdata *v_stream = 0
ffffff3332794e28 enum vtype v_type = 2 (VDIR)
ffffff3332794e30 dev_t v_rdev = 0
ffffff3332794e38 struct vfs *v_vfsmountedhere = 0
ffffff3332794e40 struct vnodeops *v_op = 0xffffff32575cb4c0
ffffff3332794e48 struct page *v_pages = 0
ffffff3332794e50 struct filock *v_filocks = 0
ffffff3332794e58 struct shrlocklist *v_shrlocks = 0
ffffff3332794e60 krwlock_t v_nbllock = {
ffffff3332794e60 void *[1] _opaque = [ 0 ]
}
ffffff3332794e68 kcondvar_t v_cv = {
ffffff3332794e68 ushort_t _opaque = 0
}
ffffff3332794e70 void *v_locality = 0
ffffff3332794e78 struct fem_head *v_femhead = 0
ffffff3332794e80 char *v_path = 0
ffffff3332794e88 uint_t v_rdcnt = 0
ffffff3332794e8c uint_t v_wrcnt = 0
ffffff3332794e90 u_longlong_t v_mmap_read = 0
ffffff3332794e98 u_longlong_t v_mmap_write = 0
ffffff3332794ea0 void *v_mpssdata = 0
ffffff3332794ea8 void *v_fopdata = 0
ffffff3332794eb0 kmutex_t v_vsd_lock = {
ffffff3332794eb0 void *[1] _opaque = [ 0 ]
}
ffffff3332794eb8 struct vsd_node *v_vsd = 0
ffffff3332794ec0 struct vnode *v_xattrdir = 0
ffffff3332794ec8 uint_t v_count_dnlc = 0
}
On Mon, Sep 15, 2014 at 10:31 AM, Youzhong Yang via illumos-developer <
Post by Youzhong Yang via illumos-developer
Hi all,
When I tried to troubleshoot an issue of unable to mount a snapshot, I
came across something weird. Inside zfsctl_snapdir_lookup(), there is a
jump to 'domount', in which case leaves 'snapname' uninitialized.
Is this a bug? I saw junk data passed to the 2nd arg of
zfsctl_snapdir_lookup() when dtracing it.
Yes, this is a bug. It was reported back in 2012 but nobody pushed the
Post by Youzhong Yang via illumos-developer
It looks like in zfsctl_snapdir_lookup in the case marked with the
following
Post by Youzhong Yang via illumos-developer
comment
Post by Youzhong Yang via illumos-developer
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
'snapname' variable remains uninitialized and that likely would lead
to a
Post by Youzhong Yang via illumos-developer
failure in mount.
Agreed. If you have a fix I'd be happy to review it and help get it
integrated.
How does the following look to you?
--- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
+++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
@@ -1002,6 +1002,7 @@ zfsctl_snapdir_lookup(ap)
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
+ VERIFY(zfsctl_snapshot_zname(dvp, nm, MAXNAMELEN,
snapname) == 0);
goto domount;
} else {
/*
--matt
Post by Youzhong Yang via illumos-developer
Thanks,
-Youzhong
http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_ctldir.c#777
zfsctl_snapdir_lookup() {
.
.
.
char snapname[MAXNAMELEN];
.
.
.
mutex_enter(&sdp->sd_lock);
search.se_name = (char *)nm;
if ((sep = avl_find(&sdp->sd_snaps, &search, &where)) != NULL) {
*vpp = sep->se_root;
VN_HOLD(*vpp);
err = traverse(vpp);
if (err != 0) {
VN_RELE(*vpp);
*vpp = NULL;
} else if (*vpp == sep->se_root) {
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
goto domount;
} else {
/*
* VROOT was set during the traverse call. We need
* to clear it since we're pretending to be part
* of our parent's vfs.
*/
(*vpp)->v_flag &= ~VROOT;
}
mutex_exit(&sdp->sd_lock);
ZFS_EXIT(zfsvfs);
return (err);
}
.
.
.
mountpoint_len = strlen(refstr_value(dvp->v_vfsp->vfs_mntpt)) +
strlen("/.zfs/snapshot/") + strlen(nm) + 1;
mountpoint = kmem_alloc(mountpoint_len, KM_SLEEP);
(void) snprintf(mountpoint, mountpoint_len, "%s/.zfs/snapshot/%s",
refstr_value(dvp->v_vfsp->vfs_mntpt), nm);
margs.spec = snapname;
margs.dir = mountpoint;
margs.flags = MS_SYSSPACE | MS_NOMNTTAB;
margs.fstype = "zfs";
margs.dataptr = NULL;
margs.datalen = 0;
margs.optptr = NULL;
margs.optlen = 0;
err = domount("zfs", &margs, *vpp, kcred, &vfsp);
kmem_free(mountpoint, mountpoint_len);
.
.
.
}
*illumos-developer* | Archives
<https://www.listbox.com/member/archive/182179/=now>
<https://www.listbox.com/member/archive/rss/182179/21175174-cd73734d> |
Modify
<https://www.listbox.com/member/?&>
Your Subscription <http://www.listbox.com>
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Matthew Ahrens via illumos-developer
2014-09-15 22:34:30 UTC
Permalink
Post by Youzhong Yang via illumos-developer
Thanks Matt.
Your proposed fix looks good to me.
The particular snapshot which cannot be mounted on our server has v_count
1 in its vnode_t struct, which leads zfs_mount() to return EBUSY.
I don't know how the zfs_snapentry_t->se_root->v_count got leaked, do you
happen to know similar issue(s)? In one of your e-mails you mentioned a
potential fix for bug 5160 (https://www.illumos.org/issues/5160), but I
cannot relate it to what happened in our system.
By the way, would you please share the fix for bug 5160?
Yes, I'll send it out for review probably later this week.

--matt
Post by Youzhong Yang via illumos-developer
Thanks again,
-Youzhong
0xffffff3e779203d0::print -at zfs_snapentry_t
ffffff3e779203d0 zfs_snapentry_t {
ffffff3e779203d0 char *se_name = 0xffffff3375c2f470
"Bmvm.148164.pass.ja1"
ffffff3e779203d8 vnode_t *se_root = 0xffffff3332794e00
ffffff3e779203e0 avl_node_t se_node = {
ffffff3e779203e0 struct avl_node *[2] avl_child = [
0xffffff4d8d834cf8, 0 ]
ffffff3e779203f0 uintptr_t avl_pcb = 0xffffff4df5951ec0
}
}
0xffffff3332794e00::print -at vnode_t
ffffff3332794e00 vnode_t {
ffffff3332794e00 kmutex_t v_lock = {
ffffff3332794e00 void *[1] _opaque = [ 0 ]
}
ffffff3332794e08 uint_t v_flag = 0x100
ffffff3332794e0c uint_t v_count = 0x1
ffffff3332794e10 void *v_data = 0xffffff34d23f6700
ffffff3332794e18 struct vfs *v_vfsp = 0xffffff32c8cfede0
ffffff3332794e20 struct stdata *v_stream = 0
ffffff3332794e28 enum vtype v_type = 2 (VDIR)
ffffff3332794e30 dev_t v_rdev = 0
ffffff3332794e38 struct vfs *v_vfsmountedhere = 0
ffffff3332794e40 struct vnodeops *v_op = 0xffffff32575cb4c0
ffffff3332794e48 struct page *v_pages = 0
ffffff3332794e50 struct filock *v_filocks = 0
ffffff3332794e58 struct shrlocklist *v_shrlocks = 0
ffffff3332794e60 krwlock_t v_nbllock = {
ffffff3332794e60 void *[1] _opaque = [ 0 ]
}
ffffff3332794e68 kcondvar_t v_cv = {
ffffff3332794e68 ushort_t _opaque = 0
}
ffffff3332794e70 void *v_locality = 0
ffffff3332794e78 struct fem_head *v_femhead = 0
ffffff3332794e80 char *v_path = 0
ffffff3332794e88 uint_t v_rdcnt = 0
ffffff3332794e8c uint_t v_wrcnt = 0
ffffff3332794e90 u_longlong_t v_mmap_read = 0
ffffff3332794e98 u_longlong_t v_mmap_write = 0
ffffff3332794ea0 void *v_mpssdata = 0
ffffff3332794ea8 void *v_fopdata = 0
ffffff3332794eb0 kmutex_t v_vsd_lock = {
ffffff3332794eb0 void *[1] _opaque = [ 0 ]
}
ffffff3332794eb8 struct vsd_node *v_vsd = 0
ffffff3332794ec0 struct vnode *v_xattrdir = 0
ffffff3332794ec8 uint_t v_count_dnlc = 0
}
On Mon, Sep 15, 2014 at 10:31 AM, Youzhong Yang via illumos-developer <
Post by Youzhong Yang via illumos-developer
Hi all,
When I tried to troubleshoot an issue of unable to mount a snapshot, I
came across something weird. Inside zfsctl_snapdir_lookup(), there is a
jump to 'domount', in which case leaves 'snapname' uninitialized.
Is this a bug? I saw junk data passed to the 2nd arg of
zfsctl_snapdir_lookup() when dtracing it.
Yes, this is a bug. It was reported back in 2012 but nobody pushed the
Post by Youzhong Yang via illumos-developer
It looks like in zfsctl_snapdir_lookup in the case marked with the
following
Post by Youzhong Yang via illumos-developer
comment
Post by Youzhong Yang via illumos-developer
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
'snapname' variable remains uninitialized and that likely would
lead to a
Post by Youzhong Yang via illumos-developer
failure in mount.
Agreed. If you have a fix I'd be happy to review it and help get it
integrated.
How does the following look to you?
--- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
+++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
@@ -1002,6 +1002,7 @@ zfsctl_snapdir_lookup(ap)
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
+ VERIFY(zfsctl_snapshot_zname(dvp, nm,
MAXNAMELEN, snapname) == 0);
goto domount;
} else {
/*
--matt
Post by Youzhong Yang via illumos-developer
Thanks,
-Youzhong
http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_ctldir.c#777
zfsctl_snapdir_lookup() {
.
.
.
char snapname[MAXNAMELEN];
.
.
.
mutex_enter(&sdp->sd_lock);
search.se_name = (char *)nm;
if ((sep = avl_find(&sdp->sd_snaps, &search, &where)) != NULL) {
*vpp = sep->se_root;
VN_HOLD(*vpp);
err = traverse(vpp);
if (err != 0) {
VN_RELE(*vpp);
*vpp = NULL;
} else if (*vpp == sep->se_root) {
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
goto domount;
} else {
/*
* VROOT was set during the traverse call. We need
* to clear it since we're pretending to be part
* of our parent's vfs.
*/
(*vpp)->v_flag &= ~VROOT;
}
mutex_exit(&sdp->sd_lock);
ZFS_EXIT(zfsvfs);
return (err);
}
.
.
.
mountpoint_len = strlen(refstr_value(dvp->v_vfsp->vfs_mntpt)) +
strlen("/.zfs/snapshot/") + strlen(nm) + 1;
mountpoint = kmem_alloc(mountpoint_len, KM_SLEEP);
(void) snprintf(mountpoint, mountpoint_len, "%s/.zfs/snapshot/%s",
refstr_value(dvp->v_vfsp->vfs_mntpt), nm);
margs.spec = snapname;
margs.dir = mountpoint;
margs.flags = MS_SYSSPACE | MS_NOMNTTAB;
margs.fstype = "zfs";
margs.dataptr = NULL;
margs.datalen = 0;
margs.optptr = NULL;
margs.optlen = 0;
err = domount("zfs", &margs, *vpp, kcred, &vfsp);
kmem_free(mountpoint, mountpoint_len);
.
.
.
}
*illumos-developer* | Archives
<https://www.listbox.com/member/archive/182179/=now>
<https://www.listbox.com/member/archive/rss/182179/21175174-cd73734d> |
Modify
<https://www.listbox.com/member/?&>
Your Subscription <http://www.listbox.com>
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Matthew Ahrens via illumos-zfs
2014-09-15 22:35:44 UTC
Permalink
Post by Youzhong Yang via illumos-developer
Thanks Matt.
Your proposed fix looks good to me.
The fix is actually from Andriy. If you can test the fix on illumos, I'll
see about integrating it.

--matt
Post by Youzhong Yang via illumos-developer
The particular snapshot which cannot be mounted on our server has v_count
1 in its vnode_t struct, which leads zfs_mount() to return EBUSY.
I don't know how the zfs_snapentry_t->se_root->v_count got leaked, do you
happen to know similar issue(s)? In one of your e-mails you mentioned a
potential fix for bug 5160 (https://www.illumos.org/issues/5160), but I
cannot relate it to what happened in our system.
By the way, would you please share the fix for bug 5160?
Thanks again,
-Youzhong
0xffffff3e779203d0::print -at zfs_snapentry_t
ffffff3e779203d0 zfs_snapentry_t {
ffffff3e779203d0 char *se_name = 0xffffff3375c2f470
"Bmvm.148164.pass.ja1"
ffffff3e779203d8 vnode_t *se_root = 0xffffff3332794e00
ffffff3e779203e0 avl_node_t se_node = {
ffffff3e779203e0 struct avl_node *[2] avl_child = [
0xffffff4d8d834cf8, 0 ]
ffffff3e779203f0 uintptr_t avl_pcb = 0xffffff4df5951ec0
}
}
0xffffff3332794e00::print -at vnode_t
ffffff3332794e00 vnode_t {
ffffff3332794e00 kmutex_t v_lock = {
ffffff3332794e00 void *[1] _opaque = [ 0 ]
}
ffffff3332794e08 uint_t v_flag = 0x100
ffffff3332794e0c uint_t v_count = 0x1
ffffff3332794e10 void *v_data = 0xffffff34d23f6700
ffffff3332794e18 struct vfs *v_vfsp = 0xffffff32c8cfede0
ffffff3332794e20 struct stdata *v_stream = 0
ffffff3332794e28 enum vtype v_type = 2 (VDIR)
ffffff3332794e30 dev_t v_rdev = 0
ffffff3332794e38 struct vfs *v_vfsmountedhere = 0
ffffff3332794e40 struct vnodeops *v_op = 0xffffff32575cb4c0
ffffff3332794e48 struct page *v_pages = 0
ffffff3332794e50 struct filock *v_filocks = 0
ffffff3332794e58 struct shrlocklist *v_shrlocks = 0
ffffff3332794e60 krwlock_t v_nbllock = {
ffffff3332794e60 void *[1] _opaque = [ 0 ]
}
ffffff3332794e68 kcondvar_t v_cv = {
ffffff3332794e68 ushort_t _opaque = 0
}
ffffff3332794e70 void *v_locality = 0
ffffff3332794e78 struct fem_head *v_femhead = 0
ffffff3332794e80 char *v_path = 0
ffffff3332794e88 uint_t v_rdcnt = 0
ffffff3332794e8c uint_t v_wrcnt = 0
ffffff3332794e90 u_longlong_t v_mmap_read = 0
ffffff3332794e98 u_longlong_t v_mmap_write = 0
ffffff3332794ea0 void *v_mpssdata = 0
ffffff3332794ea8 void *v_fopdata = 0
ffffff3332794eb0 kmutex_t v_vsd_lock = {
ffffff3332794eb0 void *[1] _opaque = [ 0 ]
}
ffffff3332794eb8 struct vsd_node *v_vsd = 0
ffffff3332794ec0 struct vnode *v_xattrdir = 0
ffffff3332794ec8 uint_t v_count_dnlc = 0
}
On Mon, Sep 15, 2014 at 10:31 AM, Youzhong Yang via illumos-developer <
Post by Youzhong Yang via illumos-developer
Hi all,
When I tried to troubleshoot an issue of unable to mount a snapshot, I
came across something weird. Inside zfsctl_snapdir_lookup(), there is a
jump to 'domount', in which case leaves 'snapname' uninitialized.
Is this a bug? I saw junk data passed to the 2nd arg of
zfsctl_snapdir_lookup() when dtracing it.
Yes, this is a bug. It was reported back in 2012 but nobody pushed the
Post by Youzhong Yang via illumos-developer
It looks like in zfsctl_snapdir_lookup in the case marked with the
following
Post by Youzhong Yang via illumos-developer
comment
Post by Youzhong Yang via illumos-developer
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
'snapname' variable remains uninitialized and that likely would
lead to a
Post by Youzhong Yang via illumos-developer
failure in mount.
Agreed. If you have a fix I'd be happy to review it and help get it
integrated.
How does the following look to you?
--- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
+++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
@@ -1002,6 +1002,7 @@ zfsctl_snapdir_lookup(ap)
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
+ VERIFY(zfsctl_snapshot_zname(dvp, nm,
MAXNAMELEN, snapname) == 0);
goto domount;
} else {
/*
--matt
Post by Youzhong Yang via illumos-developer
Thanks,
-Youzhong
http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_ctldir.c#777
zfsctl_snapdir_lookup() {
.
.
.
char snapname[MAXNAMELEN];
.
.
.
mutex_enter(&sdp->sd_lock);
search.se_name = (char *)nm;
if ((sep = avl_find(&sdp->sd_snaps, &search, &where)) != NULL) {
*vpp = sep->se_root;
VN_HOLD(*vpp);
err = traverse(vpp);
if (err != 0) {
VN_RELE(*vpp);
*vpp = NULL;
} else if (*vpp == sep->se_root) {
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
goto domount;
} else {
/*
* VROOT was set during the traverse call. We need
* to clear it since we're pretending to be part
* of our parent's vfs.
*/
(*vpp)->v_flag &= ~VROOT;
}
mutex_exit(&sdp->sd_lock);
ZFS_EXIT(zfsvfs);
return (err);
}
.
.
.
mountpoint_len = strlen(refstr_value(dvp->v_vfsp->vfs_mntpt)) +
strlen("/.zfs/snapshot/") + strlen(nm) + 1;
mountpoint = kmem_alloc(mountpoint_len, KM_SLEEP);
(void) snprintf(mountpoint, mountpoint_len, "%s/.zfs/snapshot/%s",
refstr_value(dvp->v_vfsp->vfs_mntpt), nm);
margs.spec = snapname;
margs.dir = mountpoint;
margs.flags = MS_SYSSPACE | MS_NOMNTTAB;
margs.fstype = "zfs";
margs.dataptr = NULL;
margs.datalen = 0;
margs.optptr = NULL;
margs.optlen = 0;
err = domount("zfs", &margs, *vpp, kcred, &vfsp);
kmem_free(mountpoint, mountpoint_len);
.
.
.
}
*illumos-developer* | Archives
<https://www.listbox.com/member/archive/182179/=now>
<https://www.listbox.com/member/archive/rss/182179/21175174-cd73734d> |
Modify
<https://www.listbox.com/member/?&>
Your Subscription <http://www.listbox.com>
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Youzhong Yang via illumos-developer
2014-09-17 14:07:19 UTC
Permalink
Hi Matt,

I built a smartos image with the fix, I saw no issue but has no idea how to
make it go through that particular code path.

Thanks,
-Youzhong
Post by Matthew Ahrens via illumos-zfs
Post by Youzhong Yang via illumos-developer
Thanks Matt.
Your proposed fix looks good to me.
The fix is actually from Andriy. If you can test the fix on illumos, I'll
see about integrating it.
--matt
Post by Youzhong Yang via illumos-developer
The particular snapshot which cannot be mounted on our server has v_count
1 in its vnode_t struct, which leads zfs_mount() to return EBUSY.
I don't know how the zfs_snapentry_t->se_root->v_count got leaked, do you
happen to know similar issue(s)? In one of your e-mails you mentioned a
potential fix for bug 5160 (https://www.illumos.org/issues/5160), but I
cannot relate it to what happened in our system.
By the way, would you please share the fix for bug 5160?
Thanks again,
-Youzhong
0xffffff3e779203d0::print -at zfs_snapentry_t
ffffff3e779203d0 zfs_snapentry_t {
ffffff3e779203d0 char *se_name = 0xffffff3375c2f470
"Bmvm.148164.pass.ja1"
ffffff3e779203d8 vnode_t *se_root = 0xffffff3332794e00
ffffff3e779203e0 avl_node_t se_node = {
ffffff3e779203e0 struct avl_node *[2] avl_child = [
0xffffff4d8d834cf8, 0 ]
ffffff3e779203f0 uintptr_t avl_pcb = 0xffffff4df5951ec0
}
}
0xffffff3332794e00::print -at vnode_t
ffffff3332794e00 vnode_t {
ffffff3332794e00 kmutex_t v_lock = {
ffffff3332794e00 void *[1] _opaque = [ 0 ]
}
ffffff3332794e08 uint_t v_flag = 0x100
ffffff3332794e0c uint_t v_count = 0x1
ffffff3332794e10 void *v_data = 0xffffff34d23f6700
ffffff3332794e18 struct vfs *v_vfsp = 0xffffff32c8cfede0
ffffff3332794e20 struct stdata *v_stream = 0
ffffff3332794e28 enum vtype v_type = 2 (VDIR)
ffffff3332794e30 dev_t v_rdev = 0
ffffff3332794e38 struct vfs *v_vfsmountedhere = 0
ffffff3332794e40 struct vnodeops *v_op = 0xffffff32575cb4c0
ffffff3332794e48 struct page *v_pages = 0
ffffff3332794e50 struct filock *v_filocks = 0
ffffff3332794e58 struct shrlocklist *v_shrlocks = 0
ffffff3332794e60 krwlock_t v_nbllock = {
ffffff3332794e60 void *[1] _opaque = [ 0 ]
}
ffffff3332794e68 kcondvar_t v_cv = {
ffffff3332794e68 ushort_t _opaque = 0
}
ffffff3332794e70 void *v_locality = 0
ffffff3332794e78 struct fem_head *v_femhead = 0
ffffff3332794e80 char *v_path = 0
ffffff3332794e88 uint_t v_rdcnt = 0
ffffff3332794e8c uint_t v_wrcnt = 0
ffffff3332794e90 u_longlong_t v_mmap_read = 0
ffffff3332794e98 u_longlong_t v_mmap_write = 0
ffffff3332794ea0 void *v_mpssdata = 0
ffffff3332794ea8 void *v_fopdata = 0
ffffff3332794eb0 kmutex_t v_vsd_lock = {
ffffff3332794eb0 void *[1] _opaque = [ 0 ]
}
ffffff3332794eb8 struct vsd_node *v_vsd = 0
ffffff3332794ec0 struct vnode *v_xattrdir = 0
ffffff3332794ec8 uint_t v_count_dnlc = 0
}
On Mon, Sep 15, 2014 at 10:31 AM, Youzhong Yang via illumos-developer <
Post by Youzhong Yang via illumos-developer
Hi all,
When I tried to troubleshoot an issue of unable to mount a snapshot, I
came across something weird. Inside zfsctl_snapdir_lookup(), there is a
jump to 'domount', in which case leaves 'snapname' uninitialized.
Is this a bug? I saw junk data passed to the 2nd arg of
zfsctl_snapdir_lookup() when dtracing it.
Yes, this is a bug. It was reported back in 2012 but nobody pushed the
Post by Youzhong Yang via illumos-developer
It looks like in zfsctl_snapdir_lookup in the case marked with
the following
Post by Youzhong Yang via illumos-developer
comment
Post by Youzhong Yang via illumos-developer
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
'snapname' variable remains uninitialized and that likely would
lead to a
Post by Youzhong Yang via illumos-developer
failure in mount.
Agreed. If you have a fix I'd be happy to review it and help get it
integrated.
How does the following look to you?
--- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
+++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
@@ -1002,6 +1002,7 @@ zfsctl_snapdir_lookup(ap)
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
+ VERIFY(zfsctl_snapshot_zname(dvp, nm,
MAXNAMELEN, snapname) == 0);
goto domount;
} else {
/*
--matt
Post by Youzhong Yang via illumos-developer
Thanks,
-Youzhong
http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_ctldir.c#777
zfsctl_snapdir_lookup() {
.
.
.
char snapname[MAXNAMELEN];
.
.
.
mutex_enter(&sdp->sd_lock);
search.se_name = (char *)nm;
if ((sep = avl_find(&sdp->sd_snaps, &search, &where)) != NULL) {
*vpp = sep->se_root;
VN_HOLD(*vpp);
err = traverse(vpp);
if (err != 0) {
VN_RELE(*vpp);
*vpp = NULL;
} else if (*vpp == sep->se_root) {
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
goto domount;
} else {
/*
* VROOT was set during the traverse call. We need
* to clear it since we're pretending to be part
* of our parent's vfs.
*/
(*vpp)->v_flag &= ~VROOT;
}
mutex_exit(&sdp->sd_lock);
ZFS_EXIT(zfsvfs);
return (err);
}
.
.
.
mountpoint_len = strlen(refstr_value(dvp->v_vfsp->vfs_mntpt)) +
strlen("/.zfs/snapshot/") + strlen(nm) + 1;
mountpoint = kmem_alloc(mountpoint_len, KM_SLEEP);
(void) snprintf(mountpoint, mountpoint_len, "%s/.zfs/snapshot/%s",
refstr_value(dvp->v_vfsp->vfs_mntpt), nm);
margs.spec = snapname;
margs.dir = mountpoint;
margs.flags = MS_SYSSPACE | MS_NOMNTTAB;
margs.fstype = "zfs";
margs.dataptr = NULL;
margs.datalen = 0;
margs.optptr = NULL;
margs.optlen = 0;
err = domount("zfs", &margs, *vpp, kcred, &vfsp);
kmem_free(mountpoint, mountpoint_len);
.
.
.
}
*illumos-developer* | Archives
<https://www.listbox.com/member/archive/182179/=now>
<https://www.listbox.com/member/archive/rss/182179/21175174-cd73734d>
| Modify
<https://www.listbox.com/member/?&>
Your Subscription <http://www.listbox.com>
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Youzhong Yang via illumos-zfs
2014-09-17 14:09:05 UTC
Permalink
forgot to mention that I filed a bug report for this particular issue:

https://www.illumos.org/issues/5168
Post by Youzhong Yang via illumos-developer
Hi Matt,
I built a smartos image with the fix, I saw no issue but has no idea how
to make it go through that particular code path.
Thanks,
-Youzhong
Post by Matthew Ahrens via illumos-zfs
Post by Youzhong Yang via illumos-developer
Thanks Matt.
Your proposed fix looks good to me.
The fix is actually from Andriy. If you can test the fix on illumos,
I'll see about integrating it.
--matt
Post by Youzhong Yang via illumos-developer
The particular snapshot which cannot be mounted on our server has
v_count 1 in its vnode_t struct, which leads zfs_mount() to return EBUSY.
I don't know how the zfs_snapentry_t->se_root->v_count got leaked, do
you happen to know similar issue(s)? In one of your e-mails you mentioned a
potential fix for bug 5160 (https://www.illumos.org/issues/5160), but I
cannot relate it to what happened in our system.
By the way, would you please share the fix for bug 5160?
Thanks again,
-Youzhong
0xffffff3e779203d0::print -at zfs_snapentry_t
ffffff3e779203d0 zfs_snapentry_t {
ffffff3e779203d0 char *se_name = 0xffffff3375c2f470
"Bmvm.148164.pass.ja1"
ffffff3e779203d8 vnode_t *se_root = 0xffffff3332794e00
ffffff3e779203e0 avl_node_t se_node = {
ffffff3e779203e0 struct avl_node *[2] avl_child = [
0xffffff4d8d834cf8, 0 ]
ffffff3e779203f0 uintptr_t avl_pcb = 0xffffff4df5951ec0
}
}
0xffffff3332794e00::print -at vnode_t
ffffff3332794e00 vnode_t {
ffffff3332794e00 kmutex_t v_lock = {
ffffff3332794e00 void *[1] _opaque = [ 0 ]
}
ffffff3332794e08 uint_t v_flag = 0x100
ffffff3332794e0c uint_t v_count = 0x1
ffffff3332794e10 void *v_data = 0xffffff34d23f6700
ffffff3332794e18 struct vfs *v_vfsp = 0xffffff32c8cfede0
ffffff3332794e20 struct stdata *v_stream = 0
ffffff3332794e28 enum vtype v_type = 2 (VDIR)
ffffff3332794e30 dev_t v_rdev = 0
ffffff3332794e38 struct vfs *v_vfsmountedhere = 0
ffffff3332794e40 struct vnodeops *v_op = 0xffffff32575cb4c0
ffffff3332794e48 struct page *v_pages = 0
ffffff3332794e50 struct filock *v_filocks = 0
ffffff3332794e58 struct shrlocklist *v_shrlocks = 0
ffffff3332794e60 krwlock_t v_nbllock = {
ffffff3332794e60 void *[1] _opaque = [ 0 ]
}
ffffff3332794e68 kcondvar_t v_cv = {
ffffff3332794e68 ushort_t _opaque = 0
}
ffffff3332794e70 void *v_locality = 0
ffffff3332794e78 struct fem_head *v_femhead = 0
ffffff3332794e80 char *v_path = 0
ffffff3332794e88 uint_t v_rdcnt = 0
ffffff3332794e8c uint_t v_wrcnt = 0
ffffff3332794e90 u_longlong_t v_mmap_read = 0
ffffff3332794e98 u_longlong_t v_mmap_write = 0
ffffff3332794ea0 void *v_mpssdata = 0
ffffff3332794ea8 void *v_fopdata = 0
ffffff3332794eb0 kmutex_t v_vsd_lock = {
ffffff3332794eb0 void *[1] _opaque = [ 0 ]
}
ffffff3332794eb8 struct vsd_node *v_vsd = 0
ffffff3332794ec0 struct vnode *v_xattrdir = 0
ffffff3332794ec8 uint_t v_count_dnlc = 0
}
On Mon, Sep 15, 2014 at 10:31 AM, Youzhong Yang via illumos-developer <
Post by Youzhong Yang via illumos-developer
Hi all,
When I tried to troubleshoot an issue of unable to mount a snapshot, I
came across something weird. Inside zfsctl_snapdir_lookup(), there is a
jump to 'domount', in which case leaves 'snapname' uninitialized.
Is this a bug? I saw junk data passed to the 2nd arg of
zfsctl_snapdir_lookup() when dtracing it.
Yes, this is a bug. It was reported back in 2012 but nobody pushed the
Post by Youzhong Yang via illumos-developer
It looks like in zfsctl_snapdir_lookup in the case marked with
the following
Post by Youzhong Yang via illumos-developer
comment
Post by Youzhong Yang via illumos-developer
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
'snapname' variable remains uninitialized and that likely would
lead to a
Post by Youzhong Yang via illumos-developer
failure in mount.
Agreed. If you have a fix I'd be happy to review it and help get it
integrated.
How does the following look to you?
--- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
+++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
@@ -1002,6 +1002,7 @@ zfsctl_snapdir_lookup(ap)
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
+ VERIFY(zfsctl_snapshot_zname(dvp, nm,
MAXNAMELEN, snapname) == 0);
goto domount;
} else {
/*
--matt
Post by Youzhong Yang via illumos-developer
Thanks,
-Youzhong
http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_ctldir.c#777
zfsctl_snapdir_lookup() {
.
.
.
char snapname[MAXNAMELEN];
.
.
.
mutex_enter(&sdp->sd_lock);
search.se_name = (char *)nm;
if ((sep = avl_find(&sdp->sd_snaps, &search, &where)) != NULL) {
*vpp = sep->se_root;
VN_HOLD(*vpp);
err = traverse(vpp);
if (err != 0) {
VN_RELE(*vpp);
*vpp = NULL;
} else if (*vpp == sep->se_root) {
/*
* The snapshot was unmounted behind our backs,
* try to remount it.
*/
goto domount;
} else {
/*
* VROOT was set during the traverse call. We need
* to clear it since we're pretending to be part
* of our parent's vfs.
*/
(*vpp)->v_flag &= ~VROOT;
}
mutex_exit(&sdp->sd_lock);
ZFS_EXIT(zfsvfs);
return (err);
}
.
.
.
mountpoint_len = strlen(refstr_value(dvp->v_vfsp->vfs_mntpt)) +
strlen("/.zfs/snapshot/") + strlen(nm) + 1;
mountpoint = kmem_alloc(mountpoint_len, KM_SLEEP);
(void) snprintf(mountpoint, mountpoint_len, "%s/.zfs/snapshot/%s",
refstr_value(dvp->v_vfsp->vfs_mntpt), nm);
margs.spec = snapname;
margs.dir = mountpoint;
margs.flags = MS_SYSSPACE | MS_NOMNTTAB;
margs.fstype = "zfs";
margs.dataptr = NULL;
margs.datalen = 0;
margs.optptr = NULL;
margs.optlen = 0;
err = domount("zfs", &margs, *vpp, kcred, &vfsp);
kmem_free(mountpoint, mountpoint_len);
.
.
.
}
*illumos-developer* | Archives
<https://www.listbox.com/member/archive/182179/=now>
<https://www.listbox.com/member/archive/rss/182179/21175174-cd73734d>
| Modify
<https://www.listbox.com/member/?&>
Your Subscription <http://www.listbox.com>
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

Loading...