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

[David Marchand <david.marchand@redhat.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