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

[David Marchand <david.marchand@redhat.com>]

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