Project

General

Profile

Bug #6093

zfsctl_shares_lookup should only VN_RELE() on zfs_zget() success

Added by Dan McDonald about 4 years ago. Updated about 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
zfs - Zettabyte File System
Start date:
2015-07-31
Due date:
% Done:

100%

Estimated time:
Difficulty:
Bite-size
Tags:
needs-triage

Description

An OmniOS customer is triggering panics when using smbsrv to service MacOS 10.10.4 clients. Said customer provided me with a coredump.

> $c
zfsctl_shares_lookup+0x104(ffffff23b8f09d00, ffffff00f71be500, ffffff00f71be648
, ffffff00f71be6d0, 0, ffffff21d376e740)
fop_lookup+0xa2(ffffff23b8f09d00, ffffff00f71be500, ffffff00f71be648, 
ffffff00f71be6d0, 0, ffffff21d376e740)
lookuppnvp+0x1fe(ffffff00f71be6d0, 0, 0, 0, ffffff00f71be6f8, ffffff21d376e740)
smb_tree_acl_access+0xf2(ffffff2296f6e328, ffffff228875ad50, ffffff2288e38b80)
smb_tree_chkaccess+0x9f(ffffff2296f6e328, ffffff228875ad50, ffffff2288e38b80)
smb_tree_connect_disk+0x156(ffffff2296f6e328, ffffff22891df642)
smb_tree_connect_core+0xc3(ffffff2296f6e328)
smb_tree_connect+0x3a(ffffff2296f6e328)
smb_com_tree_connect_andx+0x18(ffffff2296f6e328)
smb_dispatch_request+0x687(ffffff2296f6e328)
smb_session_worker+0xa0(ffffff2296f6e328)
taskq_d_thread+0xb7(ffffff224e25a860)
thread_start+8()
> ::status
debugging crash dump vmcore.2 (64-bit) from remotium
operating system: 5.11 omnios-7648372 (i86pc)
image uuid: 5de575b1-5751-cd6b-d1f7-eb0ea21ba17a
panic message: 
BAD TRAP: type=e (#pf Page fault) rp=ffffff00f71be210 addr=8 occurred in module 
"zfs" due to a NULL pointer dereference
dump content: kernel pages only
> 

So clearly a NULL pointer is being dereferenced here, where offset 8 is being desired. Let's look at the offensive code:

> zfsctl_shares_lookup+0x104::dis
zfsctl_shares_lookup+0xe5:      nop    
zfsctl_shares_lookup+0xe6:      movl   $0x30,%eax
zfsctl_shares_lookup+0xeb:      jmp    -0x60    <zfsctl_shares_lookup+0x8d>
zfsctl_shares_lookup+0xed:      nopl   (%rax)
zfsctl_shares_lookup+0xf0:      leaq   -0x68(%rbp),%rdx
zfsctl_shares_lookup+0xf4:      movq   %r14,%rdi
zfsctl_shares_lookup+0xf7:      call   -0x1854c <zfs_zget>
zfsctl_shares_lookup+0xfc:      testl  %eax,%eax
zfsctl_shares_lookup+0xfe:      je     +0x48    <zfsctl_shares_lookup+0x148>
zfsctl_shares_lookup+0x100:     movq   -0x68(%rbp),%rdx
zfsctl_shares_lookup+0x104:     movq   0x8(%rdx),%rdi
zfsctl_shares_lookup+0x108:     movl   %eax,-0x90(%rbp)
zfsctl_shares_lookup+0x10e:     call   +0x4114d0d       <vn_rele>
zfsctl_shares_lookup+0x113:     movq   $0xfffffffff7a695a0,%rsi <__func__.34297>
zfsctl_shares_lookup+0x11a:     movq   %rbx,%rdi
zfsctl_shares_lookup+0x11d:     call   -0x4bca2 <rrm_exit>
zfsctl_shares_lookup+0x122:     movl   -0x90(%rbp),%eax
zfsctl_shares_lookup+0x128:     jmp    -0xa0    <zfsctl_shares_lookup+0x8d>
zfsctl_shares_lookup+0x12d:     nopl   (%rax)
zfsctl_shares_lookup+0x130:     movq   $0xfffffffff7a695a0,%rsi <__func__.34297>
zfsctl_shares_lookup+0x137:     movq   %rbx,%rdi
> <rdx=P
                0               
> 

Looks like code right after a call to zfs_zget() that does NOT return 0. Source shows:

        if ((error = zfs_zget(zfsvfs, zfsvfs->z_shares_dir, &dzp)) == 0)
                error = VOP_LOOKUP(ZTOV(dzp), nm, vpp, pnp,
                    flags, rdir, cr, ct, direntflags, realpnp);

        VN_RELE(ZTOV(dzp));      /* XXX KEBE SAYS DANGER!!! */
        ZFS_EXIT(zfsvfs);

The VN_RELE() call will have a NULL dzp if zfs_zget() fails. zfs_zget() initializes dzp to NULL as its first line, so we know the call ALWAYS puts something there.

Other portions of the code in zfs_ctldir.c only VN_RELE() if zfs_zget() returns without an error. This code shown above in zfsctl_shares_lookup() should do the same.

History

#1

Updated by Dan McDonald about 4 years ago

  • Subject changed from zfsctl_shares_lookup should only VN_RELE() on zfs_zget() success to zfs_zget() callers need to act better

Decided to open #6096 as a distinct issue.

#2

Updated by Dan McDonald about 4 years ago

  • Subject changed from zfs_zget() callers need to act better to zfsctl_shares_lookup should only VN_RELE() on zfs_zget() success
#3

Updated by Electric Monk about 4 years ago

  • Status changed from New to Closed
  • % Done changed from 80 to 100

git commit 0f92170f1ec2737ee5a0e51b5f74093904811452

commit  0f92170f1ec2737ee5a0e51b5f74093904811452
Author: Dan McDonald <danmcd@omniti.com>
Date:   2015-08-03T19:19:02.000Z

    6093 zfsctl_shares_lookup should only VN_RELE() on zfs_zget() success
    Reviewed by: Gordon Ross <gwr@nexenta.com>
    Reviewed by: Matthew Ahrens <mahrens@delphix.com>
    Reviewed by: George Wilson <george.wilson@delphix.com>
    Approved by: Robert Mustacchi <rm@joyent.com>

Also available in: Atom PDF