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
