Message ID | c8769b2c-cfd2-f457-b550-0580ebc3e5e8@grimberg.me (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id D17193977; Tue, 2 Aug 2016 12:47:59 +0200 (CEST) Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id D66A22C37 for <dev@dpdk.org>; Tue, 2 Aug 2016 12:47:57 +0200 (CEST) Received: by mail-wm0-f66.google.com with SMTP id x83so29959724wma.3 for <dev@dpdk.org>; Tue, 02 Aug 2016 03:47:57 -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:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=EwkR3tpiYeXwSEWM9qXsEsFuM7Ayphg8ULsShtnI3KY=; b=HZAPIoCxwSFFGz8d1rOIZF8jcMq8MBoHbaFYp9BK2ItUscP+78+O9FA/oM9ViRSI+r yva6tMn8YCc3QRTvMQIwKhLsXPeCW1evoMclEUN5zsVpWBOeeOtpAZT44jwx4kW37PRe h1yZayyG0wJvUb1KunDQ6usRrcE5q/9Vwn3SJcxMSoW1AGVX4lPVBddvxriHtuuiPko/ DMGuFnpv2Orp/MFFNaLi/Vy9IX6nd0E07vBNN6ehiCHvnKMggKsUwwG+oS/A/fVWE1+/ xt62wEa8IX7fvmjwPyH0RGhjQkzNTFm7Wuba1iWwVdktD/7DwOvSJLPmztgTu0BSmpCt tR5w== X-Gm-Message-State: AEkooutsWAvf8N76d7xKlklaYo8Twl9HaoPbKnt5JqkyzvksH4Oaz90BFkZ+6+NwJ8QVoQ== X-Received: by 10.28.63.21 with SMTP id m21mr63794193wma.77.1470134877568; Tue, 02 Aug 2016 03:47:57 -0700 (PDT) Received: from [192.168.1.204] (bzq-82-81-101-184.red.bezeqint.net. [82.81.101.184]) by smtp.gmail.com with ESMTPSA id r16sm21447294wme.16.2016.08.02.03.47.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Aug 2016 03:47:56 -0700 (PDT) To: Adrien Mazarguil <adrien.mazarguil@6wind.com> References: <1470041061-8059-1-git-send-email-sagi@grimberg.me> <20160801164342.GL9044@6wind.com> <0e002bcc-017b-8d5e-f820-111f5c3a7b46@grimberg.me> <20160802095852.GB30580@6wind.com> Cc: dev@dpdk.org From: Sagi Grimberg <sagi@grimberg.me> Message-ID: <c8769b2c-cfd2-f457-b550-0580ebc3e5e8@grimberg.me> Date: Tue, 2 Aug 2016 13:47:55 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160802095852.GB30580@6wind.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] net/mlx5: Fix possible NULL deref in RX path X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Sagi Grimberg
Aug. 2, 2016, 10:47 a.m. UTC
On 02/08/16 12:58, Adrien Mazarguil wrote: > On Tue, Aug 02, 2016 at 12:31:35PM +0300, Sagi Grimberg wrote: >> >> >> On 01/08/16 19:43, Adrien Mazarguil wrote: >>> Hi Sagi, >>> >>> On Mon, Aug 01, 2016 at 11:44:21AM +0300, Sagi Grimberg wrote: >>>> The user is allowed to call ->rx_pkt_burst() even without free >>>> mbufs in the pool. In this scenario we'll fail allocating a rep mbuf >>>> on the first iteration (where pkt is still NULL). This would cause us >>>> to deref a NULL pkt (reset refcount and free). >>>> >>>> Fix this by checking the pkt before freeing it. >>> >>> Just to be sure, did you get an actual NULL deref crash here or is that an >>> assumed possibility? >>> >>> I'm asking because this problem was supposed to be addressed by: >>> >>> a1bdb71a32da ("net/mlx5: fix crash in Rx") >> >> I actually got the NULL deref. This happens when the application doesn't >> restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc >> will fail on the first iteration (pkt wasn't assigned) unlike the >> condition handled in a1bdb71a32da. >> >> With this applied, I didn't see the crash. > > Thanks for confirming this, Hey Adrien, I just noticed that I missed the rest of your response in the previous message (pre-coffee mail browsing...) You analysis was on spot. > now what about the different approach I > suggested in my previous message to avoid the extra check in the inner loop: > > if (!pkt) > pkt = seg; > while (pkt != seg) { > ... > } We can go this way, but it looks kinda confusing to set pkt = seg and then iterate on pkt != seg. How about a more explicit approach: -- seg = NEXT(pkt); @@ -1579,7 +1587,6 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n) __rte_mbuf_raw_free(pkt); pkt = seg; } - ++rxq->stats.rx_nombuf; break; } if (!pkt) { -- > Also the fixes line in your commit message? I'll add it in v2. Thanks.
Comments
On Tue, Aug 02, 2016 at 01:47:55PM +0300, Sagi Grimberg wrote: > > > On 02/08/16 12:58, Adrien Mazarguil wrote: > >On Tue, Aug 02, 2016 at 12:31:35PM +0300, Sagi Grimberg wrote: > >> > >> > >>On 01/08/16 19:43, Adrien Mazarguil wrote: > >>>Hi Sagi, > >>> > >>>On Mon, Aug 01, 2016 at 11:44:21AM +0300, Sagi Grimberg wrote: > >>>>The user is allowed to call ->rx_pkt_burst() even without free > >>>>mbufs in the pool. In this scenario we'll fail allocating a rep mbuf > >>>>on the first iteration (where pkt is still NULL). This would cause us > >>>>to deref a NULL pkt (reset refcount and free). > >>>> > >>>>Fix this by checking the pkt before freeing it. > >>> > >>>Just to be sure, did you get an actual NULL deref crash here or is that an > >>>assumed possibility? > >>> > >>>I'm asking because this problem was supposed to be addressed by: > >>> > >>>a1bdb71a32da ("net/mlx5: fix crash in Rx") > >> > >>I actually got the NULL deref. This happens when the application doesn't > >>restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc > >>will fail on the first iteration (pkt wasn't assigned) unlike the > >>condition handled in a1bdb71a32da. > >> > >>With this applied, I didn't see the crash. > > > >Thanks for confirming this, > > Hey Adrien, I just noticed that I missed the rest of > your response in the previous message (pre-coffee mail > browsing...) > > You analysis was on spot. > > >now what about the different approach I > >suggested in my previous message to avoid the extra check in the inner loop: > > > > if (!pkt) > > pkt = seg; > > while (pkt != seg) { > > ... > > } > > We can go this way, but it looks kinda confusing to set pkt = seg and > then iterate on pkt != seg. > > How about a more explicit approach: > -- > diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c > index fce3381ae87a..37573668e43e 100644 > --- a/drivers/net/mlx5/mlx5_rxtx.c > +++ b/drivers/net/mlx5/mlx5_rxtx.c > @@ -1572,6 +1572,14 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, > uint16_t pkts_n) > rte_prefetch0(wqe); > rep = rte_mbuf_raw_alloc(rxq->mp); > if (unlikely(rep == NULL)) { > + ++rxq->stats.rx_nombuf; > + if (!pkt) { > + /* > + * no buffers before we even started, > + * bail out silently. > + */ > + break; > + } > while (pkt != seg) { > assert(pkt != (*rxq->elts)[idx]); > seg = NEXT(pkt); > @@ -1579,7 +1587,6 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, > uint16_t pkts_n) > __rte_mbuf_raw_free(pkt); > pkt = seg; > } > - ++rxq->stats.rx_nombuf; > break; > } > if (!pkt) { > -- Yes, that's also fine. > >Also the fixes line in your commit message? > > I'll add it in v2. Thanks. Go ahead, thanks!
diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c index fce3381ae87a..37573668e43e 100644 --- a/drivers/net/mlx5/mlx5_rxtx.c +++ b/drivers/net/mlx5/mlx5_rxtx.c @@ -1572,6 +1572,14 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n) rte_prefetch0(wqe); rep = rte_mbuf_raw_alloc(rxq->mp); if (unlikely(rep == NULL)) { + ++rxq->stats.rx_nombuf; + if (!pkt) { + /* + * no buffers before we even started, + * bail out silently. + */ + break; + } while (pkt != seg) { assert(pkt != (*rxq->elts)[idx]);