diff mbox series

[v4,2/2] net/ice: fix scalar Tx path segment

Message ID 20221111161241.861732-2-mingjinx.ye@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Qi Zhang
Headers
Series [v4,1/2] net/ice: fix scalar Rx path segment |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed

Commit Message

Mingjin Ye Nov. 11, 2022, 4:12 p.m. UTC 
  The scalar Tx path would send empty buffer that causes the Tx queue to
overflow.

This patch adds the last buffer length judgment in tx_prepare to fix this
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(+)

Comments

Qi Zhang Nov. 11, 2022, 9:01 a.m. UTC | #1 
> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Saturday, November 12, 2022 12:13 AM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>; Zhang, Qi
> Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Liu,
> KevinX <kevinx.liu@intel.com>
> Subject: [PATCH v4 2/2] net/ice: fix scalar Tx path segment
> 
> The scalar Tx path would send empty buffer that causes the Tx queue to
> overflow.
> 
> This patch adds the last buffer length judgment in tx_prepare to fix this 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>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi
Ke Xu Nov. 11, 2022, 9:14 a.m. UTC | #2 
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Friday, November 11, 2022 5:02 PM
> To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Ferruh
> Yigit <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Liu,
> KevinX <kevinx.liu@intel.com>
> Subject: RE: [PATCH v4 2/2] net/ice: fix scalar Tx path segment
> 
> 
> 
> > -----Original Message-----
> > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > Sent: Saturday, November 12, 2022 12:13 AM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou,
> > YidingX <yidingx.zhou@intel.com>; Ye, MingjinX
> > <mingjinx.ye@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Liu,
> > KevinX <kevinx.liu@intel.com>
> > Subject: [PATCH v4 2/2] net/ice: fix scalar Tx path segment
> >
> > The scalar Tx path would send empty buffer that causes the Tx queue to
> > overflow.
> >
> > This patch adds the last buffer length judgment in tx_prepare to fix
> > this 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>
> 
> Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> 
> Applied to dpdk-next-net-intel.
> 
> Thanks
> Qi

Tested and passed.
Tested-by: Ke Xu <ke1.xu@intel.com>
David Marchand Sept. 19, 2023, 8:15 a.m. UTC | #3 
Hello,

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.


> +
> +       return 0;
> +}
> +
>  uint16_t
>  ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
>               uint16_t nb_pkts)
David Marchand Sept. 27, 2023, 8:44 a.m. UTC | #4 
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.
diff mbox series

Patch

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*/
+static inline uint16_t
+ice_check_empty_mbuf(struct rte_mbuf *tx_pkt)
+{
+	struct rte_mbuf *txd = tx_pkt;
+
+	while (txd != NULL) {
+		if (txd->data_len == 0)
+			return -1;
+		txd = txd->next;
+	}
+
+	return 0;
+}
+
 uint16_t
 ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	      uint16_t nb_pkts)
@@ -3689,6 +3705,12 @@  ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 			rte_errno = -ret;
 			return i;
 		}
+
+		if (ice_check_empty_mbuf(m) != 0) {
+			rte_errno = EINVAL;
+			PMD_DRV_LOG(ERR, "INVALID mbuf:	last mbuf data_len=[0]");
+			return i;
+		}
 	}
 	return i;
 }