| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 19 Nov 2014 16:39:20 -0800
From: Pravin Shelar <pshelar@...ira.com>
To: Joe Stringer <joestringer@...ira.com>
Cc: "dev@...nvswitch.org" <dev@...nvswitch.org>,
netdev <netdev@...r.kernel.org>
Subject: Re: [PATCHv10 ovs 12/15] datapath: Add support for unique flow identifiers.
On Wed, Nov 19, 2014 at 4:20 PM, Joe Stringer <joestringer@...ira.com> wrote:
> On Wednesday, November 19, 2014 15:34:24 Pravin Shelar wrote:
>> On Thu, Nov 13, 2014 at 11:17 AM, Joe Stringer <joestringer@...ira.com>
> wrote:
>> > @@ -684,33 +691,43 @@ static size_t ovs_flow_cmd_msg_size(const struct
>> > sw_flow_actions *acts)
>> >
>> > /* Called with ovs_mutex or RCU read lock. */
>> > static int ovs_flow_cmd_fill_match(const struct sw_flow *flow,
>> >
>> > - struct sk_buff *skb)
>> > + struct sk_buff *skb, u32 ufid_flags)
>> >
>> > {
>> >
>> > struct nlattr *nla;
>> > int err;
>> >
>> > - /* Fill flow key. */
>> > - nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
>> > - if (!nla)
>> > - return -EMSGSIZE;
>> > -
>> > - err = ovs_nla_put_flow(&flow->unmasked_key, &flow->unmasked_key,
>> > skb, - false);
>> > - if (err)
>> > - return err;
>> > -
>> > - nla_nest_end(skb, nla);
>> > + /* Fill flow key. If userspace didn't specify a UFID, then ignore
>> > the + * OMIT_KEY flag. */
>> > + if (!(ufid_flags & OVS_UFID_F_OMIT_KEY) ||
>> > + !flow->index_by_ufid) {
>>
>> I am not sure about this check, userspace needs to send atleast ufid
>> or the unmasked key as id for flow. otherwise we shld flag error. Here
>> we can serialize flow->key.
>> There could be another function which takes care of flow-id
>> serialization where we serialize use ufid or unmasked key as flow id.
>> Lets group ufid and unmasked key together rather than masked key and
>> unmasked key which are not related.
>
> Right, at flow setup time the flow key is always required, but the UFID is
> optional. For most other cases, one of the two most be specified. For flow dump,
> neither is required from userspace, but OMIT_KEY flag may be raised. That's the
> particular case that this logic is trying to catch (dump all flows, including
> those that were set up without UFID - in which case the OMIT_KEY flag doesn't
> make sense, so treat the flag like a request rather than a command).
>
How do you handle overlapping flows without the flow id in dump operation?
> Happy to split the key/identifier out from the mask.
>
>> > @@ -740,6 +757,32 @@ static int ovs_flow_cmd_fill_stats(const struct
>> > sw_flow *flow,
>> >
>> > }
>> >
>> > /* Called with ovs_mutex or RCU read lock. */
>> >
>> > +static int ovs_flow_cmd_fill_ufid(const struct sw_flow *flow,
>> > + struct sk_buff *skb)
>> > +{
>> > + struct nlattr *start;
>> > + const struct sw_flow_id *sfid;
>> > +
>> > + if (!flow->index_by_ufid)
>> > + return 0;
>> > +
>> > + sfid = &flow->index.ufid;
>> > + start = nla_nest_start(skb, OVS_FLOW_ATTR_UFID);
>> > + if (start) {
>> > + int err;
>> > +
>> > + err = nla_put(skb, OVS_UFID_ATTR_ID, sfid->ufid_len,
>> > + sfid->ufid);
>> > + if (err)
>> > + return err;
>> > + nla_nest_end(skb, start);
>> > + } else
>> > + return -EMSGSIZE;
>> > +
>> > + return 0;
>> > +}
>> > +
>>
>> Can you change this function according to comments above?
>
> OK.
>
>> > @@ -795,18 +838,24 @@ static int ovs_flow_cmd_fill_info(const struct
>> > sw_flow *flow, int dp_ifindex,
>> >
>> > ovs_header->dp_ifindex = dp_ifindex;
>> >
>> > - err = ovs_flow_cmd_fill_match(flow, skb);
>> > + err = ovs_flow_cmd_fill_match(flow, skb, ufid_flags);
>> >
>> > if (err)
>> >
>> > goto error;
>> >
>> > - err = ovs_flow_cmd_fill_stats(flow, skb);
>> > + err = ovs_flow_cmd_fill_ufid(flow, skb);
>> >
>> > if (err)
>> >
>> > goto error;
>>
>> Flow ID should go first in the netlink msg.
>
> OK.
>
>> > @@ -915,8 +980,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb,
>> > struct genl_info *info)
>> >
>> > error = -ENODEV;
>> > goto err_unlock_ovs;
>> >
>> > }
>> >
>> > +
>> >
>> > /* Check if this is a duplicate flow */
>> >
>> > - flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->unmasked_key);
>> > + flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
>>
>> Need to check for ufid table to find duplicate ufid entry here.
>
> OK.
>
>> > diff --git a/datapath/flow.h b/datapath/flow.h
>> > index 2bbf789..736e0eb 100644
>> > --- a/datapath/flow.h
>> > +++ b/datapath/flow.h
>> > @@ -196,6 +196,13 @@ struct sw_flow_match {
>> >
>> > struct sw_flow_mask *mask;
>> >
>> > };
>> >
>> > +struct sw_flow_id {
>> > + struct hlist_node node[2];
>> > + u32 hash;
>> > + u8 *ufid;
>> > + u8 ufid_len;
>> > +};
>> > +
>>
>> Lets make ufid array of size 256, we can reject any key greater than
>> this. current patch does not support key greater than 256 anyways.
>
> Ok, that sounds reasonable.
>
>> > struct sw_flow_actions {
>> >
>> > struct rcu_head rcu;
>> > u32 actions_len;
>> >
>> > @@ -212,13 +219,20 @@ struct flow_stats {
>> >
>> > struct sw_flow {
>> >
>> > struct rcu_head rcu;
>> >
>> > - struct hlist_node hash_node[2];
>> > - u32 hash;
>> > + struct {
>> > + struct hlist_node node[2];
>> > + u32 hash;
>> > + } flow_hash;
>>
>> This change does not look related to this work.
>
> Right, this is unnecessary leftover from earlier iteration.
>
>
>> > int stats_last_writer; /* NUMA-node id of the last
>> > writer on
>> >
>> > * 'stats[0]'.
>> > */
>> >
>> > struct sw_flow_key key;
>> >
>> > - struct sw_flow_key unmasked_key;
>> > + bool index_by_ufid; /* Which of the below that
>> > userspace + uses to index this
>> > flow. */ + union {
>> > + struct sw_flow_key unmasked_key;
>> > + struct sw_flow_id ufid;
>> > + } index;
>>
>> Rather than storing ufid or unmasked key inside struct flow we can
>> keep pointer to these objects, that will save some memory.
>
> OK. Do you still care about the union being there?
>
Lets keep both pointer so that we can get rid of the index_by_fid flag.
>> > @@ -818,31 +914,65 @@ static int flow_mask_insert(struct flow_table *tbl,
>> > struct sw_flow *flow,
>> >
>> > int ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,
>> >
>> > const struct sw_flow_mask *mask)
>> >
>> > {
>> >
>> > - struct table_instance *new_ti = NULL;
>> > - struct table_instance *ti;
>> > + struct table_instance *new_ti = NULL, *new_ufid_ti = NULL;
>> > + struct table_instance *ti, *ufid_ti = NULL;
>> >
>> > int err;
>> >
>> > err = flow_mask_insert(table, flow, mask);
>> > if (err)
>> >
>> > return err;
>> >
>> > - flow->hash = flow_hash(&flow->key, flow->mask->range.start,
>> > - flow->mask->range.end);
>> > + flow->flow_hash.hash = flow_hash(&flow->key,
>> > flow->mask->range.start, +
>> > flow->mask->range.end);
>> >
>> > ti = ovsl_dereference(table->ti);
>> > table_instance_insert(ti, flow);
>> > table->count++;
>> >
>> > + if (flow->index_by_ufid) {
>> > + flow->index.ufid.hash = ufid_hash(&flow->index.ufid);
>> > + ufid_ti = ovsl_dereference(table->ufid_ti);
>> > + ufid_table_instance_insert(ufid_ti, flow);
>> > + table->ufid_count++;
>> > + }
>> >
>> > /* Expand table, if necessary, to make room. */
>> > if (table->count > ti->n_buckets)
>> >
>> > - new_ti = table_instance_expand(ti);
>> > + new_ti = flow_table_expand(ti, false);
>> >
>> > else if (time_after(jiffies, table->last_rehash +
>> > REHASH_INTERVAL))
>> >
>> > - new_ti = table_instance_rehash(ti, ti->n_buckets);
>> > + new_ti = flow_table_rehash(ti, ti->n_buckets, false);
>> > + if (ufid_ti && table->ufid_count > ufid_ti->n_buckets)
>> > + new_ufid_ti = flow_table_expand(ufid_ti, true);
>> >
>> > if (new_ti) {
>> >
>> > rcu_assign_pointer(table->ti, new_ti);
>> >
>> > - table_instance_destroy(ti, true);
>> > + call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
>> >
>> > table->last_rehash = jiffies;
>> >
>> > }
>> >
>> > + if (new_ufid_ti) {
>> > + rcu_assign_pointer(table->ufid_ti, new_ufid_ti);
>> > + call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
>> > + }
>> > + return 0;
>> > +}
>>
>> Insert function can be simplified by first updating flow-table and
>> then updating ufid table.
>
> Sure.
>
>
>> > #define OVS_FLOW_ATTR_MAX (__OVS_FLOW_ATTR_MAX - 1)
>> >
>> > /**
>> >
>> > + * enum ovs_ufid_attr - Unique identifier types.
>> > + *
>> > + * @OVS_UFID_ATTR_FLAGS: A 32-bit value specifying changes to the
>> > behaviour of + * the current %OVS_FLOW_CMD_* request. Optional for all
>> > requests. + * @OVS_UFID_ATTR_ID: A unique identifier for a flow.
>> > + */
>> > +enum ovs_ufid_attr {
>> > + OVS_UFID_ATTR_UNSPEC,
>> > + OVS_UFID_ATTR_FLAGS, /* u32 of OVS_UFID_F_* */
>> > + OVS_UFID_ATTR_ID, /* variable length identifier. */
>> > + __OVS_UFID_ATTR_MAX
>> > +};
>> > +
>> > +#define OVS_UFID_ATTR_MAX (__OVS_UFID_ATTR_MAX - 1)
>> > +
>> > +/**
>> > + * Omit attributes for notifications.
>> > + *
>> > + * If a datapath request contains an OVS_UFID_F_OMIT_* flag, then the
>> > datapath + * may omit the corresponding 'ovs_flow_attr' from the
>> > response. + */
>> > +#define OVS_UFID_F_OMIT_KEY (1 << 0)
>> > +#define OVS_UFID_F_OMIT_MASK (1 << 1)
>> > +#define OVS_UFID_F_OMIT_ACTIONS (1 << 2)
>> > +
>>
>> These flags are related to flow operations. So OVS_UFID_ATTR_FLAGS
>> should be part of enum ovs_flow_attr.
>> This way we do not need to make UFID nested attr.
>
> OK, I'll shift it out.
>
> Thanks for the review, I'll work on a fresh revision.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists