[ovs-dev] [netlink v4 42/52] datapath: Convert flows to use Netlink framing.

[Ben Pfaff]

On Tue, Jan 18, 2011 at 05:54:54PM -0800, Jesse Gross wrote:
> On Tue, Jan 11, 2011 at 9:49 PM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index bc64f89..c8e7ca7 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > +static struct sw_flow_actions *get_actions(const struct dp_flowcmd *flowcmd)
> [...]
> > + ? ? ? actions = flow_actions_alloc(flowcmd->actions_len);
> > + ? ? ? if (!IS_ERR(actions))
> > + ? ? ? ? ? ? ? memcpy(actions->actions, flowcmd->actions, flowcmd->actions_len);
> 
> Can't flowcmd->actions potentially be NULL?

Only if flowcmd->actions_len is 0.

I don't think that's a real problem in a kernel context, but I changed
this to check for that, just in case:

-	if (!IS_ERR(actions))
+	if (!IS_ERR(actions) && flowcmd->actions_len)
 		memcpy(actions->actions, flowcmd->actions, flowcmd->actions_len);

> > +static struct sk_buff *copy_flow_from_user(struct odp_flow __user *uodp_flow,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct dp_flowcmd *flowcmd, bool need_key)
> [...]
> > + ? ? ? /* ODPFT_KEY. */
> > + ? ? ? if (need_key) {
> > + ? ? ? ? ? ? ? err = -EINVAL;
> > + ? ? ? ? ? ? ? if (!a[ODPFT_KEY])
> > + ? ? ? ? ? ? ? ? ? ? ? goto error_free_skb;
> > + ? ? ? ? ? ? ? err = flow_from_nlattrs(&flowcmd->key, a[ODPFT_KEY]);
> > + ? ? ? ? ? ? ? if (err)
> > + ? ? ? ? ? ? ? ? ? ? ? goto error_free_skb;
> > + ? ? ? }
> 
> need_key seems a little inconsistent to me.  It checks that the
> attribute is present, which isn't done for other things.  It also
> seems that there are places that don't care about the actions but we
> don't do anything special for that, though probably people just won't
> provide any actions in those cases.

I can just make the key optional all the time, I guess.  OK, I'll do
that.

For what it's worth, it seems to be an informal rule of Netlink design
that extra attributes are ignored.  It's sensible, since it allows for
many kinds of extensions.

