From bc7c423f80bd757204d3b17cfd74585ae5b30ed8 Mon Sep 17 00:00:00 2001 From: Gordon Ross Date: Fri, 30 Aug 2013 21:11:00 -0400 Subject: [PATCH] 5319 smb_oplock_acquire thread deadlock Reviewed by: Dan McDonald Reviewed by: Sarah Jelinek Reviewed by: Jeffry Molanus Approved by: Robert Mustacchi --- usr/src/uts/common/fs/smbsrv/smb_delete.c | 54 +++++++++++++++--- usr/src/uts/common/fs/smbsrv/smb_lock.c | 57 +++++++++++-------- usr/src/uts/common/fs/smbsrv/smb_node.c | 18 +----- usr/src/uts/common/fs/smbsrv/smb_oplock.c | 17 +++--- usr/src/uts/common/fs/smbsrv/smb_rename.c | 95 +++++++++++++++++++++++-------- usr/src/uts/common/smbsrv/smb_kproto.h | 4 +- 6 files changed, 162 insertions(+), 83 deletions(-) diff --git a/usr/src/uts/common/fs/smbsrv/smb_delete.c b/usr/src/uts/common/fs/smbsrv/smb_delete.c index eb6cde5bcd..4930f741ef 100644 --- a/usr/src/uts/common/fs/smbsrv/smb_delete.c +++ b/usr/src/uts/common/fs/smbsrv/smb_delete.c @@ -463,7 +463,7 @@ smb_delete_check_dosattr(smb_request_t *sr, smb_error_t *err) * NT does not always close a file immediately, which can cause the * share and access checking to fail (the node refcnt is greater * than one), and the file doesn't get deleted. Breaking the oplock - * before share and access checking gives the client a chance to + * before share and lock checking gives the client a chance to * close the file. * * Returns: 0 - success @@ -472,7 +472,7 @@ smb_delete_check_dosattr(smb_request_t *sr, smb_error_t *err) static int smb_delete_remove_file(smb_request_t *sr, smb_error_t *err) { - int rc; + int rc, count; uint32_t status; smb_fqi_t *fqi; smb_node_t *node; @@ -481,24 +481,62 @@ smb_delete_remove_file(smb_request_t *sr, smb_error_t *err) fqi = &sr->arg.dirop.fqi; node = fqi->fq_fnode; + /* + * Break BATCH oplock before ofile checks. If a client + * has a file open, this will force a flush or close, + * which may affect the outcome of any share checking. + */ (void) smb_oplock_break(sr, node, SMB_OPLOCK_BREAK_TO_LEVEL_II | SMB_OPLOCK_BREAK_BATCH); - smb_node_start_crit(node, RW_READER); - - status = smb_node_delete_check(node); + /* + * Wait (a little) for the oplock break to be + * responded to by clients closing handles. + * Hold node->n_lock as reader to keep new + * ofiles from showing up after we check. + */ + smb_node_rdlock(node); + for (count = 0; count <= 12; count++) { + status = smb_node_delete_check(node); + if (status != NT_STATUS_SHARING_VIOLATION) + break; + smb_node_unlock(node); + delay(MSEC_TO_TICK(100)); + smb_node_rdlock(node); + } if (status != NT_STATUS_SUCCESS) { smb_delete_error(err, NT_STATUS_SHARING_VIOLATION, ERRDOS, ERROR_SHARING_VIOLATION); - smb_node_end_crit(node); + smb_node_unlock(node); return (-1); } - status = smb_range_check(sr, node, 0, UINT64_MAX, B_TRUE); + /* + * Note, the combination of these two: + * smb_node_rdlock(node); + * nbl_start_crit(node->vp, RW_READER); + * is equivalent to this call: + * smb_node_start_crit(node, RW_READER) + * + * Cleanup after this point should use: + * smb_node_end_crit(node) + */ + nbl_start_crit(node->vp, RW_READER); + + /* + * This checks nbl_share_conflict, nbl_lock_conflict + */ + status = smb_nbl_conflict(node, 0, UINT64_MAX, NBL_REMOVE); + if (status == NT_STATUS_SHARING_VIOLATION) { + smb_node_end_crit(node); + smb_delete_error(err, NT_STATUS_SHARING_VIOLATION, + ERRDOS, ERROR_SHARING_VIOLATION); + return (-1); + } if (status != NT_STATUS_SUCCESS) { + smb_node_end_crit(node); smb_delete_error(err, NT_STATUS_ACCESS_DENIED, ERRDOS, ERROR_ACCESS_DENIED); - smb_node_end_crit(node); return (-1); } diff --git a/usr/src/uts/common/fs/smbsrv/smb_lock.c b/usr/src/uts/common/fs/smbsrv/smb_lock.c index f35cbc559d..98df20b66d 100644 --- a/usr/src/uts/common/fs/smbsrv/smb_lock.c +++ b/usr/src/uts/common/fs/smbsrv/smb_lock.c @@ -20,6 +20,7 @@ */ /* * Copyright (c) 2007, 2010, Oracle and/or its affiliates. All rights reserved. + * Copyright 2013 Nexenta Systems, Inc. All rights reserved. */ /* @@ -267,7 +268,7 @@ smb_lock_range_access( smb_request_t *sr, smb_node_t *node, uint64_t start, - uint64_t length, + uint64_t length, /* zero means to EoF */ boolean_t will_write) { smb_lock_t *lock; @@ -359,44 +360,50 @@ smb_lock_range_error(smb_request_t *sr, uint32_t status32) } /* - * smb_range_check() - * - * Perform range checking. First check for internal CIFS range conflicts - * and then check for external conflicts, for example, with NFS or local - * access. - * - * If nbmand is enabled, this function must be called from within an nbmand - * critical region + * An SMB variant of nbl_conflict(). + * + * SMB prevents remove or rename when conflicting locks exist + * (unlike NFS, which is why we can't just use nbl_conflict). + * + * Returns: + * NT_STATUS_SHARING_VIOLATION - nbl_share_conflict + * NT_STATUS_FILE_LOCK_CONFLICT - nbl_lock_conflict + * NT_STATUS_SUCCESS - operation can proceed + * + * NB: This function used to also check the list of ofiles, + * via: smb_lock_range_access() but we _can't_ do that here + * due to lock order constraints between node->n_lock_list + * and node->vp->vnbllock (taken via nvl_start_crit). + * They must be taken in that order, and in here, we + * already hold vp->vnbllock. */ - DWORD -smb_range_check(smb_request_t *sr, smb_node_t *node, uint64_t start, - uint64_t length, boolean_t will_write) +smb_nbl_conflict(smb_node_t *node, uint64_t off, uint64_t len, nbl_op_t op) { - smb_error_t smberr; int svmand; - int nbl_op; - int rc; SMB_NODE_VALID(node); - ASSERT(smb_node_in_crit(node)); + ASSERT(op == NBL_READ || op == NBL_WRITE || op == NBL_READWRITE || + op == NBL_REMOVE || op == NBL_RENAME); if (smb_node_is_dir(node)) return (NT_STATUS_SUCCESS); - rc = smb_lock_range_access(sr, node, start, length, will_write); - if (rc) - return (NT_STATUS_FILE_LOCK_CONFLICT); + if (nbl_share_conflict(node->vp, op, &smb_ct)) + return (NT_STATUS_SHARING_VIOLATION); - if ((rc = nbl_svmand(node->vp, kcred, &svmand)) != 0) { - smbsr_map_errno(rc, &smberr); - return (smberr.status); - } + /* + * When checking for lock conflicts, rename and remove + * are not allowed, so treat those as read/write. + */ + if (op == NBL_RENAME || op == NBL_REMOVE) + op = NBL_READWRITE; - nbl_op = (will_write) ? NBL_WRITE : NBL_READ; + if (nbl_svmand(node->vp, kcred, &svmand)) + svmand = 1; - if (nbl_lock_conflict(node->vp, nbl_op, start, length, svmand, &smb_ct)) + if (nbl_lock_conflict(node->vp, op, off, len, svmand, &smb_ct)) return (NT_STATUS_FILE_LOCK_CONFLICT); return (NT_STATUS_SUCCESS); diff --git a/usr/src/uts/common/fs/smbsrv/smb_node.c b/usr/src/uts/common/fs/smbsrv/smb_node.c index d5107a1323..a42886e23e 100644 --- a/usr/src/uts/common/fs/smbsrv/smb_node.c +++ b/usr/src/uts/common/fs/smbsrv/smb_node.c @@ -696,14 +696,7 @@ smb_node_rename_check(smb_node_t *node) } } smb_llist_exit(&node->n_ofile_list); - - /* - * system-wide share check - */ - if (nbl_share_conflict(node->vp, NBL_RENAME, NULL)) - return (NT_STATUS_SHARING_VIOLATION); - else - return (NT_STATUS_SUCCESS); + return (NT_STATUS_SUCCESS); } uint32_t @@ -740,14 +733,7 @@ smb_node_delete_check(smb_node_t *node) } } smb_llist_exit(&node->n_ofile_list); - - /* - * system-wide share check - */ - if (nbl_share_conflict(node->vp, NBL_REMOVE, NULL)) - return (NT_STATUS_SHARING_VIOLATION); - else - return (NT_STATUS_SUCCESS); + return (NT_STATUS_SUCCESS); } /* diff --git a/usr/src/uts/common/fs/smbsrv/smb_oplock.c b/usr/src/uts/common/fs/smbsrv/smb_oplock.c index 83b8402456..d86c310103 100644 --- a/usr/src/uts/common/fs/smbsrv/smb_oplock.c +++ b/usr/src/uts/common/fs/smbsrv/smb_oplock.c @@ -192,7 +192,7 @@ smb_oplock_uninstall_fem(smb_node_t *node) * - op->op_oplock_levelII is B_FALSE (LEVEL_II not supported by open cmd. * - LEVEL_II oplocks are not supported for the session * - a BATCH oplock is requested on a named stream - * - there are any range locks on the node + * - there are any range locks on the node (SMB writers) * Otherwise, grant LEVEL_II. * * ol->ol_xthread is set to the current thread to lock the oplock against @@ -214,6 +214,7 @@ smb_oplock_acquire(smb_request_t *sr, smb_node_t *node, smb_ofile_t *ofile) SMB_OFILE_VALID(ofile); ASSERT(node == SMB_OFILE_GET_NODE(ofile)); + ASSERT(RW_LOCK_HELD(&node->n_lock)); op = &sr->sr_open; tree = SMB_OFILE_GET_TREE(ofile); @@ -233,17 +234,21 @@ smb_oplock_acquire(smb_request_t *sr, smb_node_t *node, smb_ofile_t *ofile) mutex_enter(&ol->ol_mutex); smb_oplock_wait(node); - nbl_start_crit(node->vp, RW_READER); - if ((node->n_open_count > 1) || (node->n_opening_count > 1) || smb_vop_other_opens(node->vp, ofile->f_mode)) { + /* + * There are other opens. + */ if ((!op->op_oplock_levelII) || (!smb_session_levelII_oplocks(session)) || (smb_oplock_exclusive_grant(grants) != NULL) || - (smb_range_check(sr, node, 0, UINT64_MAX, B_TRUE) != 0)) { + (smb_lock_range_access(sr, node, 0, 0, B_FALSE))) { + /* + * LevelII (shared) oplock not allowed, + * so reply with "none". + */ op->op_oplock_level = SMB_OPLOCK_NONE; - nbl_end_crit(node->vp); mutex_exit(&ol->ol_mutex); return; } @@ -251,8 +256,6 @@ smb_oplock_acquire(smb_request_t *sr, smb_node_t *node, smb_ofile_t *ofile) op->op_oplock_level = SMB_OPLOCK_LEVEL_II; } - nbl_end_crit(node->vp); - og = smb_oplock_set_grant(ofile, op->op_oplock_level); if (smb_oplock_insert_grant(node, og) != 0) { smb_oplock_clear_grant(og); diff --git a/usr/src/uts/common/fs/smbsrv/smb_rename.c b/usr/src/uts/common/fs/smbsrv/smb_rename.c index 7c0bffd034..03146835c2 100644 --- a/usr/src/uts/common/fs/smbsrv/smb_rename.c +++ b/usr/src/uts/common/fs/smbsrv/smb_rename.c @@ -437,6 +437,9 @@ smb_common_rename(smb_request_t *sr, smb_fqi_t *src_fqi, smb_fqi_t *dst_fqi) } if (dst_fqi->fq_fnode) { + /* + * Destination already exists. Do delete checks. + */ dst_fnode = dst_fqi->fq_fnode; if (!(sr->arg.dirop.flags && SMB_RENAME_FLAG_OVERWRITE)) { @@ -449,26 +452,48 @@ smb_common_rename(smb_request_t *sr, smb_fqi_t *src_fqi, smb_fqi_t *dst_fqi) (void) smb_oplock_break(sr, dst_fnode, SMB_OPLOCK_BREAK_TO_NONE | SMB_OPLOCK_BREAK_BATCH); - for (count = 0; count <= 3; count++) { - if (count) { - smb_node_end_crit(dst_fnode); - delay(MSEC_TO_TICK(400)); - } - - smb_node_start_crit(dst_fnode, RW_READER); + /* + * Wait (a little) for the oplock break to be + * responded to by clients closing handles. + * Hold node->n_lock as reader to keep new + * ofiles from showing up after we check. + */ + smb_node_rdlock(dst_fnode); + for (count = 0; count <= 12; count++) { status = smb_node_delete_check(dst_fnode); - if (status != NT_STATUS_SHARING_VIOLATION) break; + smb_node_unlock(dst_fnode); + delay(MSEC_TO_TICK(100)); + smb_node_rdlock(dst_fnode); + } + if (status != NT_STATUS_SUCCESS) { + smb_node_unlock(dst_fnode); + smb_rename_release_src(sr); + smb_node_release(dst_fnode); + smb_node_release(dst_dnode); + return (EACCES); } - if (status != NT_STATUS_SHARING_VIOLATION) - status = smb_range_check(sr, dst_fnode, - 0, UINT64_MAX, B_TRUE); + /* + * Note, the combination of these two: + * smb_node_rdlock(node); + * nbl_start_crit(node->vp, RW_READER); + * is equivalent to this call: + * smb_node_start_crit(node, RW_READER) + * + * Cleanup after this point should use: + * smb_node_end_crit(dst_fnode) + */ + nbl_start_crit(dst_fnode->vp, RW_READER); + /* + * This checks nbl_share_conflict, nbl_lock_conflict + */ + status = smb_nbl_conflict(dst_fnode, 0, UINT64_MAX, NBL_REMOVE); if (status != NT_STATUS_SUCCESS) { - smb_rename_release_src(sr); smb_node_end_crit(dst_fnode); + smb_rename_release_src(sr); smb_node_release(dst_fnode); smb_node_release(dst_dnode); return (EACCES); @@ -704,41 +729,61 @@ smb_rename_lookup_src(smb_request_t *sr) } /* - * Break BATCH oplock before access checks. If a client + * Break BATCH oplock before ofile checks. If a client * has a file open, this will force a flush or close, * which may affect the outcome of any share checking. */ (void) smb_oplock_break(sr, src_node, SMB_OPLOCK_BREAK_TO_LEVEL_II | SMB_OPLOCK_BREAK_BATCH); - for (count = 0; count <= 3; count++) { - if (count) { - smb_node_end_crit(src_node); - delay(MSEC_TO_TICK(400)); - } - - smb_node_start_crit(src_node, RW_READER); - + /* + * Wait (a little) for the oplock break to be + * responded to by clients closing handles. + * Hold node->n_lock as reader to keep new + * ofiles from showing up after we check. + */ + smb_node_rdlock(src_node); + for (count = 0; count <= 12; count++) { status = smb_node_rename_check(src_node); if (status != NT_STATUS_SHARING_VIOLATION) break; + smb_node_unlock(src_node); + delay(MSEC_TO_TICK(100)); + smb_node_rdlock(src_node); } - - if (status == NT_STATUS_SHARING_VIOLATION) { - smb_node_end_crit(src_node); + if (status != NT_STATUS_SUCCESS) { + smb_node_unlock(src_node); smb_node_release(src_fqi->fq_fnode); smb_node_release(src_fqi->fq_dnode); return (EPIPE); /* = ERRbadshare */ } - status = smb_range_check(sr, src_node, 0, UINT64_MAX, B_TRUE); + /* + * Note, the combination of these two: + * smb_node_rdlock(node); + * nbl_start_crit(node->vp, RW_READER); + * is equivalent to this call: + * smb_node_start_crit(node, RW_READER) + * + * Cleanup after this point should use: + * smb_node_end_crit(src_node) + */ + nbl_start_crit(src_node->vp, RW_READER); + + /* + * This checks nbl_share_conflict, nbl_lock_conflict + */ + status = smb_nbl_conflict(src_node, 0, UINT64_MAX, NBL_RENAME); if (status != NT_STATUS_SUCCESS) { smb_node_end_crit(src_node); smb_node_release(src_fqi->fq_fnode); smb_node_release(src_fqi->fq_dnode); + if (status == NT_STATUS_SHARING_VIOLATION) + return (EPIPE); /* = ERRbadshare */ return (EACCES); } + /* NB: Caller expects holds on src_fqi fnode, dnode */ return (0); } diff --git a/usr/src/uts/common/smbsrv/smb_kproto.h b/usr/src/uts/common/smbsrv/smb_kproto.h index cb6d02ebce..f2de265176 100644 --- a/usr/src/uts/common/smbsrv/smb_kproto.h +++ b/usr/src/uts/common/smbsrv/smb_kproto.h @@ -43,6 +43,7 @@ extern "C" { #include #include #include +#include #include #include #include @@ -227,8 +228,7 @@ uint32_t smb_unlock_range(smb_request_t *, smb_node_t *, uint32_t smb_lock_range(smb_request_t *, uint64_t, uint64_t, uint32_t, uint32_t locktype); void smb_lock_range_error(smb_request_t *, uint32_t); -DWORD smb_range_check(smb_request_t *, smb_node_t *, uint64_t, uint64_t, - boolean_t); +DWORD smb_nbl_conflict(smb_node_t *, uint64_t, uint64_t, nbl_op_t); void smb_mangle(const char *, ino64_t, char *, size_t); int smb_unmangle(smb_node_t *, char *, char *, int, uint32_t); -- 2.11.4.GIT