From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: dev@dpdk.org, stable@dpdk.org
Subject: Re: [dpdk-stable] [PATCH] net/mlx4: fix drop action setting before start
Date: Wed, 26 Jul 2017 11:34:01 +0200 [thread overview]
Message-ID: <20170726093401.GU19852@6wind.com> (raw)
In-Reply-To: <1500981508-13820-1-git-send-email-matan@mellanox.com>
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 <matan@mellanox.com>
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
next prev parent reply other threads:[~2017-07-26 9:34 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-25 11:18 Matan Azrad
2017-07-26 9:34 ` Adrien Mazarguil [this message]
2017-07-27 10:27 ` [dpdk-stable] [PATCH v2] net/mlx4: fix flow creation " Matan Azrad
2017-07-31 16:48 ` [dpdk-stable] [dpdk-dev] " Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170726093401.GU19852@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=matan@mellanox.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).