From fe4627ef755b7c263f91a0e6f07cdca5d7083501 Mon Sep 17 00:00:00 2001 From: Sebastien Roy Date: Mon, 15 May 2017 10:08:43 -0700 Subject: [PATCH] 8149 deadlock between datalink deletion and kstat read Reviewed by: Steve Gonczi Reviewed by: Pavel Zakharov Reviewed by: George Wilson Reviewed by: Ryan Zezeski Approved by: Robert Mustacchi --- usr/src/pkg/manifests/system-test-ostest.mf | 4 +- usr/src/test/os-tests/runfiles/default.run | 4 ++ usr/src/test/os-tests/tests/Makefile | 4 +- usr/src/test/os-tests/tests/{ => stress}/Makefile | 30 +++++++++- usr/src/test/os-tests/tests/stress/dladm-kstat.sh | 67 +++++++++++++++++++++++ usr/src/uts/common/io/dls/dls_mgmt.c | 60 ++++++-------------- 6 files changed, 120 insertions(+), 49 deletions(-) copy usr/src/test/os-tests/tests/{ => stress}/Makefile (52%) create mode 100755 usr/src/test/os-tests/tests/stress/dladm-kstat.sh diff --git a/usr/src/pkg/manifests/system-test-ostest.mf b/usr/src/pkg/manifests/system-test-ostest.mf index c4c6d98836..23e941d67a 100644 --- a/usr/src/pkg/manifests/system-test-ostest.mf +++ b/usr/src/pkg/manifests/system-test-ostest.mf @@ -10,7 +10,7 @@ # # -# Copyright (c) 2012 by Delphix. All rights reserved. +# Copyright (c) 2012, 2016 by Delphix. All rights reserved. # Copyright 2014, OmniTI Computer Consulting, Inc. All rights reserved. # Copyright 2016, Joyent, Inc. # @@ -29,6 +29,7 @@ dir path=opt/os-tests/tests/file-locking dir path=opt/os-tests/tests/sdevfs dir path=opt/os-tests/tests/secflags dir path=opt/os-tests/tests/sigqueue +dir path=opt/os-tests/tests/stress file path=opt/os-tests/README mode=0444 file path=opt/os-tests/bin/ostest mode=0555 file path=opt/os-tests/runfiles/default.run mode=0444 @@ -56,6 +57,7 @@ file path=opt/os-tests/tests/secflags/secflags_zonecfg mode=0555 file path=opt/os-tests/tests/secflags/stacky mode=0555 file path=opt/os-tests/tests/sigqueue/sigqueue_queue_size mode=0555 file path=opt/os-tests/tests/spoof-ras mode=0555 +file path=opt/os-tests/tests/stress/dladm-kstat mode=0555 license cr_Sun license=cr_Sun license lic_CDDL license=lic_CDDL depend fmri=system/test/testrunner type=require diff --git a/usr/src/test/os-tests/runfiles/default.run b/usr/src/test/os-tests/runfiles/default.run index 8b7e82f916..fa1049c6e6 100644 --- a/usr/src/test/os-tests/runfiles/default.run +++ b/usr/src/test/os-tests/runfiles/default.run @@ -48,5 +48,9 @@ tests = ['sigqueue_queue_size'] user = root tests = ['sdevfs_eisdir'] +[/opt/os-tests/tests/stress] +user = root +tests = ['dladm-kstat'] + [/opt/os-tests/tests/file-locking] tests = ['runtests.32', 'runtests.64'] diff --git a/usr/src/test/os-tests/tests/Makefile b/usr/src/test/os-tests/tests/Makefile index 6b6e0f9fce..3b2fd26082 100644 --- a/usr/src/test/os-tests/tests/Makefile +++ b/usr/src/test/os-tests/tests/Makefile @@ -10,9 +10,9 @@ # # -# Copyright (c) 2012 by Delphix. All rights reserved. +# Copyright (c) 2012, 2016 by Delphix. All rights reserved. # -SUBDIRS = poll secflags sigqueue spoof-ras sdevfs file-locking +SUBDIRS = poll secflags sigqueue spoof-ras sdevfs stress file-locking include $(SRC)/test/Makefile.com diff --git a/usr/src/test/os-tests/tests/Makefile b/usr/src/test/os-tests/tests/stress/Makefile similarity index 52% copy from usr/src/test/os-tests/tests/Makefile copy to usr/src/test/os-tests/tests/stress/Makefile index 6b6e0f9fce..7aed99b982 100644 --- a/usr/src/test/os-tests/tests/Makefile +++ b/usr/src/test/os-tests/tests/stress/Makefile @@ -10,9 +10,33 @@ # # -# Copyright (c) 2012 by Delphix. All rights reserved. +# Copyright (c) 2016 by Delphix. All rights reserved. # -SUBDIRS = poll secflags sigqueue spoof-ras sdevfs file-locking - +include $(SRC)/cmd/Makefile.cmd include $(SRC)/test/Makefile.com + +PROG = dladm-kstat + +ROOTOPTPKG = $(ROOT)/opt/os-tests +TESTDIR = $(ROOTOPTPKG)/tests/stress + +CMDS = $(PROG:%=$(TESTDIR)/%) +$(CMDS) := FILEMODE = 0555 + +all: $(PROG) + +install: all $(CMDS) + +clobber: clean + +clean: + -$(RM) $(PROG) + +$(CMDS): $(TESTDIR) $(PROG) + +$(TESTDIR): + $(INS.dir) + +$(TESTDIR)/%: % + $(INS.file) diff --git a/usr/src/test/os-tests/tests/stress/dladm-kstat.sh b/usr/src/test/os-tests/tests/stress/dladm-kstat.sh new file mode 100755 index 0000000000..f0ab886d9f --- /dev/null +++ b/usr/src/test/os-tests/tests/stress/dladm-kstat.sh @@ -0,0 +1,67 @@ +#!/bin/bash + +# +# This file and its contents are supplied under the terms of the +# Common Development and Distribution License ("CDDL"), version 1.0. +# You may only use this file in accordance with the terms of version +# 1.0 of the CDDL. +# +# A full copy of the text of the CDDL should have accompanied this +# source. A copy of the CDDL is also available via the Internet at +# http://www.illumos.org/license/CDDL. +# + +# +# Copyright (c) 2016 by Delphix. All rights reserved. +# + +# +# This test attempts to stress the interaction between threads adding +# and deleting datalinks, and those reading kstats associated with +# those datalinks. +# + +RUNFILE=$(mktemp) +linkname1=laverne0 +linkname2=shirley0 +duration=20 # seconds + +# +# Delete any potential datalinks left behind by the etherstub function. +# +function cleanup +{ + rm -f $RUNFILE +} + +function etherstub +{ + while [[ -e $RUNFILE ]]; do + dladm create-etherstub -t $linkname1 + dladm rename-link $linkname1 $linkname2 + dladm delete-etherstub -t $linkname2 + done +} + +function readkstat +{ + local linkname=$1 + while [[ -e $RUNFILE ]]; do + kstat link:0:$linkname &>/dev/null + done +} + +trap "cleanup; exit" SIGHUP SIGINT SIGTERM + +etherstub & +readkstat $linkname1 & +readkstat $linkname1 & +readkstat $linkname2 & +readkstat $linkname2 & + +sleep $duration +cleanup + +wait + +exit 0 diff --git a/usr/src/uts/common/io/dls/dls_mgmt.c b/usr/src/uts/common/io/dls/dls_mgmt.c index 049c4bd757..4473266242 100644 --- a/usr/src/uts/common/io/dls/dls_mgmt.c +++ b/usr/src/uts/common/io/dls/dls_mgmt.c @@ -22,6 +22,9 @@ * Copyright 2009 Sun Microsystems, Inc. All rights reserved. * Use is subject to license terms. */ +/* + * Copyright (c) 2016 by Delphix. All rights reserved. + */ /* * Datalink management routines. @@ -79,9 +82,9 @@ boolean_t devnet_need_rebuild; /* Upcall door handle */ static door_handle_t dls_mgmt_dh = NULL; +/* dls_devnet_t dd_flags */ #define DD_CONDEMNED 0x1 -#define DD_KSTAT_CHANGING 0x2 -#define DD_IMPLICIT_IPTUN 0x4 /* Implicitly-created ip*.*tun* tunnel */ +#define DD_IMPLICIT_IPTUN 0x2 /* Implicitly-created ip*.*tun* tunnel */ /* * This structure is used to keep the mapping. @@ -702,27 +705,20 @@ dls_devnet_rele_link(dls_dl_handle_t dlh, dls_link_t *dlp) static int dls_devnet_stat_update(kstat_t *ksp, int rw) { - dls_devnet_t *ddp = ksp->ks_private; + datalink_id_t linkid = (datalink_id_t)(uintptr_t)ksp->ks_private; + dls_devnet_t *ddp; dls_link_t *dlp; int err; - /* - * Check the link is being renamed or if the link is going away - * before incrementing dd_tref which in turn prevents the link - * from being renamed or deleted until we finish. - */ - mutex_enter(&ddp->dd_mutex); - if (ddp->dd_flags & (DD_CONDEMNED | DD_KSTAT_CHANGING)) { - mutex_exit(&ddp->dd_mutex); - return (ENOENT); + if ((err = dls_devnet_hold_tmp(linkid, &ddp)) != 0) { + return (err); } - ddp->dd_tref++; - mutex_exit(&ddp->dd_mutex); /* * If a device detach happens at this time, it will block in - * dls_devnet_unset since the dd_tref has been bumped up above. So the - * access to 'dlp' is safe even though we don't hold the mac perimeter. + * dls_devnet_unset since the dd_tref has been bumped in + * dls_devnet_hold_tmp(). So the access to 'dlp' is safe even though + * we don't hold the mac perimeter. */ if (mod_hash_find(i_dls_link_hash, (mod_hash_key_t)ddp->dd_mac, (mod_hash_val_t *)&dlp) != 0) { @@ -745,7 +741,8 @@ dls_devnet_stat_create(dls_devnet_t *ddp, zoneid_t zoneid) kstat_t *ksp; if (dls_stat_create("link", 0, ddp->dd_linkname, zoneid, - dls_devnet_stat_update, ddp, &ksp) == 0) { + dls_devnet_stat_update, (void *)(uintptr_t)ddp->dd_linkid, + &ksp) == 0) { ASSERT(ksp != NULL); if (zoneid == ddp->dd_owner_zid) { ASSERT(ddp->dd_ksp == NULL); @@ -988,7 +985,7 @@ dls_devnet_hold_common(datalink_id_t linkid, dls_devnet_t **ddpp, if (dls_mgmt_get_phydev(linkid, &phydev) == 0) (void) softmac_hold_device(phydev, &ddh); - rw_enter(&i_dls_devnet_lock, RW_WRITER); + rw_enter(&i_dls_devnet_lock, RW_READER); if ((err = mod_hash_find(i_dls_devnet_id_hash, (mod_hash_key_t)(uintptr_t)linkid, (mod_hash_val_t *)&ddp)) != 0) { ASSERT(err == MH_ERR_NOTFOUND); @@ -1069,7 +1066,7 @@ dls_devnet_hold_by_dev(dev_t dev, dls_dl_handle_t *ddhp) if (DLS_MINOR2INST(getminor(dev)) <= DLS_MAX_PPA) (void) softmac_hold_device(dev, &ddh); - rw_enter(&i_dls_devnet_lock, RW_WRITER); + rw_enter(&i_dls_devnet_lock, RW_READER); if ((err = mod_hash_find(i_dls_devnet_hash, (mod_hash_key_t)name, (mod_hash_val_t *)&ddp)) != 0) { ASSERT(err == MH_ERR_NOTFOUND); @@ -1272,7 +1269,6 @@ dls_devnet_rename(datalink_id_t id1, datalink_id_t id2, const char *link) mac_perim_handle_t mph = NULL; mac_handle_t mh; mod_hash_val_t val; - boolean_t clear_dd_flag = B_FALSE; /* * In the second case, id2 must be a REMOVED physical link. @@ -1308,22 +1304,12 @@ dls_devnet_rename(datalink_id_t id1, datalink_id_t id2, const char *link) goto done; } - /* - * Return EBUSY if any applications have this link open, if any thread - * is currently accessing the link kstats, or if the link is on-loan - * to a non-global zone. Then set the DD_KSTAT_CHANGING flag to - * prevent any access to the kstats while we delete and recreate - * kstats below. - */ mutex_enter(&ddp->dd_mutex); if (ddp->dd_ref > 1) { mutex_exit(&ddp->dd_mutex); err = EBUSY; goto done; } - - ddp->dd_flags |= DD_KSTAT_CHANGING; - clear_dd_flag = B_TRUE; mutex_exit(&ddp->dd_mutex); if (id2 == DATALINK_INVALID_LINKID) { @@ -1397,23 +1383,11 @@ dls_devnet_rename(datalink_id_t id1, datalink_id_t id2, const char *link) mutex_exit(&ddp->dd_mutex); done: - /* - * Change the name of the kstat based on the new link name. - * We can't hold the i_dls_devnet_lock across calls to the kstat - * subsystem. Instead the DD_KSTAT_CHANGING flag set above in this - * function prevents any access to the dd_ksp while we delete and - * recreate it below. - */ rw_exit(&i_dls_devnet_lock); + if (err == 0) dls_devnet_stat_rename(ddp); - if (clear_dd_flag) { - mutex_enter(&ddp->dd_mutex); - ddp->dd_flags &= ~DD_KSTAT_CHANGING; - mutex_exit(&ddp->dd_mutex); - } - if (mph != NULL) mac_perim_exit(mph); softmac_rele_device(ddh); -- 2.11.4.GIT