From patchwork Tue May 22 17:51:33 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Ledford X-Patchwork-Id: 10419255 X-Patchwork-Delegate: dledford@redhat.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id F11A9600CC for ; Tue, 22 May 2018 17:52:14 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DF02F28E3E for ; Tue, 22 May 2018 17:52:14 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D18F928F52; Tue, 22 May 2018 17:52:14 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI,T_TVD_MIME_EPI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 11BC728E3E for ; Tue, 22 May 2018 17:52:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751436AbeEVRwL (ORCPT ); Tue, 22 May 2018 13:52:11 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:51360 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751291AbeEVRwJ (ORCPT ); Tue, 22 May 2018 13:52:09 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 61C4D40122D1; Tue, 22 May 2018 17:52:09 +0000 (UTC) Received: from haswell-e.nc.xsintricity.com (ovpn-122-18.rdu2.redhat.com [10.10.122.18]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 382CB2024CAD; Tue, 22 May 2018 17:52:09 +0000 (UTC) Message-ID: Subject: Re: [PATCH] RDMA/ipoib: drop skb on path record lookup failure From: Doug Ledford To: Evgenii Smirnov , linux-rdma@vger.kernel.org Cc: Jason Gunthorpe Date: Tue, 22 May 2018 13:51:33 -0400 In-Reply-To: <20180522140047.25524-1-evgenii.smirnov@profitbricks.com> References: <20180522140047.25524-1-evgenii.smirnov@profitbricks.com> Organization: Red Hat, Inc. Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 22 May 2018 17:52:09 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 22 May 2018 17:52:09 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'dledford@redhat.com' RCPT:'' Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 2018-05-22 at 16:00 +0200, Evgenii Smirnov wrote: > In unicast_arp_send function there is an inconsistency in error handling > of path_rec_start call. If path_rec_start is called because of an absent > ah field, skb will be dropped. But if it is called on a creation of a new path, > or if the path is invalid, skb will be added to the tail of path queue. > In case of a new path it will be dropped on path_free, but in case > of invalid path it can stay in the queue forever. > > This patch unifies the behavior, dropping skb in all cases > of path_rec_start failure. > > Signed-off-by: Evgenii Smirnov > Your patch wouldn't apply (would have needed to be rebased on top of my final patch). So, I hand applied your patch, and simplified things even a little further in the process (if we allocate a new path, just add it right away and greatly simplify the code path, allowing us to entirely eliminate the new_path variable and unwind sequences). Here it is for review: commit 5755dd0e647eecaf3c32af858e9ef69db199b0a7 (HEAD -> k.o/wip/dl-for-next) Author: Evgenii Smirnov Date: Tue May 22 16:00:47 2018 +0200 RDMA/ipoib: drop skb on path record lookup failure In unicast_arp_send function there is an inconsistency in error handling of path_rec_start call. If path_rec_start is called because of an absent ah field, skb will be dropped. But if it is called on a creation of a new path, or if the path is invalid, skb will be added to the tail of path queue. In case of a new path it will be dropped on path_free, but in case of invalid path it can stay in the queue forever. This patch unifies the behavior, dropping skb in all cases of path_rec_start failure. Signed-off-by: Evgenii Smirnov Signed-off-by: Doug Ledford diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index 788bb9573f1f..b84f9e8f881d 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -1054,62 +1054,36 @@ static void unicast_arp_send(struct sk_buff *skb, struct net_device *dev, path = __path_find(dev, phdr->hwaddr + 4); if (!path || !path->ah || !path->ah->valid) { - int new_path = 0; - if (!path) { path = path_rec_create(dev, phdr->hwaddr + 4); - new_path = 1; + if (!path) + goto drop_and_unlock; + __path_add(dev, path); + } + if (!path->query && path_rec_start(dev, path)) { + goto drop_and_unlock; } - if (path) { - if (!new_path) - /* make sure there is no changes in the existing path record */ - init_path_rec(priv, path, phdr->hwaddr + 4); - - if (skb_queue_len(&path->queue) < IPOIB_MAX_PATH_REC_QUEUE) { - push_pseudo_header(skb, phdr->hwaddr); - __skb_queue_tail(&path->queue, skb); - } else { - ++dev->stats.tx_dropped; - dev_kfree_skb_any(skb); - } - if (!path->query && path_rec_start(dev, path)) { - spin_unlock_irqrestore(&priv->lock, flags); - if (new_path) - path_free(dev, path); - return; - } else - __path_add(dev, path); + if (skb_queue_len(&path->queue) < IPOIB_MAX_PATH_REC_QUEUE) { + push_pseudo_header(skb, phdr->hwaddr); + __skb_queue_tail(&path->queue, skb); + goto unlock; } else { goto drop_and_unlock; } - - spin_unlock_irqrestore(&priv->lock, flags); - return; - } - - if (path->ah && path->ah->valid) { - ipoib_dbg(priv, "Send unicast ARP to %08x\n", - be32_to_cpu(sa_path_get_dlid(&path->pathrec))); - - spin_unlock_irqrestore(&priv->lock, flags); - path->ah->last_send = rn->send(dev, skb, path->ah->ah, - IPOIB_QPN(phdr->hwaddr)); - return; - } else if ((path->query || !path_rec_start(dev, path)) && - skb_queue_len(&path->queue) < IPOIB_MAX_PATH_REC_QUEUE) { - push_pseudo_header(skb, phdr->hwaddr); - __skb_queue_tail(&path->queue, skb); - } else { - goto drop_and_unlock; } spin_unlock_irqrestore(&priv->lock, flags); + ipoib_dbg(priv, "Send unicast ARP to %08x\n", + be32_to_cpu(sa_path_get_dlid(&path->pathrec))); + path->ah->last_send = rn->send(dev, skb, path->ah->ah, + IPOIB_QPN(phdr->hwaddr)); return; drop_and_unlock: ++dev->stats.tx_dropped; dev_kfree_skb_any(skb); +unlock: spin_unlock_irqrestore(&priv->lock, flags); }