qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] net: e1000e: fix an infinite loop issue


From: Li Qiang
Subject: Re: [Qemu-devel] [PATCH] net: e1000e: fix an infinite loop issue
Date: Wed, 8 Feb 2017 17:30:05 +0800

Hello,

2017-02-08 16:38 GMT+08:00 Dmitry Fleytman <address@hidden>:

> Hello,
>
> Thanks for the patch!
>
> The problem of infinite loop indeed exists in e1000e, however it is
> different from the one fixed in e1000.
> Please find my comments inline.
>
>
If I read the code correctly, I think this issue is the same. Could you
please explain more? Thanks.


> ~Dmitry
>
> > On 7 Feb 2017, at 11:43 AM, Li Qiang <address@hidden> wrote:
> >
> > From: Li Qiang <address@hidden>
> >
> > This issue is the same as e1000 network card in this commit:
> > e1000: eliminate infinite loops on out-of-bounds transfer start.
> >
> > Signed-off-by: Li Qiang <address@hidden>
> > ---
> > hw/net/e1000e_core.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > index 2b11499..53f2b1d 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -914,7 +914,8 @@ e1000e_start_xmit(E1000ECore *core, const
> E1000E_TxRing *txr)
> >     struct e1000_tx_desc desc;
> >     bool ide = false;
> >     const E1000E_RingInfo *txi = txr->i;
> > -    uint32_t cause = E1000_ICS_TXQE;
> > +    uint32_t tdh_start = core->mac[txi->dh], cause = E1000_ICS_TXQE;
> > +
> >
> >     if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
> >         trace_e1000e_tx_disabled();
> > @@ -933,6 +934,14 @@ e1000e_start_xmit(E1000ECore *core, const
> E1000E_TxRing *txr)
> >         cause |= e1000e_txdesc_writeback(core, base, &desc, &ide,
> txi->idx);
> >
> >         e1000e_ring_advance(core, txi, 1);
> > +
> > +        /*
> > +         * The following avoid infinite loop, just as the e1000
> > +         */
> > +        if (core->mac[txi->dh] == tdh_start ||
> > +            tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN) {
> > +            break;
> > +            }
>
> Part of this validity check, namely
> tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN)
> already done by e1000e_ring_advance(), therefore not needed here.
>
>
e1000e_ring_advance() check the added r->dh. Not the original one.


> The second part - full wraparound protection is relevant, but it fixes
> more symptoms than the cause.
> The only possible cause for full wraparound is r->dt (tail) value bigger
> or equal to r->dlen (length),
> so I would suggest to check for this in e1000e_ring_empty() and cover both
> TX and RX cases at once.
>
>
Do you mean 'core->mac[r->dt] >= core->mac[r->dlen]' indicate an empty ring?


> >     }
> >
> >     if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
> > @@ -1500,6 +1509,7 @@ e1000e_write_packet_to_guest(E1000ECore *core,
> struct NetRxPkt *pkt,
> >     size_t desc_size;
> >     size_t desc_offset = 0;
> >     size_t iov_ofs = 0;
> > +    uint32_t rdh_start;
> >
> >     struct iovec *iov = net_rx_pkt_get_iovec(pkt);
> >     size_t size = net_rx_pkt_get_total_len(pkt);
> > @@ -1509,6 +1519,7 @@ e1000e_write_packet_to_guest(E1000ECore *core,
> struct NetRxPkt *pkt,
> >     bool do_ps = e1000e_do_ps(core, pkt, &ps_hdr_len);
> >
> >     rxi = rxr->i;
> > +    rdh_start = core->mac[rxi->dh];
> >
> >     do {
> >         hwaddr ba[MAX_PS_BUFFERS];
> > @@ -1605,6 +1616,10 @@ e1000e_write_packet_to_guest(E1000ECore *core,
> struct NetRxPkt *pkt,
> >         e1000e_ring_advance(core, rxi,
> >                             core->rx_desc_len / E1000_MIN_RX_DESC_LEN);
> >
> > +        if (core->mac[rxi->dh] == rdh_start ||
> > +            rdh_start >= core->mac[rxi->dlen] / E1000_RING_DESC_LEN) {
> > +            break;
> > +            }
> >     } while (desc_offset < total_size);
> >
> >     e1000e_update_rx_stats(core, size, total_size);
> > --
> > 1.8.3.1
> >
>
>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]