From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f48.google.com (mail-wm0-f48.google.com [74.125.82.48]) by dpdk.org (Postfix) with ESMTP id 0FD54201 for ; Wed, 26 Jul 2017 11:34:10 +0200 (CEST) Received: by mail-wm0-f48.google.com with SMTP id t201so67122062wmt.1 for ; Wed, 26 Jul 2017 02:34:10 -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:references:mime-version :content-disposition:in-reply-to; bh=yVJvAxsXcc7Rz46MJBDW0VpzhnMyiRbWs4WDWiY6N6E=; b=gfSBxK0RaIZubUE80U6LV3crzLR8zBvVFNdpxOyA6pedrKejUwB5xBeg7A2dwxUvfP 2cAStypHMZAOzzI5/mS1vJmD4K8tswepNyuVB6njafYpa9CA4kmhVwD1FnOhTLEWa/Kz Q+Hg3yQf+lQsAQJhMv0IF0yY8FqzIm5X9RSNCa3awehcqSLuE+hR3PUnYkEbt2C8RI3M tTjeeUJl/NFPs956hBPBdqB6qKQGZNB35hOxPelPSByMbkPhHKLVWrchVEythdfht8pW 2gHwmiHxtmpFQZvBW6C/yDg71HTpw9koeRtKRVBz3W/v4BmnvLLugIWuQSotXLhE9eMk krSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=yVJvAxsXcc7Rz46MJBDW0VpzhnMyiRbWs4WDWiY6N6E=; b=QY6642hAb3Rng72bqxuCoPVtk8uVxnGNw17uBQSUM9RVTqqe6CfpYCHs8WFmRWf2L3 RzQoN8WDb5aT3ao7XTGOHHOnhvuV+liFlPMKM2TWgOUMTmG7+phdPJYGTp4Z+XTcXExG Keeid929vXScR6oUUwxucTmwiaBzr0QSQMf+K4RIZfG/MvUyI++BRcks6WeeZIx4Mwvm 8p3K0fDZhkZxKLc2CvBt+pRh++l7N2MGPkQOlj3aZ3vedYrRdFMNoeVOA1Vz4amY9klx ABkLUKK3/eDsQESlH5guTf3Zvauk044w+zXLxJ2VM84FGFBfZ7Z71DE/MX2cAD4J71lo hdhg== X-Gm-Message-State: AIVw113Nepq1QANTfhvfHkAeDUo5xPWdtWkoxsb6iCg7PSPCwwhJG+/n pwm5OupRZkMVssRJrKk= X-Received: by 10.28.131.130 with SMTP id f124mr244601wmd.25.1501061650408; Wed, 26 Jul 2017 02:34:10 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id u11sm11419466wma.22.2017.07.26.02.34.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 26 Jul 2017 02:34:09 -0700 (PDT) Date: Wed, 26 Jul 2017 11:34:01 +0200 From: Adrien Mazarguil To: Matan Azrad Cc: dev@dpdk.org, stable@dpdk.org Message-ID: <20170726093401.GU19852@6wind.com> References: <1500981508-13820-1-git-send-email-matan@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1500981508-13820-1-git-send-email-matan@mellanox.com> Subject: Re: [dpdk-dev] [PATCH] net/mlx4: fix drop action setting before start X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Jul 2017 09:34:11 -0000 Hi Matan, On Tue, Jul 25, 2017 at 02:18:28PM +0300, Matan Azrad wrote: > The corrupted code causes segmentation fault when user creates > flow with drop action before device starting. Thanks for debugging this issue. This should address the crash but I'm concerned about its root cause, as you've described: > For example, failsafe PMD recreates all the flows before calling > dev_start in plug-in sequence and mlx4 allocated its flow drop > queue in dev_start. > Hence, when failsafe created flow with drop action after plug-in > event, mlx4 tried to dereference flow drop queue which was uninitialized. So question is, how come the mlx4 PMD attempts to create underlying flows if the device is not yet started? In my opinion it should only record the flow and refrain from applying it (by calling ibv_create_flow()) until the application requests the device to be started. > The fix changed the device private structure to hold the flow drop > queue by value instead of by reference. > Hence, the flow drop queue dynamic allocation and free were removed, and > all the accesses to its internal fields were changed. > > The segmentation fault should not occur anymore because the memory > of flow drop queue is always allocated in configuration time. > > Fixes: 642fe56a1ba5 ("net/mlx4: use a single drop queue for all drop flows") > Cc: stable@dpdk.org > > Signed-off-by: Matan Azrad In the current code this drop queue is unconditionally created by mlx4_priv_flow_start() before applying the remaining flows. So it makes sense to assume no flows are supposed to be applied before the device is started, right? Then the problem is actually when a user calls rte_flow_create(), priv_flow_create_action_queue() does not check the started state of the device before applying the flow, because if it did, the drop queue would have been present. Fixing this behavior is also necessary since it causes the device to receive packets through configured flows even though the application hasn't started it yet. This is a major issue present since the beginning in commit: 46d5736a7049 ("net/mlx4: support basic flow items and actions") Therefore this patch does not address the root cause, I think it should just add a check on the started flag where appropriate. Making the drop queue allocation more static shouldn't be necessary afterward. > --- > drivers/net/mlx4/mlx4.h | 7 ++++++- > drivers/net/mlx4/mlx4_flow.c | 32 +++++++------------------------- > 2 files changed, 13 insertions(+), 26 deletions(-) > > diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h > index a2e0ae7..ecaab35 100644 > --- a/drivers/net/mlx4/mlx4.h > +++ b/drivers/net/mlx4/mlx4.h > @@ -309,6 +309,11 @@ struct txq { > > struct rte_flow; > > +struct rte_flow_drop { > + struct ibv_qp *qp; /**< Verbs queue pair. */ > + struct ibv_cq *cq; /**< Verbs completion queue. */ > +}; > + > struct priv { > struct rte_eth_dev *dev; /* Ethernet device. */ > struct ibv_context *ctx; /* Verbs context. */ > @@ -352,7 +357,7 @@ struct priv { > struct txq *(*txqs)[]; /* TX queues. */ > struct rte_intr_handle intr_handle_dev; /* Device interrupt handler. */ > struct rte_intr_handle intr_handle; /* Interrupt handler. */ > - struct rte_flow_drop *flow_drop_queue; /* Flow drop queue. */ > + struct rte_flow_drop flow_drop_queue; /* Flow drop queue. */ > LIST_HEAD(mlx4_flows, rte_flow) flows; > struct rte_intr_conf intr_conf; /* Active interrupt configuration. */ > LIST_HEAD(mlx4_parents, rxq) parents; > diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c > index b998bb9..a398f46 100644 > --- a/drivers/net/mlx4/mlx4_flow.c > +++ b/drivers/net/mlx4/mlx4_flow.c > @@ -103,11 +103,6 @@ struct mlx4_flow_items { > const enum rte_flow_item_type *const items; > }; > > -struct rte_flow_drop { > - struct ibv_qp *qp; /**< Verbs queue pair. */ > - struct ibv_cq *cq; /**< Verbs completion queue. */ > -}; > - > /** Valid action for this PMD. */ > static const enum rte_flow_action_type valid_actions[] = { > RTE_FLOW_ACTION_TYPE_DROP, > @@ -795,13 +790,9 @@ struct rte_flow_drop { > static void > mlx4_flow_destroy_drop_queue(struct priv *priv) > { > - if (priv->flow_drop_queue) { > - struct rte_flow_drop *fdq = priv->flow_drop_queue; > - > - priv->flow_drop_queue = NULL; > - claim_zero(ibv_destroy_qp(fdq->qp)); > - claim_zero(ibv_destroy_cq(fdq->cq)); > - rte_free(fdq); > + if (priv->flow_drop_queue.cq) { > + claim_zero(ibv_destroy_qp(priv->flow_drop_queue.qp)); > + claim_zero(ibv_destroy_cq(priv->flow_drop_queue.cq)); > } > } > > @@ -819,20 +810,14 @@ struct rte_flow_drop { > { > struct ibv_qp *qp; > struct ibv_cq *cq; > - struct rte_flow_drop *fdq; > > - fdq = rte_calloc(__func__, 1, sizeof(*fdq), 0); > - if (!fdq) { > - ERROR("Cannot allocate memory for drop struct"); > - goto err; > - } > cq = ibv_exp_create_cq(priv->ctx, 1, NULL, NULL, 0, > &(struct ibv_exp_cq_init_attr){ > .comp_mask = 0, > }); > if (!cq) { > ERROR("Cannot create drop CQ"); > - goto err_create_cq; > + goto err; > } > qp = ibv_exp_create_qp(priv->ctx, > &(struct ibv_exp_qp_init_attr){ > @@ -853,16 +838,13 @@ struct rte_flow_drop { > ERROR("Cannot create drop QP"); > goto err_create_qp; > } > - *fdq = (struct rte_flow_drop){ > + priv->flow_drop_queue = (struct rte_flow_drop){ > .qp = qp, > .cq = cq, > }; > - priv->flow_drop_queue = fdq; > return 0; > err_create_qp: > claim_zero(ibv_destroy_cq(cq)); > -err_create_cq: > - rte_free(fdq); > err: > return -1; > } > @@ -977,7 +959,7 @@ struct rte_flow_drop { > return NULL; > } > if (action->drop) { > - qp = priv->flow_drop_queue->qp; > + qp = priv->flow_drop_queue.qp; > } else { > int ret; > unsigned int i; > @@ -1307,7 +1289,7 @@ struct rte_flow * > for (flow = LIST_FIRST(&priv->flows); > flow; > flow = LIST_NEXT(flow, next)) { > - qp = flow->qp ? flow->qp : priv->flow_drop_queue->qp; > + qp = flow->qp ? flow->qp : priv->flow_drop_queue.qp; > flow->ibv_flow = ibv_create_flow(qp, flow->ibv_attr); > if (!flow->ibv_flow) { > DEBUG("Flow %p cannot be applied", (void *)flow); > -- > 1.8.3.1 > -- Adrien Mazarguil 6WIND