And now branch-1.3.
On Fri, Dec 16, 2011 at 10:14:56AM -0800, Ben Pfaff wrote:
> Thank you. I pushed this to master and branch-1.4.
>
> On Thu, Dec 15, 2011 at 05:31:35PM -0800, Ethan Jackson wrote:
> > Looks good.
> >
> > I'm planning to get rid of this function entirely in the fairly near future.
> >
> > Ethan
> >
> > On Thu, Dec 15, 2011 at 16:24, Ben Pfaff <b...@nicira.com> wrote:
> > > When a flow consists solely of an output to OFPP_CONTROLLER, we avoid a
> > > round trip to the kernel and back by calling execute_controller_action()
> > > from handle_flow_miss(). ?However, execute_controller_action() frees the
> > > packet passed in. ?This is dangerous, because the packet and the upcall
> > > key are in the same block of malloc()'d memory, as the comment on struct
> > > dpif_upcall says:
> > >
> > > /* A packet passed up from the datapath to userspace.
> > > ?*
> > > ?* If 'key' or 'actions' is nonnull, then it points into data owned by
> > > ?* 'packet', so their memory cannot be freed separately. ?(This is hardly
> > > a
> > > ?* great way to do things but it works out OK for the dpif providers and
> > > ?* clients that exist so far.)
> > > ?*/
> > >
> > > Thus, we get a use-after-free later on in handle_flow_miss() and
> > > eventually
> > > a double free.
> > >
> > > This fixes the problem by making execute_controller_action() clone the
> > > packet in this case.
> > > ---
> > > ?ofproto/ofproto-dpif.c | ? 16 +++++++++++-----
> > > ?1 files changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > > index e68bec3..1043a5d 100644
> > > --- a/ofproto/ofproto-dpif.c
> > > +++ b/ofproto/ofproto-dpif.c
> > > @@ -318,7 +318,7 @@ static bool execute_controller_action(struct
> > > ofproto_dpif *,
> > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const struct flow *,
> > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const struct nlattr *odp_actions,
> > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? size_t actions_len,
> > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct ofpbuf *packet);
> > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct ofpbuf *packet, bool clone);
> > >
> > > ?static void facet_flush_stats(struct ofproto_dpif *, struct facet *);
> > >
> > > @@ -2533,7 +2533,7 @@ handle_flow_miss(struct ofproto_dpif *ofproto,
> > > struct flow_miss *miss,
> > > ? ? ? ? }
> > > ? ? ? ? if (!execute_controller_action(ofproto, &facet->flow,
> > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?subfacet->actions,
> > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? subfacet->actions_len, packet)) {
> > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? subfacet->actions_len, packet,
> > > true)) {
> > > ? ? ? ? ? ? struct flow_miss_op *op = &ops[(*n_ops)++];
> > > ? ? ? ? ? ? struct dpif_execute *execute = &op->dpif_op.execute;
> > >
> > > @@ -3059,11 +3059,17 @@ facet_free(struct facet *facet)
> > > ? ? free(facet);
> > > ?}
> > >
> > > +/* If the 'actions_len' bytes of actions in 'odp_actions' are just a
> > > single
> > > + * OVS_ACTION_ATTR_USERSPACE action, executes it internally and returns
> > > true.
> > > + * Otherwise, returns false without doing anything.
> > > + *
> > > + * If 'clone' is true, the caller always retains ownership of 'packet'.
> > > + * Otherwise, ownership is transferred to this function if it returns
> > > true. */
> > > ?static bool
> > > ?execute_controller_action(struct ofproto_dpif *ofproto,
> > > ? ? ? ? ? ? ? ? ? ? ? ? ? const struct flow *flow,
> > > ? ? ? ? ? ? ? ? ? ? ? ? ? const struct nlattr *odp_actions, size_t
> > > actions_len,
> > > - ? ? ? ? ? ? ? ? ? ? ? ? ?struct ofpbuf *packet)
> > > + ? ? ? ? ? ? ? ? ? ? ? ? ?struct ofpbuf *packet, bool clone)
> > > ?{
> > > ? ? if (actions_len
> > > ? ? ? ? && odp_actions->nla_type == OVS_ACTION_ATTR_USERSPACE
> > > @@ -3079,7 +3085,7 @@ execute_controller_action(struct ofproto_dpif
> > > *ofproto,
> > >
> > > ? ? ? ? nla = nl_attr_find_nested(odp_actions,
> > > OVS_USERSPACE_ATTR_USERDATA);
> > > ? ? ? ? send_packet_in_action(ofproto, packet, nl_attr_get_u64(nla), flow,
> > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?false);
> > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?clone);
> > > ? ? ? ? return true;
> > > ? ? } else {
> > > ? ? ? ? return false;
> > > @@ -3100,7 +3106,7 @@ execute_odp_actions(struct ofproto_dpif *ofproto,
> > > const struct flow *flow,
> > > ? ? int error;
> > >
> > > ? ? if (execute_controller_action(ofproto, flow, odp_actions, actions_len,
> > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?packet)) {
> > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?packet, false)) {
> > > ? ? ? ? return true;
> > > ? ? }
> > >
> > > --
> > > 1.7.2.5
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
