From patchwork Tue Jul 31 13:49:58 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Desrochers X-Patchwork-Id: 951611 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=canonical.com Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 41fyVx2ZFcz9s3x; Tue, 31 Jul 2018 23:50:17 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1fkV2O-0003X0-IK; Tue, 31 Jul 2018 13:50:08 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.86_2) (envelope-from ) id 1fkV2M-0003WU-8n for kernel-team@lists.ubuntu.com; Tue, 31 Jul 2018 13:50:06 +0000 Received: from mail-qk0-f199.google.com ([209.85.220.199]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1fkV2L-0002Br-Rz for kernel-team@lists.ubuntu.com; Tue, 31 Jul 2018 13:50:06 +0000 Received: by mail-qk0-f199.google.com with SMTP id u22-v6so13969123qkk.10 for ; Tue, 31 Jul 2018 06:50:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=kxCyR+JG9N/i49xJZohWbvNYEHLKNBx4sPHE2DbITqE=; b=iA5bd0QYHSjhRjmE6MCanbUM7zdzZ2CO5sT9vsSJi/eeoly/9WnV06esuIf20O5cmC Ugqu7jWyVcXAPYNpS932nkEsMOMtjjUsnaRngGSqDnwMeqjnjlf5frhq1RzLjVE1yECf I/ANmqErNDyJkPsxatGEYbHWMqdXbJiIq/u8D8/PWvjM2mk9fFmPDQaJK2N04HD5CT7Y U4ByhDBmQMwy+sWI5QrDitMvVkQop3HRbks97TZ7rxl4i+QZyIiJ906ddbsgofHYZ/84 GA0RVdbt46geBpzmAnBHv1Mi7lI0kfX7vXddcrvlC0CQH7jEwjPYWBv6/Z91x7fN2m/h G8kQ== X-Gm-Message-State: AOUpUlG9uMf3ovBd3u/9KCYSQfHz6UaxdqbkY8TVXOirFKOW93Okd1v6 bjEE5I2y6B5VAYQB19/eEN98saictJwFcnFGOjwWe96OjcvZyabrMYmv3dg60UrLhFRV1xM6JXg 0hl8t/Yv63XOJUKWS9GwHNcB8p97t3OSA2rQzt7EWfw== X-Received: by 2002:ac8:2bb3:: with SMTP id m48-v6mr20813923qtm.207.1533045004877; Tue, 31 Jul 2018 06:50:04 -0700 (PDT) X-Google-Smtp-Source: AAOMgpeSU1GdWfuT+4cOSQqZK5NinJjOmsmLq/nV8HHmgj3fM7kwPx/EksdU0FneDOEzRFfjJ50SMA== X-Received: by 2002:ac8:2bb3:: with SMTP id m48-v6mr20813892qtm.207.1533045004482; Tue, 31 Jul 2018 06:50:04 -0700 (PDT) Received: from thinkpad.in.azkaban.com (modemcable048.146-83-70.mc.videotron.ca. [70.83.146.48]) by smtp.gmail.com with ESMTPSA id h195-v6sm9095468qke.58.2018.07.31.06.50.03 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 31 Jul 2018 06:50:03 -0700 (PDT) From: Eric Desrochers To: kernel-team@lists.ubuntu.com Subject: [SRU][Trusty][PATCH 1/3] VSOCK: sock_put wasn't safe to call in interrupt context Date: Tue, 31 Jul 2018 09:49:58 -0400 Message-Id: <20180731135000.6114-2-eric.desrochers@canonical.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180731135000.6114-1-eric.desrochers@canonical.com> References: <20180731135000.6114-1-eric.desrochers@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Jorgen Hansen BugLink: https://bugs.launchpad.net/bugs/1780470 In the vsock vmci_transport driver, sock_put wasn't safe to call in interrupt context, since that may call the vsock destructor which in turn calls several functions that should only be called from process context. This change defers the callling of these functions to a worker thread. All these functions were deallocation of resources related to the transport itself. Furthermore, an unused callback was removed to simplify the cleanup. Multiple customers have been hitting this issue when using VMware tools on vSphere 2015. Reviewed-by: Aditya Asarwade Reviewed-by: Thomas Hellstrom Signed-off-by: Jorgen Hansen Signed-off-by: David S. Miller (cherry picked from 4ef7ea9195ea73262cd9730fb54e1eb726da157b) Signed-off-by: Eric Desrochers --- net/vmw_vsock/vmci_transport.c | 173 ++++++++++++++++----------------- net/vmw_vsock/vmci_transport.h | 4 +- 2 files changed, 86 insertions(+), 91 deletions(-) diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c index 687360da62d9..65c80a1d1bd3 100644 --- a/net/vmw_vsock/vmci_transport.c +++ b/net/vmw_vsock/vmci_transport.c @@ -40,13 +40,11 @@ static int vmci_transport_recv_dgram_cb(void *data, struct vmci_datagram *dg); static int vmci_transport_recv_stream_cb(void *data, struct vmci_datagram *dg); -static void vmci_transport_peer_attach_cb(u32 sub_id, - const struct vmci_event_data *ed, - void *client_data); static void vmci_transport_peer_detach_cb(u32 sub_id, const struct vmci_event_data *ed, void *client_data); static void vmci_transport_recv_pkt_work(struct work_struct *work); +static void vmci_transport_cleanup(struct work_struct *work); static int vmci_transport_recv_listen(struct sock *sk, struct vmci_transport_packet *pkt); static int vmci_transport_recv_connecting_server( @@ -75,6 +73,10 @@ struct vmci_transport_recv_pkt_info { struct vmci_transport_packet pkt; }; +static LIST_HEAD(vmci_transport_cleanup_list); +static DEFINE_SPINLOCK(vmci_transport_cleanup_lock); +static DECLARE_WORK(vmci_transport_cleanup_work, vmci_transport_cleanup); + static struct vmci_handle vmci_transport_stream_handle = { VMCI_INVALID_ID, VMCI_INVALID_ID }; static u32 vmci_transport_qp_resumed_sub_id = VMCI_INVALID_ID; @@ -791,44 +793,6 @@ out: return err; } -static void vmci_transport_peer_attach_cb(u32 sub_id, - const struct vmci_event_data *e_data, - void *client_data) -{ - struct sock *sk = client_data; - const struct vmci_event_payload_qp *e_payload; - struct vsock_sock *vsk; - - e_payload = vmci_event_data_const_payload(e_data); - - vsk = vsock_sk(sk); - - /* We don't ask for delayed CBs when we subscribe to this event (we - * pass 0 as flags to vmci_event_subscribe()). VMCI makes no - * guarantees in that case about what context we might be running in, - * so it could be BH or process, blockable or non-blockable. So we - * need to account for all possible contexts here. - */ - local_bh_disable(); - bh_lock_sock(sk); - - /* XXX This is lame, we should provide a way to lookup sockets by - * qp_handle. - */ - if (vmci_handle_is_equal(vmci_trans(vsk)->qp_handle, - e_payload->handle)) { - /* XXX This doesn't do anything, but in the future we may want - * to set a flag here to verify the attach really did occur and - * we weren't just sent a datagram claiming it was. - */ - goto out; - } - -out: - bh_unlock_sock(sk); - local_bh_enable(); -} - static void vmci_transport_handle_detach(struct sock *sk) { struct vsock_sock *vsk; @@ -871,28 +835,38 @@ static void vmci_transport_peer_detach_cb(u32 sub_id, const struct vmci_event_data *e_data, void *client_data) { - struct sock *sk = client_data; + struct vmci_transport *trans = client_data; const struct vmci_event_payload_qp *e_payload; - struct vsock_sock *vsk; e_payload = vmci_event_data_const_payload(e_data); - vsk = vsock_sk(sk); - if (vmci_handle_is_invalid(e_payload->handle)) - return; - - /* Same rules for locking as for peer_attach_cb(). */ - local_bh_disable(); - bh_lock_sock(sk); /* XXX This is lame, we should provide a way to lookup sockets by * qp_handle. */ - if (vmci_handle_is_equal(vmci_trans(vsk)->qp_handle, - e_payload->handle)) - vmci_transport_handle_detach(sk); + if (vmci_handle_is_invalid(e_payload->handle) || + vmci_handle_is_equal(trans->qp_handle, e_payload->handle)) + return; - bh_unlock_sock(sk); - local_bh_enable(); + /* We don't ask for delayed CBs when we subscribe to this event (we + * pass 0 as flags to vmci_event_subscribe()). VMCI makes no + * guarantees in that case about what context we might be running in, + * so it could be BH or process, blockable or non-blockable. So we + * need to account for all possible contexts here. + */ + spin_lock_bh(&trans->lock); + if (!trans->sk) + goto out; + + /* Apart from here, trans->lock is only grabbed as part of sk destruct, + * where trans->sk isn't locked. + */ + bh_lock_sock(trans->sk); + + vmci_transport_handle_detach(trans->sk); + + bh_unlock_sock(trans->sk); + out: + spin_unlock_bh(&trans->lock); } static void vmci_transport_qp_resumed_cb(u32 sub_id, @@ -1181,7 +1155,7 @@ vmci_transport_recv_connecting_server(struct sock *listener, */ err = vmci_event_subscribe(VMCI_EVENT_QP_PEER_DETACH, vmci_transport_peer_detach_cb, - pending, &detach_sub_id); + vmci_trans(vpending), &detach_sub_id); if (err < VMCI_SUCCESS) { vmci_transport_send_reset(pending, pkt); err = vmci_transport_error_to_vsock_error(err); @@ -1321,7 +1295,6 @@ vmci_transport_recv_connecting_client(struct sock *sk, || vmci_trans(vsk)->qpair || vmci_trans(vsk)->produce_size != 0 || vmci_trans(vsk)->consume_size != 0 - || vmci_trans(vsk)->attach_sub_id != VMCI_INVALID_ID || vmci_trans(vsk)->detach_sub_id != VMCI_INVALID_ID) { skerr = EPROTO; err = -EINVAL; @@ -1389,7 +1362,6 @@ static int vmci_transport_recv_connecting_client_negotiate( struct vsock_sock *vsk; struct vmci_handle handle; struct vmci_qp *qpair; - u32 attach_sub_id; u32 detach_sub_id; bool is_local; u32 flags; @@ -1399,7 +1371,6 @@ static int vmci_transport_recv_connecting_client_negotiate( vsk = vsock_sk(sk); handle = VMCI_INVALID_HANDLE; - attach_sub_id = VMCI_INVALID_ID; detach_sub_id = VMCI_INVALID_ID; /* If we have gotten here then we should be past the point where old @@ -1444,23 +1415,15 @@ static int vmci_transport_recv_connecting_client_negotiate( goto destroy; } - /* Subscribe to attach and detach events first. + /* Subscribe to detach events first. * * XXX We attach once for each queue pair created for now so it is easy * to find the socket (it's provided), but later we should only * subscribe once and add a way to lookup sockets by queue pair handle. */ - err = vmci_event_subscribe(VMCI_EVENT_QP_PEER_ATTACH, - vmci_transport_peer_attach_cb, - sk, &attach_sub_id); - if (err < VMCI_SUCCESS) { - err = vmci_transport_error_to_vsock_error(err); - goto destroy; - } - err = vmci_event_subscribe(VMCI_EVENT_QP_PEER_DETACH, vmci_transport_peer_detach_cb, - sk, &detach_sub_id); + vmci_trans(vsk), &detach_sub_id); if (err < VMCI_SUCCESS) { err = vmci_transport_error_to_vsock_error(err); goto destroy; @@ -1496,7 +1459,6 @@ static int vmci_transport_recv_connecting_client_negotiate( vmci_trans(vsk)->produce_size = vmci_trans(vsk)->consume_size = pkt->u.size; - vmci_trans(vsk)->attach_sub_id = attach_sub_id; vmci_trans(vsk)->detach_sub_id = detach_sub_id; vmci_trans(vsk)->notify_ops->process_negotiate(sk); @@ -1504,9 +1466,6 @@ static int vmci_transport_recv_connecting_client_negotiate( return 0; destroy: - if (attach_sub_id != VMCI_INVALID_ID) - vmci_event_unsubscribe(attach_sub_id); - if (detach_sub_id != VMCI_INVALID_ID) vmci_event_unsubscribe(detach_sub_id); @@ -1607,9 +1566,11 @@ static int vmci_transport_socket_init(struct vsock_sock *vsk, vmci_trans(vsk)->qp_handle = VMCI_INVALID_HANDLE; vmci_trans(vsk)->qpair = NULL; vmci_trans(vsk)->produce_size = vmci_trans(vsk)->consume_size = 0; - vmci_trans(vsk)->attach_sub_id = vmci_trans(vsk)->detach_sub_id = - VMCI_INVALID_ID; + vmci_trans(vsk)->detach_sub_id = VMCI_INVALID_ID; vmci_trans(vsk)->notify_ops = NULL; + INIT_LIST_HEAD(&vmci_trans(vsk)->elem); + vmci_trans(vsk)->sk = &vsk->sk; + vmci_trans(vsk)->lock = __SPIN_LOCK_UNLOCKED(vmci_trans(vsk)->lock); if (psk) { vmci_trans(vsk)->queue_pair_size = vmci_trans(psk)->queue_pair_size; @@ -1629,29 +1590,57 @@ static int vmci_transport_socket_init(struct vsock_sock *vsk, return 0; } -static void vmci_transport_destruct(struct vsock_sock *vsk) +static void vmci_transport_free_resources(struct list_head *transport_list) { - if (vmci_trans(vsk)->attach_sub_id != VMCI_INVALID_ID) { - vmci_event_unsubscribe(vmci_trans(vsk)->attach_sub_id); - vmci_trans(vsk)->attach_sub_id = VMCI_INVALID_ID; - } + while (!list_empty(transport_list)) { + struct vmci_transport *transport = + list_first_entry(transport_list, struct vmci_transport, + elem); + list_del(&transport->elem); - if (vmci_trans(vsk)->detach_sub_id != VMCI_INVALID_ID) { - vmci_event_unsubscribe(vmci_trans(vsk)->detach_sub_id); - vmci_trans(vsk)->detach_sub_id = VMCI_INVALID_ID; - } + if (transport->detach_sub_id != VMCI_INVALID_ID) { + vmci_event_unsubscribe(transport->detach_sub_id); + transport->detach_sub_id = VMCI_INVALID_ID; + } - if (!vmci_handle_is_invalid(vmci_trans(vsk)->qp_handle)) { - vmci_qpair_detach(&vmci_trans(vsk)->qpair); - vmci_trans(vsk)->qp_handle = VMCI_INVALID_HANDLE; - vmci_trans(vsk)->produce_size = 0; - vmci_trans(vsk)->consume_size = 0; + if (!vmci_handle_is_invalid(transport->qp_handle)) { + vmci_qpair_detach(&transport->qpair); + transport->qp_handle = VMCI_INVALID_HANDLE; + transport->produce_size = 0; + transport->consume_size = 0; + } + + kfree(transport); } +} + +static void vmci_transport_cleanup(struct work_struct *work) +{ + LIST_HEAD(pending); + + spin_lock_bh(&vmci_transport_cleanup_lock); + list_replace_init(&vmci_transport_cleanup_list, &pending); + spin_unlock_bh(&vmci_transport_cleanup_lock); + vmci_transport_free_resources(&pending); +} + +static void vmci_transport_destruct(struct vsock_sock *vsk) +{ + /* Ensure that the detach callback doesn't use the sk/vsk + * we are about to destruct. + */ + spin_lock_bh(&vmci_trans(vsk)->lock); + vmci_trans(vsk)->sk = NULL; + spin_unlock_bh(&vmci_trans(vsk)->lock); if (vmci_trans(vsk)->notify_ops) vmci_trans(vsk)->notify_ops->socket_destruct(vsk); - kfree(vsk->trans); + spin_lock_bh(&vmci_transport_cleanup_lock); + list_add(&vmci_trans(vsk)->elem, &vmci_transport_cleanup_list); + spin_unlock_bh(&vmci_transport_cleanup_lock); + schedule_work(&vmci_transport_cleanup_work); + vsk->trans = NULL; } @@ -2150,6 +2139,9 @@ module_init(vmci_transport_init); static void __exit vmci_transport_exit(void) { + cancel_work_sync(&vmci_transport_cleanup_work); + vmci_transport_free_resources(&vmci_transport_cleanup_list); + if (!vmci_handle_is_invalid(vmci_transport_stream_handle)) { if (vmci_datagram_destroy_handle( vmci_transport_stream_handle) != VMCI_SUCCESS) @@ -2168,6 +2160,7 @@ module_exit(vmci_transport_exit); MODULE_AUTHOR("VMware, Inc."); MODULE_DESCRIPTION("VMCI transport for Virtual Sockets"); +MODULE_VERSION("1.0.2.0-k"); MODULE_LICENSE("GPL v2"); MODULE_ALIAS("vmware_vsock"); MODULE_ALIAS_NETPROTO(PF_VSOCK); diff --git a/net/vmw_vsock/vmci_transport.h b/net/vmw_vsock/vmci_transport.h index ce6c9623d5f0..2ad46f39649f 100644 --- a/net/vmw_vsock/vmci_transport.h +++ b/net/vmw_vsock/vmci_transport.h @@ -119,10 +119,12 @@ struct vmci_transport { u64 queue_pair_size; u64 queue_pair_min_size; u64 queue_pair_max_size; - u32 attach_sub_id; u32 detach_sub_id; union vmci_transport_notify notify; struct vmci_transport_notify_ops *notify_ops; + struct list_head elem; + struct sock *sk; + spinlock_t lock; /* protects sk. */ }; int vmci_transport_register(void);