From patchwork Tue Jan 5 11:33:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matan Barak X-Patchwork-Id: 7954101 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 C1EF1BEEE5 for ; Tue, 5 Jan 2016 11:33:21 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id C7DF0202FE for ; Tue, 5 Jan 2016 11:33:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8D3A6202E6 for ; Tue, 5 Jan 2016 11:33:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751856AbcAELdS (ORCPT ); Tue, 5 Jan 2016 06:33:18 -0500 Received: from mail-oi0-f66.google.com ([209.85.218.66]:36305 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751620AbcAELdR convert rfc822-to-8bit (ORCPT ); Tue, 5 Jan 2016 06:33:17 -0500 Received: by mail-oi0-f66.google.com with SMTP id o124so25466948oia.3 for ; Tue, 05 Jan 2016 03:33:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dev-mellanox-co-il.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=QZu/E2V3fA6Xyc0bacFSBUvy0WlnnjdRSaczKM5/tB8=; b=1H8q5+sOnG+kvZ9H60ZaQST/SRKBsZX4lhVQC7nbU9+YiYJywndm/LMvvoxwlQLHnx 9XQyKCOhMscVtd68kBRSmold7odPu07/VLF99lucbn8CP8bEWQGAG5pOmZK55haWobMI BLeEY5HAgSjLpDzMiUi+i0LVh34gIH9rfE2yK7d1N4Xhn3tkmaBGqATlL7dd2+3vnmc1 md0gvn4F+3I90bRBrUiHxxhsSlmfF4Kj73/9tbXc5T9R1lyFWwUJNSPM/FsyMXK2p903 4IxoghFQ93tdVyP/XI0eaVVTShVmz1qKzWFincMJCJTogBBf4XrcMMvQdNgvnCGjEMUc MU8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=QZu/E2V3fA6Xyc0bacFSBUvy0WlnnjdRSaczKM5/tB8=; b=GNPA9daz03cJwo5VcIEQj7+dMM4Iim2o3xWt2uQIKhWkVhq0M7bRG8woP5XetQeh2d q+7KtneZpAe6AXMuwf/wk9iVn0cckP4SAe2w9JThgrgg76MXDPVRrU5vJBKpnSFC27JM XUJxhQOFuY0k0t3lo6sYa3xX35vmKYSOCN20NsQNc/5wGqNJibG39DKCThcUgeAh0g/N wk7hc0vURs9GwOUC18K4AWr/goHEcCJ3Q9wWhgpk3TpUrclWDLcV6CV7Mb+30rQqrSeu qNSeYKoqrwqfT1T6Ait1ebLr4W2CqFzR/OaFnuq1Wjzz6u7cXGfsPNdVbcseE9VwsI41 IDqw== X-Gm-Message-State: ALoCoQkcE0Qs7tB2GdVVwYcqgrvHZ+jgXl0xptU1L70KBvPwHpN4P5XS1NtSIAoJiR3/AAQD6/LwrEGfnaIf9TWd/CAkJpegIg== MIME-Version: 1.0 X-Received: by 10.202.58.136 with SMTP id h130mr56868382oia.89.1451993595333; Tue, 05 Jan 2016 03:33:15 -0800 (PST) Received: by 10.202.71.6 with HTTP; Tue, 5 Jan 2016 03:33:15 -0800 (PST) In-Reply-To: References: <1444568298-17289-1-git-send-email-matanb@mellanox.com> <1444568298-17289-2-git-send-email-matanb@mellanox.com> <1828884A29C6694DAF28B7E6B8A82373A9734356@ORSMSX109.amr.corp.intel.com> <1828884A29C6694DAF28B7E6B8A82373A9734A57@ORSMSX109.amr.corp.intel.com> <1828884A29C6694DAF28B7E6B8A82373A9735914@ORSMSX109.amr.corp.intel.com> <567AFE42.2080107@redhat.com> Date: Tue, 5 Jan 2016 13:33:15 +0200 Message-ID: Subject: Re: [PATCH rdma-RC] IB/cm: Fix sleeping while atomic when creating AH from WC From: Matan Barak To: Doug Ledford , Yishai Hadas Cc: "Hefty, Sean" , Matan Barak , "linux-rdma@vger.kernel.org" , Or Gerlitz , Eran Ben Elisha , Jason Gunthorpe Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,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 On Thu, Dec 24, 2015 at 9:46 AM, Matan Barak wrote: > On Wed, Dec 23, 2015 at 10:04 PM, Doug Ledford wrote: >> On 10/15/2015 12:58 PM, Hefty, Sean wrote: >>>>>> ib_create_ah_from_wc needs to resolve the DMAC in order to create the >>>>>> AH (this may result sending an ARP and waiting for response). >>>>>> CM uses this function (which is now sleepable). >>>>> >>>>> This is a significant change to the CM. The CM calls are invoked >>>> assuming that they return relatively quickly. They're invoked from >>>> callbacks and internally. Having the calls now wait for an ARP response >>>> requires that this be re-architected, so the calling thread doesn't go out >>>> to lunch for several seconds. >>>> >>>> Agree - this is a significant change, but it was done a long time ago >>>> (at v4.3 if I recall). When we need to send a message we need to >>> >>> We're at 4.3-rc5? >>> >>>> figure out the destination MAC. Even the passive side needs to do that >>>> as some vendors don't report the source MAC of the packet in their wc. >>>> Even if they did, since IP based addressing is rout-able by its >>>> nature, it should follow the networking stack rules. Some crazy >>>> configurations could force sending responses to packets that came from >>>> router1 to router2 - so we have no choice than resolving the DMAC at >>>> every side. >>> >>> Ib_create_ah_from_wc is broken. It is now an asynchronous operation, only the call itself was left as synchronous. We can't block kernel threads for a minute, or however long ARP takes to resolve. The call itself must change to be async, and all users of it updated to allocate some request, queue it, and handle all race conditions that result -- such as state changes or destruction of the work that caused the request to be initiated. >>> >> >> I don't know who had intended to address this, but it got left out of >> the 4.4 work. We need to not let this drop through the cracks (for >> another release). Can someone please put fixing this properly on their >> TODO list? >> > > IMHO, the proposed patch makes things better. Not applying the current > patch means we have a "sleeping while atomic" error (in addition to > the fact that kernel threads could wait until the ARP process > finishes), which is pretty bad. I tend to agree that adding another CM > state is probably a better approach, but unless someone steps up and > add this for v4.5, I think that's the best thing we have. > >> -- >> Doug Ledford >> GPG KeyID: 0E572FDD >> >> > > Matan Yishai has found a double free bug in the error flow of this patch. The fix is pretty simple. Thanks Yishai for catching and testing this fix. Doug, if you intend to take this patch. I can squash this fix and respin it. Thanks, Matan --- 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/core/cm.c b/drivers/infiniband/core/cm.c index 07a3bbf..832674f 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -296,10 +296,9 @@ static int _cm_alloc_response_msg(struct cm_port *port, 0, IB_MGMT_MAD_HDR, IB_MGMT_MAD_DATA, GFP_ATOMIC, IB_MGMT_BASE_VERSION); - if (IS_ERR(m)) { - ib_destroy_ah(ah); + if (IS_ERR(m)) return PTR_ERR(m); - } + m->ah = ah; *msg = m; return 0; @@ -310,13 +309,18 @@ static int cm_alloc_response_msg(struct cm_port *port, struct ib_mad_send_buf **msg) { struct ib_ah *ah; + int ret; ah = ib_create_ah_from_wc(port->mad_agent->qp->pd, mad_recv_wc->wc, mad_recv_wc->recv_buf.grh, port->port_num); if (IS_ERR(ah)) return PTR_ERR(ah); - return _cm_alloc_response_msg(port, mad_recv_wc, ah, msg); + ret = _cm_alloc_response_msg(port, mad_recv_wc, ah, msg); + if (ret) + ib_destroy_ah(ah); + + return ret; } static void cm_free_msg(struct ib_mad_send_buf *msg)