> > +static int new_flow(unsigned int cmd, struct odp_flow __user *uodp_flow)
> > +{
> > + ? ? ? struct tbl_node *flow_node;
> > + ? ? ? struct dp_flowcmd flowcmd;
> > + ? ? ? struct sw_flow *flow;
> > + ? ? ? struct sk_buff *skb;
> > + ? ? ? struct datapath *dp;
> > + ? ? ? struct tbl *table;
> > + ? ? ? u32 hash;
> > + ? ? ? int error;
> > +
> > + ? ? ? skb = copy_flow_from_user(uodp_flow, &flowcmd, true);
> > + ? ? ? error = PTR_ERR(skb);
> > + ? ? ? if (IS_ERR(skb))
> > + ? ? ? ? ? ? ? goto exit;
> > +
> > + ? ? ? dp = get_dp_locked(flowcmd.dp_idx);
> > + ? ? ? error = -ENODEV;
> > + ? ? ? if (!dp)
> > + ? ? ? ? ? ? ? goto error_kfree_skb;
> > +
> > + ? ? ? hash = flow_hash(&flowcmd.key);
> > + ? ? ? table = get_table_protected(dp);
> > + ? ? ? flow_node = tbl_lookup(table, &flowcmd.key, hash, flow_cmp);
> > + ? ? ? if (!flow_node) {
> > + ? ? ? ? ? ? ? struct sw_flow_actions *acts;
> > +
> > + ? ? ? ? ? ? ? /* No such flow. */
> > + ? ? ? ? ? ? ? error = -ENOENT;
> > + ? ? ? ? ? ? ? if (!(flowcmd.nlmsg_flags & NLM_F_CREATE))
> 
> I'm a little bit unsure of the overlap between
> ODP_FLOW_NEW/ODP_FLOW_SET and NLM_F_CREATE.  It seems like they're
> redundant but there's more two states so obviously it adds
> information.  I don't know, it just seems somewhat strange.

I should have said something about this earlier.  This is a workaround
for a nasty bug in the kernel Generic Netlink implementation.  See the
thread in which the bug was fixed at
http://permalink.gmane.org/gmane.linux.network/182705 and the thread in
which the fix had to be reverted at
http://permalink.gmane.org/gmane.linux.network/183939.

This code wants to support three possibilities:

        1. Create new flow or modify existing flow.

        2. Create new flow, fail if a flow already exists.

        3. Modify existing flow, fail if no such flow exists.

The "obvious" way to do this with netlink is:

        1. ODPFC_NEW, no flags.

        2. ODPFC_NEW, NLM_F_EXCL.

        3. ODPFC_SET, no flags.

But this Generic Netlink bug means that #2 would be interpreted as a
"dump" operation.  So instead we substitute NLM_F_CREATE for NLM_F_EXCL:

        1. ODPFC_NEW, no flags.

        2. ODPFC_NEW, NLM_F_CREATE.

        3. ODFPC_SET, no flags.

Hmm, the code is totally wrong though.  Oops.  I'll fix that.

> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index d4b7176..5d502ab 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > -u32 flow_to_nlattrs(const struct sw_flow_key *swkey, struct sk_buff *skb)
> > +int flow_to_nlattrs(const struct sw_flow_key *swkey, struct sk_buff *skb)
> [...]
> > ? ? ? ?nla_put_be16(skb, ODPKT_ETHERTYPE, swkey->dl_type);
> 
> Should this be NLA_PUT_BE16?  Also, I don't think that anything uses
> the non-macro versions of these functions that you added.  Should we
> remove them now since they aren't really compat code?

I'm willing, if you want to, I guess.

> > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> > index 38a06ea..c610a3f 100644
> > --- a/lib/dpif-linux.c
> > +++ b/lib/dpif-linux.c
> > +static int
> > +dpif_linux_flow_from_ofpbuf(struct dpif_linux_flow *flow,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ?const struct ofpbuf *buf)
> > +{
> > + ? ?static const struct nl_policy odp_flow_policy[] = {
> > + ? ? ? ?[ODPFT_KEY] = { .type = NL_A_NESTED, .optional = true },
> 
> Is the key really optional (certainly the kernel always provides it)?
> It's not clear what we would do without one.

You're right, this should be mandatory now.  I think in some
intermediate form I was using a missing key as part of a dump response
to mean "no more flows".

> > + ? ? ? ?[ODPFT_ACTIONS] = { .type = NL_A_NESTED, .optional = true },
> > + ? ? ? ?[ODPFT_STATS] = { .type = NL_A_UNSPEC,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ?.min_len = sizeof(struct odp_flow_stats),
> > + ? ? ? ? ? ? ? ? ? ? ? ? ?.max_len = sizeof(struct odp_flow_stats),
> > + ? ? ? ? ? ? ? ? ? ? ? ? ?.optional = true },
> > + ? ? ? ?[ODPFT_TCP_FLAGS] = { .type = NL_A_U8, .optional = true },
> > + ? ? ? ?[ODPFT_USED] = { .type = NL_A_U64, .optional = true },
> > + ? ? ? ?[ODPFT_CLEAR] = { .type = NL_A_FLAG, .optional = true },
> 
> What does clear mean in this context?

Zero statistics.

> > + ? ?if (a[ODPFT_STATS]) {
> > + ? ? ? ?flow->stats = nl_attr_get(a[ODPFT_STATS]);
> > + ? ?}
> > + ? ?if (a[ODPFT_TCP_FLAGS]) {
> > + ? ? ? ?flow->tcp_flags = nl_attr_get(a[ODPFT_TCP_FLAGS]);
> > + ? ?}
> > + ? ?flow->clear = a[ODPFT_CLEAR] != NULL;
> > + ? ?if (a[ODPFT_STATE]) {
> > + ? ? ? ?flow->state = nl_attr_get(a[ODPFT_STATE]);
> > + ? ?}
> 
> Why are all of these pointers?  It seems like it might be simpler to
> just copy them.

They are pointers to allowing distinguishing the "present" and "not
present" cases.