So just to make sure I'm understanding correctly,
1) revalidator calls dpif_flow_dump_next_may_destroy_keys(), which
indicates that it will not destroy the buffer.
2) revalidator calls dpif_flow_dump_next(), which dumps a flow without
actions. The most likely case is that this is the last flow in the batch.
3) dpif_flow_dump_next() attempts to retrieve the actions, but fails.
4) dpif_flow_dump_next() continues in the loop, and fetches a new batch via
nl_dump_next(), breaking the guarantee that may_destroy_keys() gave.
Then, the proposed solution is to eliminate flow deletion batching.
I have a couple of thoughts:-
Could we fix it instead by changing the
dpif_linux_flow_dump_next_may_destroy_keys() to copy the ofpbuf, seek ahead
and check if actions exist so that it provides the proper guarantee?
Alternatively, if you're more inclined towards this solution, is there any
point in keeping may_destroy_keys() at all? The main reason the function
exists is to allow batching.
On 22 May 2014 09:39, Ben Pfaff <b...@nicira.com> wrote:
> I forgot to mention that this is for branch-2.3, since this is already
> fixed on master.
>
> On Wed, May 21, 2014 at 02:33:51PM -0700, Ben Pfaff wrote:
> > Commit 1340ce0c175 (ofproto-dpif-upcall: Avoid use-after-free in
> > revalidate() corner cases.) fixed one use-after-free error in
> revalidate(),
> > but missed one more subtle case, in which dpif_linux_flow_dump_next()
> > attempts to retrieve actions for a flow that didn't have them in the main
> > dump result. If retrieving those actions fails,
> > dpif_linux_flow_dump_next() goes on to the next flow, and as part of that
> > overwrites the old dumped flows in its buffer. This is a problem because
> > dpif_linux_flow_dump_next_may_destroy_keys() would have indicated that
> > the old dumped flows would not have been destroyed, which means that the
> > data the caller relied on has gone away. In the worst case, this causes
> > a segfault and core dump.
> >
> > The best way to fix this problem is the refactoring that has already
> > happened on master in commit ac64794acb85 (dpif: Refactor flow dumping
> > interface to make better sense for batching.) That is a fairly large
> > change, and not yet well-tested, so it doesn't yet seem suitable for a
> > stable branch. For now, this commit simply turns off batching of the
> > deletions that happen during revalidation. If this proves in practice to
> > hurt performance too much (e.g. in a TCP_CRR netperf when megaflows don't
> > avoid per-microflow upcalls), we can always backport the refactoring.
> >
> > Bug #1249988.
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > lib/dpif.c | 15 +++------------
> > ofproto/ofproto-dpif-upcall.c | 30 ++++++++----------------------
> > 2 files changed, 11 insertions(+), 34 deletions(-)
> >
> > diff --git a/lib/dpif.c b/lib/dpif.c
> > index 41b8eb7..677816f 100644
> > --- a/lib/dpif.c
> > +++ b/lib/dpif.c
> > @@ -1065,18 +1065,9 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump,
> void *state,
> > return !error;
> > }
> >
> > -/* Determines whether the next call to 'dpif_flow_dump_next' for 'dump'
> and
> > - * 'state' will modify or free the keys that it previously returned.
> 'state'
> > - * must have been initialized by a call to 'dpif_flow_dump_state_init'
> for
> > - * 'dump'.
> > - *
> > - * 'dpif' guarantees that data returned by flow_dump_next() will remain
> > - * accessible and unchanging until the next call. This function
> provides a way
> > - * for callers to determine whether that guarantee extends beyond the
> next
> > - * call.
> > - *
> > - * Returns true if the next call to flow_dump_next() is expected to be
> > - * destructive to previously returned keys for 'state', false
> otherwise. */
> > +/* Returns true if the next call to flow_dump_next() is expected to be
> > + * destructive to previously returned keys for 'state', false otherwise.
> > + * The return value is only a heuristic; the caller must not rely on
> it. */
> > bool
> > dpif_flow_dump_next_may_destroy_keys(struct dpif_flow_dump *dump, void
> *state)
> > {
> > diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> > index 193f4cd..c8ec22c 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -1434,16 +1434,13 @@ revalidate(struct revalidator *revalidator)
> > {
> > struct udpif *udpif = revalidator->udpif;
> >
> > - struct dump_op ops[REVALIDATE_MAX_BATCH];
> > const struct nlattr *key, *mask, *actions;
> > size_t key_len, mask_len, actions_len;
> > const struct dpif_flow_stats *stats;
> > long long int now;
> > unsigned int flow_limit;
> > - size_t n_ops;
> > void *state;
> >
> > - n_ops = 0;
> > now = time_msec();
> > atomic_read(&udpif->flow_limit, &flow_limit);
> >
> > @@ -1451,7 +1448,7 @@ revalidate(struct revalidator *revalidator)
> > while (dpif_flow_dump_next(&udpif->dump, state, &key, &key_len,
> &mask,
> > &mask_len, &actions, &actions_len,
> &stats)) {
> > struct udpif_key *ukey;
> > - bool mark, may_destroy;
> > + bool mark;
> > long long int used, max_idle;
> > uint32_t hash;
> > size_t n_flows;
> > @@ -1508,31 +1505,20 @@ revalidate(struct revalidator *revalidator)
> > }
> >
> > if (!mark) {
> > - dump_op_init(&ops[n_ops++], key, key_len, ukey);
> > + struct dump_op op;
> > +
> > + dump_op_init(&op, key, key_len, ukey);
> > + push_dump_ops__(udpif, &op, 1);
> > }
> >
> > next:
> > - may_destroy = dpif_flow_dump_next_may_destroy_keys(&udpif->dump,
> > - state);
> > -
> > - /* Only update 'now' immediately before 'buffer' will be
> updated.
> > + /* Only update 'now' immediately before 'buffer' will be updated
> > + * (typically, at least, since this function only yields a
> heuristic).
> > * This gives us the current time relative to the time the
> datapath
> > * will write into 'stats'. */
> > - if (may_destroy) {
> > + if (dpif_flow_dump_next_may_destroy_keys(&udpif->dump, state)) {
> > now = time_msec();
> > }
> > -
> > - /* Only do a dpif_operate when we've hit our maximum batch, or
> when our
> > - * memory is about to be clobbered by the next call to
> > - * dpif_flow_dump_next(). */
> > - if (n_ops == REVALIDATE_MAX_BATCH || (n_ops && may_destroy)) {
> > - push_dump_ops__(udpif, ops, n_ops);
> > - n_ops = 0;
> > - }
> > - }
> > -
> > - if (n_ops) {
> > - push_dump_ops__(udpif, ops, n_ops);
> > }
> >
> > dpif_flow_dump_state_uninit(udpif->dpif, state);
> > --
> > 1.7.10.4
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
