1. Patchew
  2. QEMU
  3. [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c
  4. View patch

[PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags

Mauro Matteo Cascella posted 2 patches 5 years, 6 months ago
Maintainers: Jason Wang <jasowang@redhat.com>, Dmitry Fleytman <dmitry.fleytman@gmail.com>
[PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
Posted by Mauro Matteo Cascella 5 years, 6 months ago 
This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
current data fragment against the maximum number of data fragments.

Reported-by: Ziming Zhang <ezrakiez@gmail.com>
Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
---
 hw/net/net_tx_pkt.c | 5 +++++
 hw/net/net_tx_pkt.h | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
index 9560e4a49e..d035618f2c 100644
--- a/hw/net/net_tx_pkt.c
+++ b/hw/net/net_tx_pkt.c
@@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
     }
 }
 
+bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
+{
+    return pkt->raw_frags >= pkt->max_raw_frags;
+}
+
 bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
 {
     return pkt->raw_frags > 0;
diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
index 4ec8bbe9bd..e2ee46ae03 100644
--- a/hw/net/net_tx_pkt.h
+++ b/hw/net/net_tx_pkt.h
@@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
  */
 bool net_tx_pkt_parse(struct NetTxPkt *pkt);
 
+/**
+* indicates if the current data fragment exceeds max_raw_frags
+*
+* @pkt:            packet
+*
+*/
+bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
+
 /**
 * indicates if there are data fragments held by this packet object.
 *
-- 
2.26.2
Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
Posted by Jason Wang 5 years, 6 months ago 
On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
> This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
> current data fragment against the maximum number of data fragments.


I wonder whether it's better to do the check in 
net_tx_pkt_add_raw_fragment() and fail there.

Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when 
returning to true, is this a bug?

Thanks


>
> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> ---
>   hw/net/net_tx_pkt.c | 5 +++++
>   hw/net/net_tx_pkt.h | 8 ++++++++
>   2 files changed, 13 insertions(+)
>
> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> index 9560e4a49e..d035618f2c 100644
> --- a/hw/net/net_tx_pkt.c
> +++ b/hw/net/net_tx_pkt.c
> @@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
>       }
>   }
>   
> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
> +{
> +    return pkt->raw_frags >= pkt->max_raw_frags;
> +}
> +
>   bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
>   {
>       return pkt->raw_frags > 0;
> diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> index 4ec8bbe9bd..e2ee46ae03 100644
> --- a/hw/net/net_tx_pkt.h
> +++ b/hw/net/net_tx_pkt.h
> @@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
>    */
>   bool net_tx_pkt_parse(struct NetTxPkt *pkt);
>   
> +/**
> +* indicates if the current data fragment exceeds max_raw_frags
> +*
> +* @pkt:            packet
> +*
> +*/
> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
> +
>   /**
>   * indicates if there are data fragments held by this packet object.
>   *
Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
Posted by Mauro Matteo Cascella 5 years, 6 months ago 
On Tue, Jul 28, 2020 at 6:06 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
> > This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
> > current data fragment against the maximum number of data fragments.
>
>
> I wonder whether it's better to do the check in
> net_tx_pkt_add_raw_fragment() and fail there.

Given the assertion, I assumed the caller is responsible for the
check, but moving the check in net_tx_pkt_add_raw_fragment() totally
makes sense to me.

> Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when
> returning to true, is this a bug?

Isn't it unmapped in net_tx_pkt_reset()?

> Thanks
>
>
> >
> > Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > ---
> >   hw/net/net_tx_pkt.c | 5 +++++
> >   hw/net/net_tx_pkt.h | 8 ++++++++
> >   2 files changed, 13 insertions(+)
> >
> > diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> > index 9560e4a49e..d035618f2c 100644
> > --- a/hw/net/net_tx_pkt.c
> > +++ b/hw/net/net_tx_pkt.c
> > @@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
> >       }
> >   }
> >
> > +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
> > +{
> > +    return pkt->raw_frags >= pkt->max_raw_frags;
> > +}
> > +
> >   bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
> >   {
> >       return pkt->raw_frags > 0;
> > diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> > index 4ec8bbe9bd..e2ee46ae03 100644
> > --- a/hw/net/net_tx_pkt.h
> > +++ b/hw/net/net_tx_pkt.h
> > @@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
> >    */
> >   bool net_tx_pkt_parse(struct NetTxPkt *pkt);
> >
> > +/**
> > +* indicates if the current data fragment exceeds max_raw_frags
> > +*
> > +* @pkt:            packet
> > +*
> > +*/
> > +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
> > +
> >   /**
> >   * indicates if there are data fragments held by this packet object.
> >   *
>

-- 
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0
Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
Posted by Jason Wang 5 years, 6 months ago 
On 2020/7/29 上午12:26, Mauro Matteo Cascella wrote:
> On Tue, Jul 28, 2020 at 6:06 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
>>> This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
>>> current data fragment against the maximum number of data fragments.
>>
>> I wonder whether it's better to do the check in
>> net_tx_pkt_add_raw_fragment() and fail there.
> Given the assertion, I assumed the caller is responsible for the
> check, but moving the check in net_tx_pkt_add_raw_fragment() totally
> makes sense to me.


Want to send a new version for this?


>
>> Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when
>> returning to true, is this a bug?
> Isn't it unmapped in net_tx_pkt_reset()?


Probably but see how it was used in e1000e, the net_tx_pkt_reset() is 
only called when eop is set. Is this a bug?

Thanks

>
>> Thanks
>>
>>
>>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
>>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>>> ---
>>>    hw/net/net_tx_pkt.c | 5 +++++
>>>    hw/net/net_tx_pkt.h | 8 ++++++++
>>>    2 files changed, 13 insertions(+)
>>>
>>> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
>>> index 9560e4a49e..d035618f2c 100644
>>> --- a/hw/net/net_tx_pkt.c
>>> +++ b/hw/net/net_tx_pkt.c
>>> @@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
>>>        }
>>>    }
>>>
>>> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
>>> +{
>>> +    return pkt->raw_frags >= pkt->max_raw_frags;
>>> +}
>>> +
>>>    bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
>>>    {
>>>        return pkt->raw_frags > 0;
>>> diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
>>> index 4ec8bbe9bd..e2ee46ae03 100644
>>> --- a/hw/net/net_tx_pkt.h
>>> +++ b/hw/net/net_tx_pkt.h
>>> @@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
>>>     */
>>>    bool net_tx_pkt_parse(struct NetTxPkt *pkt);
>>>
>>> +/**
>>> +* indicates if the current data fragment exceeds max_raw_frags
>>> +*
>>> +* @pkt:            packet
>>> +*
>>> +*/
>>> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
>>> +
>>>    /**
>>>    * indicates if there are data fragments held by this packet object.
>>>    *
Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
Posted by Mauro Matteo Cascella 5 years, 6 months ago 
On Thu, Jul 30, 2020 at 7:28 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/7/29 上午12:26, Mauro Matteo Cascella wrote:
> > On Tue, Jul 28, 2020 at 6:06 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
> >>> This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
> >>> current data fragment against the maximum number of data fragments.
> >>
> >> I wonder whether it's better to do the check in
> >> net_tx_pkt_add_raw_fragment() and fail there.
> > Given the assertion, I assumed the caller is responsible for the
> > check, but moving the check in net_tx_pkt_add_raw_fragment() totally
> > makes sense to me.
>
>
> Want to send a new version for this?

Sure, I'll send a new version. Thank you.

>
> >
> >> Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when
> >> returning to true, is this a bug?
> > Isn't it unmapped in net_tx_pkt_reset()?
>
>
> Probably but see how it was used in e1000e, the net_tx_pkt_reset() is
> only called when eop is set. Is this a bug?

Yeah it all depends on E1000_TXD_CMD_EOP. Besides, if not set,
e1000e_tx_pkt_send() would never be called. Honestly, I don't know if
this is a reasonable scenario or not.

> Thanks
>
> >
> >> Thanks
> >>
> >>
> >>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> >>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> >>> ---
> >>>    hw/net/net_tx_pkt.c | 5 +++++
> >>>    hw/net/net_tx_pkt.h | 8 ++++++++
> >>>    2 files changed, 13 insertions(+)
> >>>
> >>> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> >>> index 9560e4a49e..d035618f2c 100644
> >>> --- a/hw/net/net_tx_pkt.c
> >>> +++ b/hw/net/net_tx_pkt.c
> >>> @@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
> >>>        }
> >>>    }
> >>>
> >>> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
> >>> +{
> >>> +    return pkt->raw_frags >= pkt->max_raw_frags;
> >>> +}
> >>> +
> >>>    bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
> >>>    {
> >>>        return pkt->raw_frags > 0;
> >>> diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> >>> index 4ec8bbe9bd..e2ee46ae03 100644
> >>> --- a/hw/net/net_tx_pkt.h
> >>> +++ b/hw/net/net_tx_pkt.h
> >>> @@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
> >>>     */
> >>>    bool net_tx_pkt_parse(struct NetTxPkt *pkt);
> >>>
> >>> +/**
> >>> +* indicates if the current data fragment exceeds max_raw_frags
> >>> +*
> >>> +* @pkt:            packet
> >>> +*
> >>> +*/
> >>> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
> >>> +
> >>>    /**
> >>>    * indicates if there are data fragments held by this packet object.
> >>>    *
>

--
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0
Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
Posted by Jason Wang 5 years, 6 months ago 
On 2020/7/31 上午1:05, Mauro Matteo Cascella wrote:
> On Thu, Jul 30, 2020 at 7:28 AM Jason Wang<jasowang@redhat.com>  wrote:
>> On 2020/7/29 上午12:26, Mauro Matteo Cascella wrote:
>>> On Tue, Jul 28, 2020 at 6:06 AM Jason Wang<jasowang@redhat.com>  wrote:
>>>> On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
>>>>> This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
>>>>> current data fragment against the maximum number of data fragments.
>>>> I wonder whether it's better to do the check in
>>>> net_tx_pkt_add_raw_fragment() and fail there.
>>> Given the assertion, I assumed the caller is responsible for the
>>> check, but moving the check in net_tx_pkt_add_raw_fragment() totally
>>> makes sense to me.
>> Want to send a new version for this?
> Sure, I'll send a new version. Thank you.
>
>>>> Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when
>>>> returning to true, is this a bug?
>>> Isn't it unmapped in net_tx_pkt_reset()?
>> Probably but see how it was used in e1000e, the net_tx_pkt_reset() is
>> only called when eop is set. Is this a bug?
> Yeah it all depends on E1000_TXD_CMD_EOP. Besides, if not set,
> e1000e_tx_pkt_send() would never be called. Honestly, I don't know if
> this is a reasonable scenario or not.


It's probably fine since anyway e1000e_core_reset() will call 
net_tx_pkt_reset().

Thanks


>
>> Thanks
>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.