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
