[1/8] net/bnxt: remove assert for zero data len in Tx path

  From: Somnath Kotur <somnath.kotur@broadcom.com>

Currently the PMD tries to detect a potential 0 byte DMA by
using RTE_VERIFY.
But since RTE_VERIFY internally calls rte_panic() it is fatal to
the application and some applications want to avoid that.
So return an error from the bnxt xmit handler if such a bad pkt is
encountered by logging an error message, dumping the pkt header and
dump the current stack as well

Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)
  

On 6/15/2022 3:56 PM, Kalesh A P wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> 
> 
> From: Somnath Kotur <somnath.kotur@broadcom.com>
> 
> Currently the PMD tries to detect a potential 0 byte DMA by
> using RTE_VERIFY.
> But since RTE_VERIFY internally calls rte_panic() it is fatal to
> the application and some applications want to avoid that.
> So return an error from the bnxt xmit handler if such a bad pkt is
> encountered by logging an error message, dumping the pkt header and
> dump the current stack as well
> 
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>   drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
>   1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> index 7a7196a..67e0167 100644
> --- a/drivers/net/bnxt/bnxt_txr.c
> +++ b/drivers/net/bnxt/bnxt_txr.c
> @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
>          return false;
>   }
> 
> +static bool
> +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
> +{
> +       const char *type_str = "Data len";
> +       uint16_t len_to_check = tx_pkt->data_len;
> +
> +       if (data_len_chk == 0) {
> +               type_str = "TSO Seg size";
> +               len_to_check = tx_pkt->tso_segsz;
> +       }
> +
> +       if (len_to_check == 0) {
> +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
> +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
> +               rte_dump_stack();
> +               return true;
> +       }
> +       return false;
> +}
> +
>   static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>                                  struct bnxt_tx_queue *txq,
>                                  uint16_t *coal_pkts,
> @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>          }
> 
>          /* Check non zero data_len */
> -       RTE_VERIFY(tx_pkt->data_len);
> +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
> +               return -EIO;
> 

Some PMDs does the similar verification in the 'rte_eth_tx_prepare()' 
API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx 
data path code, do you want to do the same?
  

On Thu, Jun 16, 2022 at 10:03 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>
> On 6/15/2022 3:56 PM, Kalesh A P wrote:
> > CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> >
> >
> > From: Somnath Kotur <somnath.kotur@broadcom.com>
> >
> > Currently the PMD tries to detect a potential 0 byte DMA by
> > using RTE_VERIFY.
> > But since RTE_VERIFY internally calls rte_panic() it is fatal to
> > the application and some applications want to avoid that.
> > So return an error from the bnxt xmit handler if such a bad pkt is
> > encountered by logging an error message, dumping the pkt header and
> > dump the current stack as well
> >
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > ---
> >   drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
> >   1 file changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> > index 7a7196a..67e0167 100644
> > --- a/drivers/net/bnxt/bnxt_txr.c
> > +++ b/drivers/net/bnxt/bnxt_txr.c
> > @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
> >          return false;
> >   }
> >
> > +static bool
> > +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
> > +{
> > +       const char *type_str = "Data len";
> > +       uint16_t len_to_check = tx_pkt->data_len;
> > +
> > +       if (data_len_chk == 0) {
> > +               type_str = "TSO Seg size";
> > +               len_to_check = tx_pkt->tso_segsz;
> > +       }
> > +
> > +       if (len_to_check == 0) {
> > +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
> > +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
> > +               rte_dump_stack();
> > +               return true;
> > +       }
> > +       return false;
> > +}
> > +
> >   static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >                                  struct bnxt_tx_queue *txq,
> >                                  uint16_t *coal_pkts,
> > @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >          }
> >
> >          /* Check non zero data_len */
> > -       RTE_VERIFY(tx_pkt->data_len);
> > +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
> > +               return -EIO;
> >
>
> Some PMDs does the similar verification in the 'rte_eth_tx_prepare()'
> API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx
> data path code, do you want to do the same?


When we originally added these checks, we were not sure how prevalent
is the usage of tx_pkt_prepare() dev_op by various applications.

We will stick with this patch for now  and implement that
rte_eth_tx_prepare() in the next release?

Thanks

>
  

