From owner-svn-src-all@freebsd.org Thu May 24 10:16:12 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id AB76FEAACD1; Thu, 24 May 2018 10:16:12 +0000 (UTC) (envelope-from royger@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 5CF6581A95; Thu, 24 May 2018 10:16:12 +0000 (UTC) (envelope-from royger@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 3974710E70; Thu, 24 May 2018 10:16:12 +0000 (UTC) (envelope-from royger@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id w4OAGBD8076556; Thu, 24 May 2018 10:16:11 GMT (envelope-from royger@FreeBSD.org) Received: (from royger@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id w4OAGBJ7076555; Thu, 24 May 2018 10:16:11 GMT (envelope-from royger@FreeBSD.org) Message-Id: <201805241016.w4OAGBJ7076555@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: royger set sender to royger@FreeBSD.org using -f From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Date: Thu, 24 May 2018 10:16:11 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r334140 - head/sys/dev/xen/xenstore X-SVN-Group: head X-SVN-Commit-Author: royger X-SVN-Commit-Paths: head/sys/dev/xen/xenstore X-SVN-Commit-Revision: 334140 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 May 2018 10:16:12 -0000 Author: royger Date: Thu May 24 10:16:11 2018 New Revision: 334140 URL: https://svnweb.freebsd.org/changeset/base/334140 Log: xenstore: remove the suspend sx lock There's no need to prevent suspend while doing xenstore transactions, callers of transactions are supposed to be prepared for a transaction to fail. This fixes a bug that could be triggered from the xenstore user-space device, since starting a transaction from user-space would result in returning there with a sx lock held, that causes a WITNESS check to trigger. Tested by: Nathan Friess Sponsored by: Citrix Systems R&D Modified: head/sys/dev/xen/xenstore/xenstore.c Modified: head/sys/dev/xen/xenstore/xenstore.c ============================================================================== --- head/sys/dev/xen/xenstore/xenstore.c Thu May 24 08:32:02 2018 (r334139) +++ head/sys/dev/xen/xenstore/xenstore.c Thu May 24 10:16:11 2018 (r334140) @@ -202,18 +202,6 @@ struct xs_softc { struct mtx watch_events_lock; /** - * Sleepable lock used to prevent VM suspension while a - * xenstore transaction is outstanding. - * - * Each active transaction holds a shared lock on the - * suspend mutex. Our suspend method blocks waiting - * to acquire an exclusive lock. This guarantees that - * suspend processing will only proceed once all active - * transactions have been retired. - */ - struct sx suspend_mutex; - - /** * The processid of the xenwatch thread. */ pid_t xenwatch_pid; @@ -710,50 +698,6 @@ xs_rcv_thread(void *arg __unused) } /*---------------- XenStore Message Request/Reply Processing -----------------*/ -/** - * Filter invoked before transmitting any message to the XenStore service. - * - * The role of the filter may expand, but currently serves to manage - * the interactions of messages with transaction state. - * - * \param request_msg_type The message type for the request. - */ -static inline void -xs_request_filter(uint32_t request_msg_type) -{ - if (request_msg_type == XS_TRANSACTION_START) - sx_slock(&xs.suspend_mutex); -} - -/** - * Filter invoked after transmitting any message to the XenStore service. - * - * The role of the filter may expand, but currently serves to manage - * the interactions of messages with transaction state. - * - * \param request_msg_type The message type for the original request. - * \param reply_msg_type The message type for any received reply. - * \param request_reply_error The error status from the attempt to send - * the request or retrieve the reply. - */ -static inline void -xs_reply_filter(uint32_t request_msg_type, - uint32_t reply_msg_type, int request_reply_error) -{ - /* - * The count of transactions drops if we attempted - * to end a transaction (even if that attempt fails - * in error), we receive a transaction end acknowledgement, - * or if our attempt to begin a transaction fails. - */ - if (request_msg_type == XS_TRANSACTION_END - || (request_reply_error == 0 && reply_msg_type == XS_TRANSACTION_END) - || (request_msg_type == XS_TRANSACTION_START - && (request_reply_error != 0 || reply_msg_type == XS_ERROR))) - sx_sunlock(&xs.suspend_mutex); - -} - #define xsd_error_count (sizeof(xsd_errors) / sizeof(xsd_errors[0])) /** @@ -843,15 +787,12 @@ xs_dev_request_and_reply(struct xsd_sockmsg *msg, void int error; request_type = msg->type; - xs_request_filter(request_type); sx_xlock(&xs.request_mutex); if ((error = xs_write_store(msg, sizeof(*msg) + msg->len)) == 0) error = xs_read_reply(&msg->type, &msg->len, result); sx_xunlock(&xs.request_mutex); - xs_reply_filter(request_type, msg->type, error); - return (error); } @@ -887,8 +828,6 @@ xs_talkv(struct xs_transaction t, enum xsd_sockmsg_typ for (i = 0; i < num_vecs; i++) msg.len += iovec[i].iov_len; - xs_request_filter(request_type); - sx_xlock(&xs.request_mutex); error = xs_write_store(&msg, sizeof(msg)); if (error) { @@ -908,7 +847,6 @@ xs_talkv(struct xs_transaction t, enum xsd_sockmsg_typ error_lock_held: sx_xunlock(&xs.request_mutex); - xs_reply_filter(request_type, msg.type, error); if (error) return (error); @@ -1206,7 +1144,6 @@ xs_attach(device_t dev) mtx_init(&xs.reply_lock, "reply lock", NULL, MTX_DEF); sx_init(&xs.xenwatch_mutex, "xenwatch"); sx_init(&xs.request_mutex, "xenstore request"); - sx_init(&xs.suspend_mutex, "xenstore suspend"); mtx_init(&xs.registered_watches_lock, "watches", NULL, MTX_DEF); mtx_init(&xs.watch_events_lock, "watch events", NULL, MTX_DEF); @@ -1249,7 +1186,6 @@ xs_suspend(device_t dev) if (error != 0) return (error); - sx_xlock(&xs.suspend_mutex); sx_xlock(&xs.request_mutex); return (0); @@ -1269,16 +1205,16 @@ xs_resume(device_t dev __unused) sx_xunlock(&xs.request_mutex); /* - * No need for registered_watches_lock: the suspend_mutex - * is sufficient. + * NB: since xenstore childs have not been resumed yet, there's + * no need to hold any watch mutex. Having clients try to add or + * remove watches at this point (before xenstore is resumed) is + * clearly a violantion of the resume order. */ LIST_FOREACH(watch, &xs.registered_watches, list) { sprintf(token, "%lX", (long)watch); xs_watch(watch->node, token); } - sx_xunlock(&xs.suspend_mutex); - /* Resume child Xen devices. */ bus_generic_resume(dev); @@ -1631,8 +1567,6 @@ xs_register_watch(struct xs_watch *watch) sprintf(token, "%lX", (long)watch); - sx_slock(&xs.suspend_mutex); - mtx_lock(&xs.registered_watches_lock); KASSERT(find_watch(token) == NULL, ("watch already registered")); LIST_INSERT_HEAD(&xs.registered_watches, watch, list); @@ -1650,8 +1584,6 @@ xs_register_watch(struct xs_watch *watch) mtx_unlock(&xs.registered_watches_lock); } - sx_sunlock(&xs.suspend_mutex); - return (error); } @@ -1664,12 +1596,9 @@ xs_unregister_watch(struct xs_watch *watch) sprintf(token, "%lX", (long)watch); - sx_slock(&xs.suspend_mutex); - mtx_lock(&xs.registered_watches_lock); if (find_watch(token) == NULL) { mtx_unlock(&xs.registered_watches_lock); - sx_sunlock(&xs.suspend_mutex); return; } LIST_REMOVE(watch, list); @@ -1679,8 +1608,6 @@ xs_unregister_watch(struct xs_watch *watch) if (error) log(LOG_WARNING, "XENSTORE Failed to release watch %s: %i\n", watch->node, error); - - sx_sunlock(&xs.suspend_mutex); /* Cancel pending watch events. */ mtx_lock(&xs.watch_events_lock);