From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id 9F6975913 for ; Mon, 1 Aug 2016 18:43:47 +0200 (CEST) Received: by mail-wm0-f46.google.com with SMTP id f65so377396128wmi.0 for ; Mon, 01 Aug 2016 09:43:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=IBzHtsmjYgj4YFriQ/SxI3FxsgpI5AM4knSRkcWdUY8=; b=HcWIAaz2zA0FvZ1uCMr4uaBs21WUDoj+Ip6p0pvhvHkXMiaJsqxkS1O1KBVRi5y0hr ViV1pBdspBdNWIleKdAVDpk9fF5nfzvfTJ7i9b2peBliFXTsMpg2E4xj9Xrktc92Qk44 6EEV5mkaPEoxxfactRXReqUBbuYfA0zKK8qaSJkevsrT0shkHT9vmSCdLc5ZiGp1FrLK eVRKDsbIZMuaPyFrxbrjdQyNpqGUKJ/E4mexojUeJBY8+FPwMS5IMt2Jw/5s/lQyVwYL mvInRGWyYqwWWdyTqF/ZgRxFbCH0WO1z5xJrn5ZTk4AW1BGBp0JN+0eWq0Qu1ufeoLCq Kbyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=IBzHtsmjYgj4YFriQ/SxI3FxsgpI5AM4knSRkcWdUY8=; b=lbxBKq/Q8kFrRZdf7+RB4K665UgLIbDw4cWm38FQH5AqIggSPwKSXtlRV3D8IgH7we 5xyQ0wFz+j4M+uEhVCjf/wQD8deE0qkfIj9Q7PYCjFnVUf7FNUgZM6DNYFafu5wYCClV 4rTgaWJFtaYPakG4Ckj44z84zy/def9fxEYAbpKMN+dW3wXPSFwE2OUzCYKcIDyiLaAK redm0l5mcfqTgfIiC03DfU+9ivCQIHcUrW4gSVr2dX/SbHl0FqqHvEs0ivgZVux9ZEEc +G+qDKuVCH+So4uOTK1ZU/+okHuQuTVJiZMy/G0XACyz9GRTR8N4L9HUkdupbBhHm806 wwhQ== X-Gm-Message-State: AEkooutbxnbwOgPaSK+7ZiBMP6EuZaVBY4njxcsvUVNuvUwkhpA5hcPtm/VCk/8WwUwmAAaw X-Received: by 10.28.171.214 with SMTP id u205mr38454227wme.97.1470069827325; Mon, 01 Aug 2016 09:43:47 -0700 (PDT) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id c139sm17907338wme.4.2016.08.01.09.43.46 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 01 Aug 2016 09:43:46 -0700 (PDT) Date: Mon, 1 Aug 2016 18:43:42 +0200 From: Adrien Mazarguil To: Sagi Grimberg Cc: dev@dpdk.org Message-ID: <20160801164342.GL9044@6wind.com> Mail-Followup-To: Sagi Grimberg , dev@dpdk.org References: <1470041061-8059-1-git-send-email-sagi@grimberg.me> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1470041061-8059-1-git-send-email-sagi@grimberg.me> 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Aug 2016 16:43:47 -0000 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") > Signed-off-by: Sagi Grimberg > --- > drivers/net/mlx5/mlx5_rxtx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c > index fce3381ae87a..a07cc4794023 100644 > --- a/drivers/net/mlx5/mlx5_rxtx.c > +++ b/drivers/net/mlx5/mlx5_rxtx.c > @@ -1572,7 +1572,7 @@ 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)) { > - while (pkt != seg) { > + while (pkt && pkt != seg) { > assert(pkt != (*rxq->elts)[idx]); > seg = NEXT(pkt); > rte_mbuf_refcnt_set(pkt, 0); > -- > 1.9.1 I've reviewed your patch and it indeed seems to address an issue, please confirm my analysis below. When rep cannot be allocated and is thus NULL, either pkt is still NULL because the first packet segment has not been seen yet or points to the first segment. Either way at this point, seg points to current segment to process in the queue and is never NULL. Thus when pkt is still NULL (first segment) and rep cannot be allocated, the comparison (pkt != seg) between a valid pointer (seg) and NULL (pkt) succeeds. This case is not handled by the assert() statement and a crash occurs. We really want to avoid useless code in the data path, particularly inside loops. The extra check you added is performed for each iteration, so what about modifying your patch by adding the following if statement instead? if (!pkt) pkt = seg; while (pkt != seg) { ... } I guess you could add "Fixes: a1bdb71a32da ("net/mlx5: fix crash in Rx")" line to your commit log as well because the original patch only solved half of the issue. Thanks. -- Adrien Mazarguil 6WIND