[ovs-dev] [PATCH 2/2] netflow: Avoid (theoretically) looping 2**32 times.

Ben Pfaff blp at nicira.com
Fri Oct 1 21:33:09 UTC 2010


Thanks, I pushed these out.

On Fri, Oct 01, 2010 at 02:28:34PM -0700, Justin Pettit wrote:
> The set looks good.  Thanks.
> 
> --Justin
> 
> 
> On Sep 1, 2010, at 12:46 PM, Ben Pfaff wrote:
> 
> > If the netflow byte counter is UINT64_MAX, or at any rate much larger than
> > UINT32_MAX, netflow_expire() could loop for a very long time.  This commit
> > avoids that case.
> > 
> > This is only a theoretical bug fix.  I don't know of any actual bug that
> > would cause a counter to be that high.
> > ---
> > ofproto/netflow.c |   49 ++++++++++++++++++++++++++++++++-----------------
> > 1 files changed, 32 insertions(+), 17 deletions(-)
> > 
> > diff --git a/ofproto/netflow.c b/ofproto/netflow.c
> > index a70b2fc..4881c5f 100644
> > --- a/ofproto/netflow.c
> > +++ b/ofproto/netflow.c
> > @@ -184,23 +184,38 @@ netflow_expire(struct netflow *nf, struct netflow_flow *nf_flow,
> >         return;
> >     }
> > 
> > -    /* NetFlow v5 records are limited to 32-bit counters.  If we've wrapped
> > -     * a counter, send as multiple records so we don't lose track of any
> > -     * traffic.  We try to evenly distribute the packet and byte counters,
> > -     * so that the bytes-per-packet lengths don't look wonky across the
> > -     * records. */
> > -    while (byte_delta > UINT32_MAX) {
> > -        uint32_t n_recs = byte_delta >> 32;
> > -        uint32_t pkt_count = pkt_delta / n_recs;
> > -        uint32_t byte_count = byte_delta / n_recs;
> > -
> > -        gen_netflow_rec(nf, nf_flow, expired, pkt_count, byte_count);
> > -
> > -        pkt_delta -= pkt_count;
> > -        byte_delta -= byte_count;
> > -    }
> > -    if (byte_delta > 0) {
> > -        gen_netflow_rec(nf, nf_flow, expired, pkt_delta, byte_delta);
> > +    if ((byte_delta >> 32) <= 175) {
> > +        /* NetFlow v5 records are limited to 32-bit counters.  If we've wrapped
> > +         * a counter, send as multiple records so we don't lose track of any
> > +         * traffic.  We try to evenly distribute the packet and byte counters,
> > +         * so that the bytes-per-packet lengths don't look wonky across the
> > +         * records. */
> > +        while (byte_delta > UINT32_MAX) {
> > +            uint32_t n_recs = byte_delta >> 32;
> > +            uint32_t pkt_count = pkt_delta / n_recs;
> > +            uint32_t byte_count = byte_delta / n_recs;
> > +
> > +            gen_netflow_rec(nf, nf_flow, expired, pkt_count, byte_count);
> > +
> > +            pkt_delta -= pkt_count;
> > +            byte_delta -= byte_count;
> > +        }
> > +        if (byte_delta > 0) {
> > +            gen_netflow_rec(nf, nf_flow, expired, pkt_delta, byte_delta);
> > +        }
> > +    } else {
> > +        /* In 600 seconds, a 10GbE link can theoretically transmit 75 * 10**10
> > +         * == 175 * 2**32 bytes.  The byte counter is bigger than that, so it's
> > +         * probably a bug--for example, the netdev code uses UINT64_MAX to
> > +         * report "unknown value", and perhaps that has leaked through to here.
> > +         *
> > +         * We wouldn't want to hit the loop above in this case, because it
> > +         * would try to send up to UINT32_MAX netflow records, which would take
> > +         * a long time.
> > +         */
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> > +
> > +        VLOG_WARN_RL(&rl, "impossible byte counter %"PRIu64, byte_delta);
> >     }
> > 
> >     /* Update flow tracking data. */
> > -- 
> > 1.7.1
> > 
> 




More information about the dev mailing list