Thanks for the review. Ordinarily I would spend some time refining the
commit message to better explain, because I like commit messages to be
clear to everyone, but I am feeling a lot of pressure to fix this bug,
so I pushed it as is.
On Thu, May 22, 2014 at 02:24:37PM +1200, Joe Stringer wrote:
> 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
