On Mon, Sep 26, 2011 at 4:00 PM, Pravin Shelar <[email protected]> wrote:
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 32e44c0..6fb6ea6 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> +static int sample(struct datapath *dp, struct sk_buff *skb,
> + const struct nlattr *attr)
> +{
> + const struct nlattr *acts_list = NULL;
> + const struct nlattr *a;
> + int rem;
> +
> + nla_for_each_nested(a, attr, rem) {
As a quick optimization, you expand this into a for loop like is used
in do_execute_actions() and drop the unnecessary nla_ok() test since
we've already performed validation.
> + switch (nla_type(a)) {
> + case OVS_SAMPLE_ATTR_PROBABILITY:
> + if (net_random() >= nla_get_u32(a))
> + return 0;
> + break;
> +
> + case OVS_SAMPLE_ATTR_ACTIONS:
> + acts_list = nla_data(a);
The payload of OVS_SAMPLE_ATTR_ACTIONS is the first action. As a
result, when you call nla_len() below, you get the length of the first
action only, not the entire length of the argument list. I think you
really want to store a pointer to a at this point.
> + break;
> + }
> + }
> +
> + return __do_execute_actions(dp, skb, acts_list, nla_len(acts_list),
> 1);
Please use true or false for bool instead of 1 or 0.
> @@ -349,37 +373,17 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
>
> if (prev_port != -1)
> do_output(dp, skb, prev_port);
> - else
> + else if (!keep_skb)
> consume_skb(skb);
Calling do_output() will also consume the skb, so I think you have a
use-after-free problem with the following series of actions:
sample(output), output
> +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);
> }
I don't think this function has much value - you can just directly
call the inner function in both places. The '__' and 'do' prefixes
also seem slightly excessive.
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index 03bebd1..361fe6b 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> @@ -34,6 +34,8 @@ struct vport;
>
> #define DP_MAX_PORTS 1024
>
> +#define MAX_ACTIONS_DEPTH 3
Can you name this something that provides a little more scope, like
SAMPLE_ACTION_DEPTH?
> @@ -127,18 +123,13 @@ 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: Becomes %OVS_PACKET_ATTR_USERDATA if nonzero.
> - * @sample_pool: Becomes %OVS_PACKET_ATTR_SAMPLE_POOL if nonzero.
> - * @actions: Becomes %OVS_PACKET_ATTR_ACTIONS if nonnull.
> - * @actions_len: Number of bytes in @actions.
> -*/
> + * @userdata: Is passed to user-space as %OVS_PACKET_ATTR_USERDATA if cmd is
> + * %OVS_PACKET_CMD_ACTION.
It should @cmd, since it refers to a struct member.
> diff --git a/include/openvswitch/datapath-protocol.h
> b/include/openvswitch/datapath-protocol.h
> index c48426f..3f83b23 100644
> --- a/include/openvswitch/datapath-protocol.h
> +++ b/include/openvswitch/datapath-protocol.h
> @@ -94,10 +94,6 @@ struct ovs_header {
> @@ -181,12 +175,8 @@ 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_SAMPLE_POOL: Present for %OVS_PACKET_CMD_SAMPLE.
> Contains
> - * the number of packets processed so far that were candidates for sampling.
> - * @OVS_PACKET_ATTR_ACTIONS: Present for %OVS_PACKET_CMD_SAMPLE. Contains a
> - * copy of the actions applied to the packet, as nested %OVS_ACTION_ATTR_*
> - * attributes.
> - *
> + * @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.
This comment isn't quite accurate anymore - since the actions are only
used for executed packets they're not really a copy of anything. I
would probably also group this under OVS_PACKET_ATTR_KEY since they
seem related to me.
> +/**
> + * enum ovs_sample_attr - Attributes for OVS_ACTION_ATTR_SAMPLE
> + * @OVS_SAMPLE_ATTR_PROBABILITY: Prabability for sample event.
"Probability".
Can you also describe how this is used represent probability?
> @@ -442,6 +446,8 @@ enum ovs_action_type {
> OVS_ACTION_ATTR_SET_TUNNEL, /* Set the encapsulating tunnel ID. */
> 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".
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 3d94398..c8ee983 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -32,6 +32,7 @@
> #include "packets.h"
> #include "timeval.h"
> #include "util.h"
> +#include "ofproto/ofproto.h"
What do we need ofproto.h for? It seems like a worrisome dependency.
> +static void
> +format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
> +{
> + static const struct nl_policy ovs_sample_act[] = {
What's "act"? Should it be "policy"?
> + [OVS_SAMPLE_ATTR_PROBABILITY] = { .type = NL_A_U32, .optional =
> false },
> + [OVS_SAMPLE_ATTR_ACTIONS] = { .type = NL_A_UNSPEC, .optional = false
> },
The option fields can just be left out. Also, shouldn't
OVS_SAMPLE_ATTR_ACTIONS be NL_A_NESTED?
> @@ -107,7 +146,21 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
> ds_put_format(ds, "%"PRIu16, nl_attr_get_u32(a));
> break;
> case OVS_ACTION_ATTR_USERSPACE:
> - ds_put_format(ds, "userspace(%"PRIu64")", nl_attr_get_u64(a));
> + data = nl_attr_get_u64(a);
> + memcpy(&cookie, &data, sizeof(cookie));
> +
> + if (cookie.type == USER_ACTION_COOKIE_CONTROLLER) {
> + ds_put_format(ds, "userspace(controller,data=%"PRIu32")",
If I remember correctly, we use this for length, right?
> + cookie.data);
> + } else if (cookie.type == USER_ACTION_COOKIE_SFLOW) {
> + ds_put_format(ds, "userspace(sFlow,n_output=%"PRIu8","
> + "vid=%"PRIu16",pcp=%"PRIu8",ifindex=%"PRIu32")",
> + 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")",
> + nl_attr_get_u64(a));
> + }
I think we use lower case for everything else here.
> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> index 68eb804..b96d7d2 100644
> --- a/ofproto/ofproto-dpif-sflow.c
> +++ b/ofproto/ofproto-dpif-sflow.c
> +dpif_sflow_received(struct dpif_sflow *ds, struct ofpbuf *packet,
> + const struct flow *flow,
> + const struct user_action_cookie *cookie)
> {
> SFL_FLOW_SAMPLE_TYPE fs;
> SFLFlow_sample_element hdrElem;
> SFLSampled_header *header;
> SFLFlow_sample_element switchElem;
> SFLSampler *sampler;
> - unsigned int left;
> - struct nlattr *a;
> - size_t n_outputs;
> + struct dpif_sflow_port *in_dsp;
> + struct netdev_stats stats;
> + int error;
>
> /* Build a flow sample */
> memset(&fs, 0, sizeof fs);
> - fs.input = dpif_sflow_odp_port_to_ifindex(ds,
> - ofp_port_to_odp_port(flow->in_port));
> - fs.output = 0; /* Filled in correctly below. */
> - fs.sample_pool = upcall->sample_pool;
> +
> + 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;
> + }
Currently if the controller sends a packet without a corresponding
port then userspace doesn't fill in the in_port member when sending
the flow key to the kernel. At the moment, the kernel uses a private
constant to represent this. However, this can now leak back up to
userspace, triggering the above warning. There's nothing that sFlow
can do about it (we should just not sample it) but we should probably
have a better way of representing this and not reporting spurious
warnings.
> + fs.input = SFL_DS_INDEX(in_dsp->dsi);
> +
> + error = netdev_get_stats(in_dsp->netdev, &stats);
> + if (error) {
> + VLOG_WARN_RL(&rl, "netdev get-stats error %s", strerror(error));
> + return;
> + }
> + fs.sample_pool = stats.rx_packets + stats.tx_packets;
The sample pool is just the number of packets received on an
interface. This also won't include packets that are sent from
userspace "manually", which are eligible for sampling.
> @@ -529,37 +550,21 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct
> dpif_upcall *upcall,
> 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);
> + switchElem.flowType.sw.dst_priority = vlan_tci_to_pcp(cookie->vlan_tci);
I don't think that we need to set these twice in a row.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
