This is incremental patch to sFlow patch posted. Fixed according to
comments from Jesse.
- Fixed use-after-free bug in sample()
- Fixed comments
- Controller generated packets at undefined in_port are not
sampled
Signed-off-by: Pravin Shelar <pshe...@nicira.com>
---
datapath/actions.c | 29 +++++++--------
datapath/datapath.c | 2 +-
datapath/datapath.h | 4 +-
include/openvswitch/datapath-protocol.h | 14 +++++---
lib/odp-util.c | 17 ++++-----
ofproto/ofproto-dpif-sflow.c | 8 +----
ofproto/ofproto-dpif.c | 57 ++++++++++++++++---------------
7 files changed, 64 insertions(+), 67 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index 6fb6ea6..cba12e6 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -28,7 +28,7 @@
#include "vlan.h"
#include "vport.h"
-static int __do_execute_actions(struct datapath *dp, struct sk_buff *skb,
+static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
const struct nlattr *attr, int len, bool keep_skb);
static int make_writable(struct sk_buff *skb, int write_len)
@@ -265,7 +265,8 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
const struct nlattr *a;
int rem;
- nla_for_each_nested(a, attr, rem) {
+ for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
+ a = nla_next(a, &rem)) {
switch (nla_type(a)) {
case OVS_SAMPLE_ATTR_PROBABILITY:
if (net_random() >= nla_get_u32(a))
@@ -273,16 +274,17 @@ static int sample(struct datapath *dp, struct sk_buff
*skb,
break;
case OVS_SAMPLE_ATTR_ACTIONS:
- acts_list = nla_data(a);
+ acts_list = a;
break;
}
}
- return __do_execute_actions(dp, skb, acts_list, nla_len(acts_list), 1);
+ return do_execute_actions(dp, skb, nla_data(acts_list),
+ nla_len(acts_list), true);
}
/* Execute a list of actions against 'skb'. */
-static int __do_execute_actions(struct datapath *dp, struct sk_buff *skb,
+static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
const struct nlattr *attr, int len, bool keep_skb)
{
/* Every output action needs a separate clone of 'skb', but the common
@@ -371,21 +373,17 @@ static int __do_execute_actions(struct datapath *dp,
struct sk_buff *skb,
}
}
- if (prev_port != -1)
+ if (prev_port != -1) {
+ if (keep_skb)
+ skb = skb_clone(skb, GFP_ATOMIC);
+
do_output(dp, skb, prev_port);
- else if (!keep_skb)
+ } else if (!keep_skb)
consume_skb(skb);
return 0;
}
-
-static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
- struct sw_flow_actions *acts)
-{
- return __do_execute_actions(dp, skb, acts->actions, acts->actions_len,
0);
-}
-
/* Execute a list of actions against 'skb'. */
int execute_actions(struct datapath *dp, struct sk_buff *skb)
{
@@ -404,7 +402,8 @@ int execute_actions(struct datapath *dp, struct sk_buff
*skb)
}
OVS_CB(skb)->tun_id = 0;
- error = do_execute_actions(dp, skb, acts);
+ error = do_execute_actions(dp, skb, acts->actions,
+ acts->actions_len, false);
/* Check whether sub-actions looped too much. */
if (unlikely(loop->looping))
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 189ca39..63a3932 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -544,7 +544,7 @@ static int validate_actions(const struct nlattr *attr, int
depth)
const struct nlattr *a;
int rem, err;
- if (depth >= MAX_ACTIONS_DEPTH)
+ if (depth >= SAMPLE_ACTION_DEPTH)
return -EOVERFLOW;
nla_for_each_nested(a, attr, rem) {
diff --git a/datapath/datapath.h b/datapath/datapath.h
index 361fe6b..ea200a3 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -34,7 +34,7 @@ struct vport;
#define DP_MAX_PORTS 1024
-#define MAX_ACTIONS_DEPTH 3
+#define SAMPLE_ACTION_DEPTH 3
/**
* struct dp_stats_percpu - per-cpu packet processing statistics for a given
@@ -123,7 +123,7 @@ struct ovs_skb_cb {
* struct dp_upcall - metadata to include with a packet to send to userspace
* @cmd: One of %OVS_PACKET_CMD_*.
* @key: Becomes %OVS_PACKET_ATTR_KEY. Must be nonnull.
- * @userdata: Is passed to user-space as %OVS_PACKET_ATTR_USERDATA if cmd is
+ * @userdata: Is passed to user-space as %OVS_PACKET_ATTR_USERDATA if @cmd is
* %OVS_PACKET_CMD_ACTION.
*/
struct dp_upcall_info {
diff --git a/include/openvswitch/datapath-protocol.h
b/include/openvswitch/datapath-protocol.h
index 3f83b23..b29ff0e 100644
--- a/include/openvswitch/datapath-protocol.h
+++ b/include/openvswitch/datapath-protocol.h
@@ -168,6 +168,8 @@ enum ovs_packet_cmd {
* extracted from the packet as nested %OVS_KEY_ATTR_* attributes. This allows
* userspace to adapt its flow setup strategy by comparing its notion of the
* flow key against the kernel's.
+ * @OVS_PACKET_ATTR_ACTIONS: Contains actions for the packet. Used
+ * for %OVS_PACKET_CMD_EXECUTE. It has nested %OVS_ACTION_ATTR_* attributes.
* @OVS_PACKET_ATTR_UPCALL_PID: Optionally present for OVS_PACKET_CMD_EXECUTE.
* The Netlink socket in userspace that OVS_PACKET_CMD_USERSPACE and
* OVS_PACKET_CMD_SAMPLE upcalls will be directed to for actions triggered by
@@ -175,8 +177,7 @@ enum ovs_packet_cmd {
* @OVS_PACKET_ATTR_USERDATA: Present for an %OVS_PACKET_CMD_ACTION
* notification if the %OVS_ACTION_ATTR_USERSPACE, action's argument was
* nonzero.
- * @OVS_PACKET_ATTR_ACTIONS: Contains a copy of the actions for the packet.
Used
- * for %OVS_PACKET_CMD_EXECUTE. It has nested %OVS_ACTION_ATTR_* attributes.
+ *
* These attributes follow the &struct ovs_header within the Generic Netlink
* payload for %OVS_PACKET_* commands.
*/
@@ -184,9 +185,9 @@ enum ovs_packet_attr {
OVS_PACKET_ATTR_UNSPEC,
OVS_PACKET_ATTR_PACKET, /* Packet data. */
OVS_PACKET_ATTR_KEY, /* Nested OVS_KEY_ATTR_* attributes. */
+ OVS_PACKET_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */
OVS_PACKET_ATTR_UPCALL_PID, /* Netlink PID to receive upcalls. */
OVS_PACKET_ATTR_USERDATA, /* u64 OVS_ACTION_ATTR_USERSPACE arg. */
- OVS_PACKET_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */
__OVS_PACKET_ATTR_MAX
};
@@ -416,7 +417,10 @@ enum ovs_flow_attr {
/**
* enum ovs_sample_attr - Attributes for OVS_ACTION_ATTR_SAMPLE
- * @OVS_SAMPLE_ATTR_PROBABILITY: Prabability for sample event.
+ * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample with
+ * @OVS_ACTION_ATTR_SAMPLE. A value of 0 samples no packets, a value of
+ * %UINT32_MAX samples all packets and intermediate values sample intermediate
+ * fractions of packets.
* @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling event.
* Actions are passed as nested attributes.
*/
@@ -447,7 +451,7 @@ enum ovs_action_type {
OVS_ACTION_ATTR_SET_PRIORITY, /* Set skb->priority. */
OVS_ACTION_ATTR_POP_PRIORITY, /* Restore original skb->priority. */
OVS_ACTION_ATTR_SAMPLE, /* Execute list of actions at given
- prabability. */
+ probability. */
__OVS_ACTION_ATTR_MAX
};
diff --git a/lib/odp-util.c b/lib/odp-util.c
index c8ee983..d97d30c 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -32,7 +32,6 @@
#include "packets.h"
#include "timeval.h"
#include "util.h"
-#include "ofproto/ofproto.h"
/* The interface between userspace and kernel uses an "OVS_*" prefix.
* Since this is fairly non-specific for the OVS userspace components,
@@ -94,11 +93,11 @@ format_generic_odp_action(struct ds *ds, const struct
nlattr *a)
static void
format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
{
- static const struct nl_policy ovs_sample_act[] = {
- [OVS_SAMPLE_ATTR_PROBABILITY] = { .type = NL_A_U32, .optional = false
},
- [OVS_SAMPLE_ATTR_ACTIONS] = { .type = NL_A_UNSPEC, .optional = false },
+ static const struct nl_policy ovs_sample_policy[] = {
+ [OVS_SAMPLE_ATTR_PROBABILITY] = { .type = NL_A_U32 },
+ [OVS_SAMPLE_ATTR_ACTIONS] = { .type = NL_A_NESTED }
};
- struct nlattr *a[ARRAY_SIZE(ovs_sample_act)];
+ struct nlattr *a[ARRAY_SIZE(ovs_sample_policy)];
struct ofpbuf buf;
double percentage;
const struct nlattr *nla_acts;
@@ -107,8 +106,8 @@ format_odp_sample_action(struct ds *ds, const struct nlattr
*attr)
ds_put_cstr(ds, "sample");
ofpbuf_use_const(&buf, nl_attr_get(attr), nl_attr_get_size(attr));
- if (!nl_policy_parse(&buf, 0, ovs_sample_act, a,
- ARRAY_SIZE(ovs_sample_act))) {
+ if (!nl_policy_parse(&buf, 0, ovs_sample_policy, a,
+ ARRAY_SIZE(ovs_sample_policy))) {
ds_put_cstr(ds, "(error)");
return;
}
@@ -150,7 +149,7 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
memcpy(&cookie, &data, sizeof(cookie));
if (cookie.type == USER_ACTION_COOKIE_CONTROLLER) {
- ds_put_format(ds, "userspace(controller,data=%"PRIu32")",
+ ds_put_format(ds, "userspace(controller,length=%"PRIu32")",
cookie.data);
} else if (cookie.type == USER_ACTION_COOKIE_SFLOW) {
ds_put_format(ds, "userspace(sFlow,n_output=%"PRIu8","
@@ -158,7 +157,7 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
cookie.n_output, vlan_tci_to_vid(cookie.vlan_tci),
vlan_tci_to_pcp(cookie.vlan_tci), cookie.data);
} else {
- ds_put_format(ds, "userspace(Unknown,data=0x%"PRIx64")",
+ ds_put_format(ds, "userspace(unknown,data=0x%"PRIx64")",
nl_attr_get_u64(a));
}
break;
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index b96d7d2..cc9a5bd 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -503,8 +503,6 @@ dpif_sflow_received(struct dpif_sflow *ds, struct ofpbuf
*packet,
in_dsp = dpif_sflow_find_port(ds, ofp_port_to_odp_port(flow->in_port));
if (!in_dsp) {
- VLOG_WARN_RL(&rl, "No sFlow port for input port (%"PRIu32")",
- flow->in_port);
return;
}
fs.input = SFL_DS_INDEX(in_dsp->dsi);
@@ -514,7 +512,7 @@ dpif_sflow_received(struct dpif_sflow *ds, struct ofpbuf
*packet,
VLOG_WARN_RL(&rl, "netdev get-stats error %s", strerror(error));
return;
}
- fs.sample_pool = stats.rx_packets + stats.tx_packets;
+ fs.sample_pool = stats.rx_packets;
/* We are going to give it to the sampler that represents this input port.
* By implementing "ingress-only" sampling like this we ensure that we
@@ -545,10 +543,6 @@ dpif_sflow_received(struct dpif_sflow *ds, struct ofpbuf
*packet,
switchElem.tag = SFLFLOW_EX_SWITCH;
switchElem.flowType.sw.src_vlan = vlan_tci_to_vid(flow->vlan_tci);
switchElem.flowType.sw.src_priority = vlan_tci_to_pcp(flow->vlan_tci);
- /* Initialize the output VLAN and priority to be the same as the input,
- but these fields can be overriden below if affected by an action. */
- switchElem.flowType.sw.dst_vlan = switchElem.flowType.sw.src_vlan;
- switchElem.flowType.sw.dst_priority = switchElem.flowType.sw.src_priority;
/* Retrieve data from user_action_cookie. */
switchElem.flowType.sw.dst_vlan = vlan_tci_to_vid(cookie->vlan_tci);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 39d9f2f..ba4a29e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -404,9 +404,9 @@ static int expire(struct ofproto_dpif *);
/* Utilities. */
static int send_packet(struct ofproto_dpif *, uint32_t odp_port,
const struct ofpbuf *packet);
-static size_t compose_sflow_action(struct ofpbuf *odp_actions,
- uint32_t probability,
- uint32_t odp_port);
+static size_t
+compose_sflow_action(struct ofproto_dpif *, struct ofpbuf *odp_actions,
+ const struct flow *, uint32_t odp_port);
/* Global variables. */
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -2939,15 +2939,7 @@ send_packet(struct ofproto_dpif *ofproto, uint32_t
odp_port,
odp_flow_key_from_flow(&key, &flow);
ofpbuf_init(&odp_actions, 32);
- if (ofproto->sflow) {
- uint32_t port_ifindex;
- uint32_t probability;
-
- probability = dpif_sflow_get_probability(ofproto->sflow);
- port_ifindex = dpif_sflow_odp_port_to_ifindex(ofproto->sflow,
odp_port);
-
- compose_sflow_action(&odp_actions, probability, port_ifindex);
- }
+ compose_sflow_action(ofproto, &odp_actions, &flow, odp_port);
nl_msg_put_u32(&odp_actions, OVS_ACTION_ATTR_OUTPUT, odp_port);
error = dpif_execute(ofproto->dpif,
@@ -2971,17 +2963,33 @@ static void xlate_normal(struct action_xlate_ctx *);
/* Compose SAMPLE action for sFlow. */
static size_t
-compose_sflow_action(struct ofpbuf *odp_actions,
- uint32_t probability,
- uint32_t port_ifindex)
+compose_sflow_action(struct ofproto_dpif *ofproto,
+ struct ofpbuf *odp_actions,
+ const struct flow *flow,
+ uint32_t odp_port)
{
+ uint32_t port_ifindex;
+ uint32_t probability;
struct user_action_cookie *cookie;
size_t sample_offset, actions_offset;
- int user_cookie_offset;
+ int user_cookie_offset, n_output;
+
+ if (!ofproto->sflow || flow->in_port == OFPP_NONE) {
+ return 0;
+ }
+
+ if (odp_port == OVSP_NONE) {
+ port_ifindex = 0;
+ n_output = 0;
+ } else {
+ port_ifindex = dpif_sflow_odp_port_to_ifindex(ofproto->sflow,
odp_port);
+ n_output = 1;
+ }
sample_offset = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_SAMPLE);
/* Number of packets out of UINT_MAX to sample. */
+ probability = dpif_sflow_get_probability(ofproto->sflow);
nl_msg_put_u32(odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, probability);
actions_offset = nl_msg_start_nested(odp_actions, OVS_SAMPLE_ATTR_ACTIONS);
@@ -2990,7 +2998,7 @@ compose_sflow_action(struct ofpbuf *odp_actions,
sizeof(*cookie));
cookie->type = USER_ACTION_COOKIE_SFLOW;
cookie->data = port_ifindex;
- cookie->n_output = 1;
+ cookie->n_output = n_output;
cookie->vlan_tci = 0;
user_cookie_offset = (char *) cookie - (char *) odp_actions->data;
@@ -3005,16 +3013,9 @@ compose_sflow_action(struct ofpbuf *odp_actions,
static void
add_sflow_action(struct action_xlate_ctx *ctx)
{
- uint32_t probability;
-
- if (!ctx->ofproto->sflow) {
- return;
- }
-
- probability = dpif_sflow_get_probability(ctx->ofproto->sflow);
- ctx->user_cookie_offset = compose_sflow_action(ctx->odp_actions,
- probability, 0);
-
+ ctx->user_cookie_offset = compose_sflow_action(ctx->ofproto,
+ ctx->odp_actions,
+ &ctx->flow, OVSP_NONE);
ctx->sflow_odp_port = 0;
ctx->sflow_n_outputs = 0;
}
@@ -3028,7 +3029,7 @@ fix_sflow_action(struct action_xlate_ctx *ctx)
const struct flow *base = &ctx->base_flow;
struct user_action_cookie *cookie;
- if (!ctx->ofproto->sflow) {
+ if (!ctx->user_cookie_offset) {
return;
}
--
1.7.1
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