On 6/20/2022 12:09 AM, Ajit Khaparde wrote:
> On Thu, Jun 16, 2022 at 10:03 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>>
>> On 6/15/2022 3:56 PM, Kalesh A P wrote:
>>>
>>> From: Somnath Kotur <somnath.kotur@broadcom.com>
>>>
>>> Currently the PMD tries to detect a potential 0 byte DMA by
>>> using RTE_VERIFY.
>>> But since RTE_VERIFY internally calls rte_panic() it is fatal to
>>> the application and some applications want to avoid that.
>>> So return an error from the bnxt xmit handler if such a bad pkt is
>>> encountered by logging an error message, dumping the pkt header and
>>> dump the current stack as well
>>>
>>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>> ---
>>>    drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
>>>    1 file changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
>>> index 7a7196a..67e0167 100644
>>> --- a/drivers/net/bnxt/bnxt_txr.c
>>> +++ b/drivers/net/bnxt/bnxt_txr.c
>>> @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
>>>           return false;
>>>    }
>>>
>>> +static bool
>>> +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
>>> +{
>>> +       const char *type_str = "Data len";
>>> +       uint16_t len_to_check = tx_pkt->data_len;
>>> +
>>> +       if (data_len_chk == 0) {
>>> +               type_str = "TSO Seg size";
>>> +               len_to_check = tx_pkt->tso_segsz;
>>> +       }
>>> +
>>> +       if (len_to_check == 0) {
>>> +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
>>> +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
>>> +               rte_dump_stack();
>>> +               return true;
>>> +       }
>>> +       return false;
>>> +}
>>> +
>>>    static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>>>                                   struct bnxt_tx_queue *txq,
>>>                                   uint16_t *coal_pkts,
>>> @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>>>           }
>>>
>>>           /* Check non zero data_len */
>>> -       RTE_VERIFY(tx_pkt->data_len);
>>> +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
>>> +               return -EIO;
>>>
>>
>> Some PMDs does the similar verification in the 'rte_eth_tx_prepare()'
>> API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx
>> data path code, do you want to do the same?
> 
> 
> When we originally added these checks, we were not sure how prevalent
> is the usage of tx_pkt_prepare() dev_op by various applications.
> 
> We will stick with this patch for now  and implement that
> rte_eth_tx_prepare() in the next release?
> 

'rte_eth_tx_prepare()' is not mandatory, so yes there may be 
applications that are not calling this API.

If we have a consensus to have checks in the prepare function, I think 
it helps both to PMDs and applications.
  

On Mon, Jun 20, 2022 at 3:55 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>
> On 6/20/2022 12:09 AM, Ajit Khaparde wrote:
> > On Thu, Jun 16, 2022 at 10:03 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
> >>
> >> On 6/15/2022 3:56 PM, Kalesh A P wrote:
> >>>
> >>> From: Somnath Kotur <somnath.kotur@broadcom.com>
> >>>
> >>> Currently the PMD tries to detect a potential 0 byte DMA by
> >>> using RTE_VERIFY.
> >>> But since RTE_VERIFY internally calls rte_panic() it is fatal to
> >>> the application and some applications want to avoid that.
> >>> So return an error from the bnxt xmit handler if such a bad pkt is
> >>> encountered by logging an error message, dumping the pkt header and
> >>> dump the current stack as well
> >>>
> >>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> >>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> >>> ---
> >>>    drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
> >>>    1 file changed, 26 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> >>> index 7a7196a..67e0167 100644
> >>> --- a/drivers/net/bnxt/bnxt_txr.c
> >>> +++ b/drivers/net/bnxt/bnxt_txr.c
> >>> @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
> >>>           return false;
> >>>    }
> >>>
> >>> +static bool
> >>> +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
> >>> +{
> >>> +       const char *type_str = "Data len";
> >>> +       uint16_t len_to_check = tx_pkt->data_len;
> >>> +
> >>> +       if (data_len_chk == 0) {
> >>> +               type_str = "TSO Seg size";
> >>> +               len_to_check = tx_pkt->tso_segsz;
> >>> +       }
> >>> +
> >>> +       if (len_to_check == 0) {
> >>> +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
> >>> +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
> >>> +               rte_dump_stack();
> >>> +               return true;
> >>> +       }
> >>> +       return false;
> >>> +}
> >>> +
> >>>    static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >>>                                   struct bnxt_tx_queue *txq,
> >>>                                   uint16_t *coal_pkts,
> >>> @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >>>           }
> >>>
> >>>           /* Check non zero data_len */
> >>> -       RTE_VERIFY(tx_pkt->data_len);
> >>> +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
> >>> +               return -EIO;
> >>>
> >>
> >> Some PMDs does the similar verification in the 'rte_eth_tx_prepare()'
> >> API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx
> >> data path code, do you want to do the same?
> >
> >
> > When we originally added these checks, we were not sure how prevalent
> > is the usage of tx_pkt_prepare() dev_op by various applications.
> >
> > We will stick with this patch for now  and implement that
> > rte_eth_tx_prepare() in the next release?
> >
>
> 'rte_eth_tx_prepare()' is not mandatory, so yes there may be
> applications that are not calling this API.
>
> If we have a consensus to have checks in the prepare function, I think
> it helps both to PMDs and applications.
I agree.
Once the changes for tx_pkt_prepare() are ready,
we will remove the checks from the Tx burst handler.
BNXT PMD will carry the current checks till then.