patches for DPDK stable branches
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: Mingjin Ye <mingjinx.ye@intel.com>,
	qiming.yang@intel.com,  Qi Zhang <qi.z.zhang@intel.com>
Cc: dev@dpdk.org, stable@dpdk.org, yidingx.zhou@intel.com,
	 Jingjing Wu <jingjing.wu@intel.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>,
	 Wenzhuo Lu <wenzhuo.lu@intel.com>,
	Kevin Liu <kevinx.liu@intel.com>
Subject: Re: [PATCH v4 2/2] net/ice: fix scalar Tx path segment
Date: Wed, 27 Sep 2023 10:44:54 +0200	[thread overview]
Message-ID: <CAJFAV8yBjpbnZ9USNVQD3q8qqqXdD8vVOSvN-Psrc9em=O5V4w@mail.gmail.com> (raw)
In-Reply-To: <CAJFAV8yOa3ShkVdEXHfnmOEmUTwV3e75Bu9U3OqpNc5usTt3Rw@mail.gmail.com>

On Tue, Sep 19, 2023 at 10:15 AM David Marchand
<david.marchand@redhat.com> wrote:
> Replying on this old thread, as it seems no in depth review was done
> but I need more info before fixing a bug added by ccf33dccf7aa
> ("net/ice: check illegal packet sizes").
>
>
> On Fri, Nov 11, 2022 at 9:26 AM Mingjin Ye <mingjinx.ye@intel.com> wrote:
> >
> > The scalar Tx path would send empty buffer that causes the Tx queue to
> > overflow.
>
> This is too vague.
> Please describe the exact issue.
> Is it a sw (read net/ice driver) bug? a hw bug?
>
>
> >
> > This patch adds the last buffer length judgment in tx_prepare to fix this
>
> This reads odd.
>
> The added code will stop and return an error when the last segment
> *that is evaluated by the code* is empty.
> But on the other hand, it is easier to understand that the check is to
> catch the *first* empty segment of a packet.
>
>
> > issue, rte_errno will be set to EINVAL and returned if the last buffer is
> > empty.
> >
> > Fixes: 17c7d0f9d6a4 ("net/ice: support basic Rx/Tx")
> > Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> >
> > v3:
> >         * delete unused variable.
> >         * Full detection of mbufs.
> > ---
> >  drivers/net/ice/ice_rxtx.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > index e6ddd2513d..9017353943 100644
> > --- a/drivers/net/ice/ice_rxtx.c
> > +++ b/drivers/net/ice/ice_rxtx.c
> > @@ -3643,6 +3643,22 @@ ice_set_tx_function_flag(struct rte_eth_dev *dev, struct ice_tx_queue *txq)
> >  #define ICE_MIN_TSO_MSS            64
> >  #define ICE_MAX_TSO_MSS            9728
> >  #define ICE_MAX_TSO_FRAME_SIZE     262144
> > +
> > +/*Check for empty mbuf*/
>
> Missing spaces.
>
>
> > +static inline uint16_t
> > +ice_check_empty_mbuf(struct rte_mbuf *tx_pkt)
>
> This function name is confusing.
>
> In dpdk, a mbuf can be both a segment descriptor (part of a packet) or
> a packet descriptor (in which case, it overlaps the first segment
> descriptor).
> So it is better to be explicit with "segment" or "packet" when
> referring to a mbuf.
>
>
> > +{
> > +       struct rte_mbuf *txd = tx_pkt;
> > +
> > +       while (txd != NULL) {
> > +               if (txd->data_len == 0)
> > +                       return -1;
> > +               txd = txd->next;
> > +       }
>
> There is nothing in the API that says that an empty segment is faulty.
>
> With the current description, I understand that any empty segment
> could lead to a bug, regardless of asking offloads (which
> ice_prep_pkts is all about).
> So a more valid fix is probably to skip empty segments in the tx
> handler function(s) and not reject packets in ice_prep_pkts.
> Another option could be to fix this "overflow" mentionned in the commitlog.
>
> Looking fwd to explanations.

Ping.


-- 
David Marchand


      reply	other threads:[~2023-09-27  8:45 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-03 17:20 [PATCH] net/ice: fix scalar Rx and " Mingjin Ye
2022-11-04  6:57 ` Xu, Ke1
2022-11-09 12:56 ` [PATCH v2] " Mingjin Ye
2022-11-10  2:01   ` Xu, Ke1
2022-11-10 10:37   ` Zhang, Qi Z
2022-11-11  3:12     ` Ye, MingjinX
2022-11-11 12:04   ` [PATCH v3 1/2] net/ice: fix scalar Rx " Mingjin Ye
2022-11-11  4:59     ` Zhang, Qi Z
2022-11-11 12:04     ` [PATCH v3 2/2] net/ice: fix scalar Tx " Mingjin Ye
2022-11-11  5:09       ` Zhang, Qi Z
2022-11-11  8:30         ` Ye, MingjinX
2022-11-11  8:45           ` Zhang, Qi Z
2022-11-11 16:01       ` [PATCH v4 1/2] net/ice: fix scalar Rx " Mingjin Ye
2022-11-11 16:01         ` [PATCH v4 2/2] net/ice: fix scalar Tx " Mingjin Ye
2022-11-11 16:12       ` [PATCH v4 1/2] net/ice: fix scalar Rx " Mingjin Ye
2022-11-11  9:03         ` Zhang, Qi Z
2022-11-11  9:13           ` Xu, Ke1
2022-11-11 16:12         ` [PATCH v4 2/2] net/ice: fix scalar Tx " Mingjin Ye
2022-11-11  9:01           ` Zhang, Qi Z
2022-11-11  9:14             ` Xu, Ke1
2023-09-19  8:15           ` David Marchand
2023-09-27  8:44             ` David Marchand [this message]

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='CAJFAV8yBjpbnZ9USNVQD3q8qqqXdD8vVOSvN-Psrc9em=O5V4w@mail.gmail.com' \
    --to=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jingjing.wu@intel.com \
    --cc=kevinx.liu@intel.com \
    --cc=mingjinx.ye@intel.com \
    --cc=qi.z.zhang@intel.com \
    --cc=qiming.yang@intel.com \
    --cc=stable@dpdk.org \
    --cc=wenzhuo.lu@intel.com \
    --cc=yidingx.zhou@intel.com \
    /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).