From patchwork Tue Aug 11 14:42:45 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sagi Grimberg X-Patchwork-Id: 6992631 Return-Path: X-Original-To: patchwork-linux-rdma@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id A1C6AC05AC for ; Tue, 11 Aug 2015 14:42:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 6695D205B5 for ; Tue, 11 Aug 2015 14:42:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4E62B205B1 for ; Tue, 11 Aug 2015 14:42:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964952AbbHKOmv (ORCPT ); Tue, 11 Aug 2015 10:42:51 -0400 Received: from mail-wi0-f171.google.com ([209.85.212.171]:37806 "EHLO mail-wi0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964946AbbHKOmu (ORCPT ); Tue, 11 Aug 2015 10:42:50 -0400 Received: by wibhh20 with SMTP id hh20so199413338wib.0 for ; Tue, 11 Aug 2015 07:42:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding; bh=khmMEFZvWnrDgmh5jhzd65leyNOPGUBjHyfpRj0aD0M=; b=ACyQwKkv6sM0hPtx8sRdDXQQge6cpC/HQ/r+4Yvs2O46bludyl5wlNMkqiNFepXH7K 8/roN7Xb0er7Zl3Oqqzu4SbIdR19myfAsHGwX1MWt4rcj6XV47ZdYHlq76OUg56/aDpt 2+CFxjdu+0lfjKLYWaOkdnuopgqy/eOg5geVoKXCO1eVYQdTONAYPg+7KKf2QY+6a5UB A+viz7brwEWJxQsk+/jgV+QxJkjBN44XogZmTuYR6QnI8uVjKVV7DwGmVkcrIYTJHLZE 2t8JqsDJG67yppbd3qB+/+GOD4aokG+t4MLEnfJGJKD2I1E3TSdZ6vM2l3nMYL/GQ5C0 OK0g== X-Gm-Message-State: ALoCoQldY5mpRd+ZCpOIgXB1bhRz90V10RpWDIt9VssXkS6MJwbSs0jEAXZRcC/Jqc3maCn1Z0N5 X-Received: by 10.180.75.233 with SMTP id f9mr35017359wiw.21.1439304168161; Tue, 11 Aug 2015 07:42:48 -0700 (PDT) Received: from [10.223.0.123] ([193.47.165.251]) by smtp.googlemail.com with ESMTPSA id r6sm19235418wiy.13.2015.08.11.07.42.46 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 11 Aug 2015 07:42:47 -0700 (PDT) Subject: Re: [PATCH] IB/srp: Fix possible use-after-free To: Bart Van Assche , Sagi Grimberg , "linux-rdma@vger.kernel.org" References: <1439216574-25936-1-git-send-email-sagig@mellanox.com> <55C8BB38.1060808@sandisk.com> From: Sagi Grimberg Message-ID: <55CA09E5.2070208@dev.mellanox.co.il> Date: Tue, 11 Aug 2015 17:42:45 +0300 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <55C8BB38.1060808@sandisk.com> Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP > With which kernel version has this been observed ? scsi_remove_host() > waits until all outstanding requests have finished. srp_free_ch_ib() is > called either before a SCSI host is registered with the SCSI core or > after scsi_remove_host() has finished. So I don't see how the above call > trace could be triggered with a recent kernel ? Bart, I think I confused in the patch I sent out. The patch I sent was designed to address a theoretical race when deleting a target during live IO. This specific use-after-free occurred in a reconnect flow where scsi_remove_host() is not invoked (assuming that dev_loss_tmo was not invoked). The below patch should address the same race in the reconnect flow: [PATCH] IB/srp: Fix possible protection fault srp_destroy_qp is designed to indicate we are safe to continue with freeing the channel resources by modifying the qp error state, posting a dummy wr on the queue-pair and waiting for it to flush. This also holds for the channel registration pool as we are unmapping the memory region when handling a scsi response. Destroying the channel registration pool before we make sure we processed all the inflight IO might introduce a use-after-free of the registration pool. This use-after-free is demonstrated in the stack trace below where srp is trying to unmap a used FMR after the fmr_pool was already destroyed. general protection fault: 0000 [#1] SMP RIP: 0010:[] [] _raw_spin_lock_irqsave+0x1b/0x50 Call Trace: [] ib_fmr_pool_unmap+0x1a/0xb0 [ib_core] [] srp_unmap_data.isra.28+0x17d/0x250 [ib_srp] [] srp_free_req+0x2b/0x60 [ib_srp] [] srp_recv_completion+0x174/0x580 [ib_srp] [] mlx4_eq_int+0x4de/0xe50 [mlx4_core] [] mlx4_msi_x_interrupt+0x10/0x20 [mlx4_core] [] handle_irq_event_percpu+0x35/0x1b0 [] handle_irq_event+0x32/0x50 [] handle_edge_irq+0x6f/0x120 [] handle_irq+0x1a/0x30 [] do_IRQ+0x45/0xb0 [] common_interrupt+0x6d/0x6d [] cpuidle_enter_state+0x4f/0xc0 [] cpuidle_idle_call+0xcc/0x210 [] arch_cpu_idle+0xa/0x30 [] cpu_startup_entry+0xe1/0x270 [] start_secondary+0x21a/0x2c0 Reported-by: Eliott Kespi Signed-off-by: Sagi Grimberg Reviewed-by: Bart Van Assche --- drivers/infiniband/ulp/srp/ib_srp.c | 22 +++++++++++----------- 1 files changed, 11 insertions(+), 11 deletions(-) Sorry for the mixup. Does this patch make more sense? Sagi. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 3a1514c..b220856 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -546,6 +546,17 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch) if (ret) goto err_qp; + if (ch->qp) + srp_destroy_qp(ch); + if (ch->recv_cq) + ib_destroy_cq(ch->recv_cq); + if (ch->send_cq) + ib_destroy_cq(ch->send_cq); + + ch->qp = qp; + ch->recv_cq = recv_cq; + ch->send_cq = send_cq; + if (dev->use_fast_reg && dev->has_fr) { fr_pool = srp_alloc_fr_pool(target); if (IS_ERR(fr_pool)) { @@ -570,17 +581,6 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch) ch->fmr_pool = fmr_pool; } - if (ch->qp) - srp_destroy_qp(ch); - if (ch->recv_cq) - ib_destroy_cq(ch->recv_cq); - - ch->qp = qp; - ch->recv_cq = recv_cq; - ch->send_cq = send_cq; - kfree(init_attr); return 0; --