The commit message is a bit confusing to me, I don't see the relationship
to commit 1340ce0c175. I see it more as a discrepancy between the behaviour
of dpif_linux_flow_dump_next_may_destroy_keys() and
dpif_linux_flow_dump_next(). However I agree with the rest of the commit
message.
Tested briefly, didn't check it fixes the original bug.
Acked-by: Joe Stringer <joestrin...@nicira.com>
On 22 May 2014 11:26, Ben Pfaff <b...@nicira.com> 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 refines the conditions that
> dpif_linux_flow_dump_next_may_destroy_keys() uses, so that if the next
> flow does not include actions it indicates that keys may be destroyed.
>
> Thanks to Joe Stringer for suggesting this particular solution.
>
> Bug #1249988.
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> lib/dpif-linux.c | 12 ++++++++++--
> lib/netlink-socket.c | 13 +++++++++++++
> lib/netlink-socket.h | 1 +
> 3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> index abb4b51..4af0607 100644
> --- a/lib/dpif-linux.c
> +++ b/lib/dpif-linux.c
> @@ -1256,8 +1256,16 @@ static bool
> dpif_linux_flow_dump_next_may_destroy_keys(void *state_)
> {
> struct dpif_linux_flow_state *state = state_;
> -
> - return ofpbuf_size(&state->buffer) ? false : true;
> + struct dpif_linux_flow flow;
> + struct ofpbuf nlmsg;
> +
> + /* Check whether there's a flow remaining in the buffer that includes
> + * actions. (If it does not include actions, then we could end up
> + * destroying keys previously returned trying to retrieve its actions
> + * fails.) */
> + return (!nl_dump_peek(&nlmsg, &state->buffer)
> + || dpif_linux_flow_from_ofpbuf(&flow, &nlmsg)
> + || !flow.actions);
> }
>
> static int
> diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
> index e4cc4ad..ef476f2 100644
> --- a/lib/netlink-socket.c
> +++ b/lib/netlink-socket.c
> @@ -797,6 +797,19 @@ exit:
> return !error;
> }
>
> +/* Attempts to look ahead in 'buffer' to obtain the next reply that will
> be
> + * returned by nl_dump_next(). Returns true if successful, in which case
> + * 'reply' will be initialize to the message that will be obtained by the
> next
> + * call to nl_dump_next(), or false on failure. Failure doesn't
> necessarily
> + * mean that the nl_dump_next() will fail, only that it needs to obtain a
> new
> + * block of dump results from the kernel. */
> +bool
> +nl_dump_peek(struct ofpbuf *reply, struct ofpbuf *buffer)
> +{
> + struct ofpbuf tmp = *buffer;
> + return nl_msg_next(&tmp, reply);
> +}
> +
> /* Completes Netlink dump operation 'dump', which must have been
> initialized
> * with nl_dump_start(). Returns 0 if the dump operation was error-free,
> * otherwise a positive errno value describing the problem. */
> diff --git a/lib/netlink-socket.h b/lib/netlink-socket.h
> index dd32409..1f06b97 100644
> --- a/lib/netlink-socket.h
> +++ b/lib/netlink-socket.h
> @@ -119,6 +119,7 @@ struct nl_dump {
> void nl_dump_start(struct nl_dump *, int protocol,
> const struct ofpbuf *request);
> bool nl_dump_next(struct nl_dump *, struct ofpbuf *reply, struct ofpbuf
> *buf);
> +bool nl_dump_peek(struct ofpbuf *reply, struct ofpbuf *buf);
> int nl_dump_done(struct nl_dump *);
>
> /* Miscellaneous */
> --
> 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
