| Message ID | 20200727170838.1101775-2-mcascell@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c | expand |
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. > *
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. > > * >
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. >>> *
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
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 >>
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. *
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(+)