[PATCH/RFC rocker-net-next 0/6] net: flow: Minor fixes and cleanups

[PATCH/RFC rocker-net-next 0/6] net: flow: Minor fixes and cleanups

[Simon Horman]

Hi John,

This series contains several minor fixes and cleanups for your
consideration.

They apply on to of https://github.com/jrfastab/rocker-net-next
head commit 1b2a1e4 ("net: rocker: implement delete flow routine")

I have previously lightly exercised them on top of
https://github.com/jrfastab/flow-net-next
head commit 9f75688 ("net: flow: add new DYNAMIC flag")

Simon Horman (6):
  net: flow: Cancel innermost nested attribute first
  net: flow: Handle error when putting a field while putting a flow
  net: flow: Remove unnecessary zero-header check when putting a flow
  net: flow: free action args
  net: flow: Return more fine-grained error when handing flows commands
  net: flow: Limit checking of ndo_flow_{set,del}_flows

 net/core/flow_table.c | 47 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)

-- 
2.1.3

[PATCH/RFC rocker-net-next 1/6] net: flow: Cancel innermost nested attribute first

[Simon Horman]

Cancel innermost nested attribute first on error when putting flow actions.

Signed-off-by: Simon Horman <simon.horman@netronome.com>

---

Its unclear to me if this makes any difference.
But it seems more logical to me.
---
 net/core/flow_table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/flow_table.c b/net/core/flow_table.c
index 542ebb5..2af831e 100644
--- a/net/core/flow_table.c
+++ b/net/core/flow_table.c
@@ -967,8 +967,8 @@ static int net_flow_put_flow_action(struct sk_buff *skb,
 
 		err = net_flow_put_act_types(skb, a[i].args);
 		if (err) {
-			nla_nest_cancel(skb, action);
 			nla_nest_cancel(skb, sigs);
+			nla_nest_cancel(skb, action);
 			return err;
 		}
 		nla_nest_end(skb, sigs);
-- 
2.1.3

Re: [PATCH/RFC rocker-net-next 1/6] net: flow: Cancel innermost nested attribute first

[David Miller]

From: Simon Horman <simon.horman@netronome.com>
Date: Mon,  5 Jan 2015 15:50:05 +0900

> Cancel innermost nested attribute first on error when putting flow actions.
> 
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> 
> ---
> 
> Its unclear to me if this makes any difference.
> But it seems more logical to me.

Hmmm.  Be careful here.  nla_nest_cancel() is just rolling back
the length of the SKB to right before the netlink attribute being
given as the cancellation point.

So you really have to cancel attributes in exactly the reverse order
in which they were added.  Otherwise we'll make a trim call with a
negative adjustment that actually expands the SKB past an already
cancelled attribute.

Re: [PATCH/RFC rocker-net-next 1/6] net: flow: Cancel innermost nested attribute first

[Thomas Graf]

On 01/05/15 at 04:17pm, David Miller wrote:
> From: Simon Horman <simon.horman@netronome.com>
> Date: Mon,  5 Jan 2015 15:50:05 +0900
> 
> > Cancel innermost nested attribute first on error when putting flow actions.
> > 
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > 
> > ---
> > 
> > Its unclear to me if this makes any difference.
> > But it seems more logical to me.
> 
> Hmmm.  Be careful here.  nla_nest_cancel() is just rolling back
> the length of the SKB to right before the netlink attribute being
> given as the cancellation point.
> 
> So you really have to cancel attributes in exactly the reverse order
> in which they were added.  Otherwise we'll make a trim call with a
> negative adjustment that actually expands the SKB past an already
> cancelled attribute.

As I told John in the other the other thread: It is often clearer
and less error prone to only ever cancel the outer most nesting
level if we are undoing all of them anyway. Dave is absolutely right,
a wrong order will lead to bugs which are hard to track down.
Thinking about this I'll send a patch to WARN() in nlmsg_trim() on
mark < skb->data.

Re: [PATCH/RFC rocker-net-next 1/6] net: flow: Cancel innermost nested attribute first

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Mon, 5 Jan 2015 22:01:13 +0000

> Thinking about this I'll send a patch to WARN() in nlmsg_trim() on
> mark < skb->data.

+1

Re: [PATCH/RFC rocker-net-next 1/6] net: flow: Cancel innermost nested attribute first

[Simon Horman]

On Mon, Jan 05, 2015 at 04:17:25PM -0500, David Miller wrote:
> From: Simon Horman <simon.horman@netronome.com>
> Date: Mon,  5 Jan 2015 15:50:05 +0900
> 
> > Cancel innermost nested attribute first on error when putting flow actions.
> > 
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > 
> > ---
> > 
> > Its unclear to me if this makes any difference.
> > But it seems more logical to me.
> 
> Hmmm.  Be careful here.  nla_nest_cancel() is just rolling back
> the length of the SKB to right before the netlink attribute being
> given as the cancellation point.
> 
> So you really have to cancel attributes in exactly the reverse order
> in which they were added.  Otherwise we'll make a trim call with a
> negative adjustment that actually expands the SKB past an already
> cancelled attribute.

Thanks for clarifying that.

The aim of my patch is to perform the roll back in reverse order
which I now know is required.

[PATCH/RFC rocker-net-next 2/6] net: flow: Handle error when putting a field while putting a flow

[Simon Horman]

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 net/core/flow_table.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/flow_table.c b/net/core/flow_table.c
index 2af831e..753ebe0 100644
--- a/net/core/flow_table.c
+++ b/net/core/flow_table.c
@@ -981,8 +981,9 @@ done:
 
 int net_flow_put_flow(struct sk_buff *skb, struct net_flow_flow *flow)
 {
-	struct nlattr *flows, *matches;
+	struct nlattr *flows;
 	struct nlattr *actions = NULL; /* must be null to unwind */
+	struct nlattr *matches = NULL; /* must be null to unwind */
 	int err, j, i = 0;
 
 	flows = nla_nest_start(skb, NET_FLOW_FLOW);
@@ -1005,7 +1006,11 @@ int net_flow_put_flow(struct sk_buff *skb, struct net_flow_flow *flow)
 			if (!f->header)
 				continue;
 
-			nla_put(skb, NET_FLOW_FIELD_REF, sizeof(*f), f);
+			err = nla_put(skb, NET_FLOW_FIELD_REF, sizeof(*f), f);
+			if (err) {
+				nla_nest_cancel(skb, matches);
+				goto flows_put_failure;
+			}
 		}
 		nla_nest_end(skb, matches);
 	}
-- 
2.1.3

Re: [PATCH/RFC rocker-net-next 2/6] net: flow: Handle error when putting a field while putting a flow

[John Fastabend]

On 01/04/2015 10:50 PM, Simon Horman wrote:
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> ---
>   net/core/flow_table.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/flow_table.c b/net/core/flow_table.c
> index 2af831e..753ebe0 100644
> --- a/net/core/flow_table.c
> +++ b/net/core/flow_table.c
> @@ -981,8 +981,9 @@ done:
>
>   int net_flow_put_flow(struct sk_buff *skb, struct net_flow_flow *flow)
>   {
> -	struct nlattr *flows, *matches;
> +	struct nlattr *flows;
>   	struct nlattr *actions = NULL; /* must be null to unwind */
> +	struct nlattr *matches = NULL; /* must be null to unwind */

Actually we don't need to initialize to NULL now. That was some crazy
unwind scheme I had in place initially.

Now we only ever do a nla_nest_cancel on nested attributes that have
been initialized with nla_nest_start(). So I can simplify this to

	struct nlattr *flows, *matches, *actions;

>   	int err, j, i = 0;
>
>   	flows = nla_nest_start(skb, NET_FLOW_FLOW);
> @@ -1005,7 +1006,11 @@ int net_flow_put_flow(struct sk_buff *skb, struct net_flow_flow *flow)
>   			if (!f->header)
>   				continue;
>
> -			nla_put(skb, NET_FLOW_FIELD_REF, sizeof(*f), f);
> +			err = nla_put(skb, NET_FLOW_FIELD_REF, sizeof(*f), f);

Great thanks. I missed this one.

> +			if (err) {
> +				nla_nest_cancel(skb, matches);
> +				goto flows_put_failure;
> +			}
>   		}
>   		nla_nest_end(skb, matches);
>   	}
>

I'll fold this into the series and resubmit thanks.

.John

-- 
John Fastabend         Intel Corporation

Re: [PATCH/RFC rocker-net-next 2/6] net: flow: Handle error when putting a field while putting a flow

[Simon Horman]

On Mon, Jan 05, 2015 at 09:28:14AM -0800, John Fastabend wrote:
> On 01/04/2015 10:50 PM, Simon Horman wrote:
> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >---
> >  net/core/flow_table.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> >diff --git a/net/core/flow_table.c b/net/core/flow_table.c
> >index 2af831e..753ebe0 100644
> >--- a/net/core/flow_table.c
> >+++ b/net/core/flow_table.c
> >@@ -981,8 +981,9 @@ done:
> >
> >  int net_flow_put_flow(struct sk_buff *skb, struct net_flow_flow *flow)
> >  {
> >-	struct nlattr *flows, *matches;
> >+	struct nlattr *flows;
> >  	struct nlattr *actions = NULL; /* must be null to unwind */
> >+	struct nlattr *matches = NULL; /* must be null to unwind */
> 
> Actually we don't need to initialize to NULL now. That was some crazy
> unwind scheme I had in place initially.
> 
> Now we only ever do a nla_nest_cancel on nested attributes that have
> been initialized with nla_nest_start(). So I can simplify this to
> 
> 	struct nlattr *flows, *matches, *actions;

Thanks, that does seem much nicer :)

> 
> >  	int err, j, i = 0;
> >
> >  	flows = nla_nest_start(skb, NET_FLOW_FLOW);
> >@@ -1005,7 +1006,11 @@ int net_flow_put_flow(struct sk_buff *skb, struct net_flow_flow *flow)
> >  			if (!f->header)
> >  				continue;
> >
> >-			nla_put(skb, NET_FLOW_FIELD_REF, sizeof(*f), f);
> >+			err = nla_put(skb, NET_FLOW_FIELD_REF, sizeof(*f), f);
> 
> Great thanks. I missed this one.
> 
> >+			if (err) {
> >+				nla_nest_cancel(skb, matches);
> >+				goto flows_put_failure;
> >+			}
> >  		}
> >  		nla_nest_end(skb, matches);
> >  	}
> >
> 
> I'll fold this into the series and resubmit thanks.
> 
> .John
> 
> -- 
> John Fastabend         Intel Corporation

[PATCH/RFC rocker-net-next 3/6] net: flow: Remove unnecessary zero-header check when putting a flow

[Simon Horman]

This is the condition for the for loop to continue.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 net/core/flow_table.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/core/flow_table.c b/net/core/flow_table.c
index 753ebe0..c734a9d 100644
--- a/net/core/flow_table.c
+++ b/net/core/flow_table.c
@@ -1003,9 +1003,6 @@ int net_flow_put_flow(struct sk_buff *skb, struct net_flow_flow *flow)
 		for (j = 0; flow->matches && flow->matches[j].header; j++) {
 			struct net_flow_field_ref *f = &flow->matches[j];
 
-			if (!f->header)
-				continue;
-
 			err = nla_put(skb, NET_FLOW_FIELD_REF, sizeof(*f), f);
 			if (err) {
 				nla_nest_cancel(skb, matches);
-- 
2.1.3

[PATCH/RFC rocker-net-next 4/6] net: flow: free action args

[Simon Horman]

Free the args of an action along with the action itself to avoid leaking
memory.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 net/core/flow_table.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/core/flow_table.c b/net/core/flow_table.c
index c734a9d..1a32a72 100644
--- a/net/core/flow_table.c
+++ b/net/core/flow_table.c
@@ -1050,6 +1050,18 @@ static int net_flow_get_field(struct net_flow_field_ref *field,
 	return 0;
 }
 
+static void net_flow_free_actions(struct net_flow_action *actions)
+{
+	int i;
+
+	if (!actions)
+		return;
+
+	for (i = 0; actions[i].uid; i++)
+		kfree(actions[i].args);
+	kfree(actions);
+}
+
 static int net_flow_get_action(struct net_flow_action *a, struct nlattr *attr)
 {
 	struct nlattr *act[NET_FLOW_ACTION_ATTR_MAX+1];
@@ -1168,7 +1180,7 @@ static int net_flow_get_flow(struct net_flow_flow *flow, struct nlattr *attr)
 			err = net_flow_get_action(&flow->actions[count], attr2);
 			if (err) {
 				kfree(flow->matches);
-				kfree(flow->actions);
+				net_flow_free_actions(flow->actions);
 				return err;
 			}
 			count++;
@@ -1251,7 +1263,7 @@ static int net_flow_table_cmd_flows(struct sk_buff *recv_skb,
 
 		/* Cleanup flow */
 		kfree(this.matches);
-		kfree(this.actions);
+		net_flow_free_actions(this.actions);
 
 		if (err && err_handle == NET_FLOW_FLOWS_ERROR_ABORT)
 			goto out;
-- 
2.1.3

[PATCH/RFC rocker-net-next 5/6] net: flow: Return more fine-grained error when handing flows commands

[Simon Horman]

net_flow_table_cmd_flows() sets err to various error values
before returning err. It seems as well to return the value.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 net/core/flow_table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/flow_table.c b/net/core/flow_table.c
index 1a32a72..bfc984f 100644
--- a/net/core/flow_table.c
+++ b/net/core/flow_table.c
@@ -1285,7 +1285,7 @@ out:
 	if (skb)
 		nlmsg_free(skb);
 	dev_put(dev);
-	return -EINVAL;
+	return err;
 }
 
 static const struct nla_policy net_flow_cmd_policy[NET_FLOW_MAX + 1] = {
-- 
2.1.3

[PATCH/RFC rocker-net-next 6/6] net: flow: Limit checking of ndo_flow_{set,del}_flows

[Simon Horman]

Only check for availability of ndo_flow_{set,del}_flows when
they are to be be used.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 net/core/flow_table.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/net/core/flow_table.c b/net/core/flow_table.c
index bfc984f..6d620d4 100644
--- a/net/core/flow_table.c
+++ b/net/core/flow_table.c
@@ -1206,9 +1206,20 @@ static int net_flow_table_cmd_flows(struct sk_buff *recv_skb,
 	if (!dev)
 		return -EINVAL;
 
-	if (!dev->netdev_ops->ndo_flow_set_flows ||
-	    !dev->netdev_ops->ndo_flow_del_flows)
+	switch (cmd) {
+	case NET_FLOW_TABLE_CMD_SET_FLOWS:
+		if (!dev->netdev_ops->ndo_flow_set_flows)
+			goto out;
+		break;
+
+	case NET_FLOW_TABLE_CMD_DEL_FLOWS:
+		if (!dev->netdev_ops->ndo_flow_del_flows)
+			goto out;
+		break;
+
+	default:
 		goto out;
+	}
 
 	if (!info->attrs[NET_FLOW_IDENTIFIER_TYPE] ||
 	    !info->attrs[NET_FLOW_IDENTIFIER] ||
-- 
2.1.3

Re: [PATCH/RFC rocker-net-next 6/6] net: flow: Limit checking of ndo_flow_{set,del}_flows

[John Fastabend]

On 01/04/2015 10:50 PM, Simon Horman wrote:
> Only check for availability of ndo_flow_{set,del}_flows when
> they are to be be used.
>

I went ahead and merged this but, I'm not sure does it make
sense to allow a user to add a flow that can't be deleted? Or
delete a flow that wasn't ever added? I guess if the driver has
a reason to do this it doesn't hurt to allow it and I think the
code looks neater this way.

Also thanks for the other fixes I pulled the other 5 in as well
I'll re-submit the series after running some basic tests.

> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> ---
>   net/core/flow_table.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/flow_table.c b/net/core/flow_table.c
> index bfc984f..6d620d4 100644
> --- a/net/core/flow_table.c
> +++ b/net/core/flow_table.c
> @@ -1206,9 +1206,20 @@ static int net_flow_table_cmd_flows(struct sk_buff *recv_skb,
>   	if (!dev)
>   		return -EINVAL;
>
> -	if (!dev->netdev_ops->ndo_flow_set_flows ||
> -	    !dev->netdev_ops->ndo_flow_del_flows)
> +	switch (cmd) {
> +	case NET_FLOW_TABLE_CMD_SET_FLOWS:
> +		if (!dev->netdev_ops->ndo_flow_set_flows)
> +			goto out;
> +		break;
> +
> +	case NET_FLOW_TABLE_CMD_DEL_FLOWS:
> +		if (!dev->netdev_ops->ndo_flow_del_flows)
> +			goto out;
> +		break;
> +
> +	default:
>   		goto out;
> +	}
>
>   	if (!info->attrs[NET_FLOW_IDENTIFIER_TYPE] ||
>   	    !info->attrs[NET_FLOW_IDENTIFIER] ||
>


-- 
John Fastabend         Intel Corporation

Re: [PATCH/RFC rocker-net-next 6/6] net: flow: Limit checking of ndo_flow_{set,del}_flows

[Simon Horman]

On Mon, Jan 05, 2015 at 09:32:02AM -0800, John Fastabend wrote:
> On 01/04/2015 10:50 PM, Simon Horman wrote:
> >Only check for availability of ndo_flow_{set,del}_flows when
> >they are to be be used.
> >
> 
> I went ahead and merged this but, I'm not sure does it make
> sense to allow a user to add a flow that can't be deleted? Or
> delete a flow that wasn't ever added? I guess if the driver has
> a reason to do this it doesn't hurt to allow it and I think the
> code looks neater this way.
> 
> Also thanks for the other fixes I pulled the other 5 in as well
> I'll re-submit the series after running some basic tests.

I don't have any strong opinions on this but it
sounds like policy that doesn't belong in flow_table.c.

> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >---
> >  net/core/flow_table.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> >diff --git a/net/core/flow_table.c b/net/core/flow_table.c
> >index bfc984f..6d620d4 100644
> >--- a/net/core/flow_table.c
> >+++ b/net/core/flow_table.c
> >@@ -1206,9 +1206,20 @@ static int net_flow_table_cmd_flows(struct sk_buff *recv_skb,
> >  	if (!dev)
> >  		return -EINVAL;
> >
> >-	if (!dev->netdev_ops->ndo_flow_set_flows ||
> >-	    !dev->netdev_ops->ndo_flow_del_flows)
> >+	switch (cmd) {
> >+	case NET_FLOW_TABLE_CMD_SET_FLOWS:
> >+		if (!dev->netdev_ops->ndo_flow_set_flows)
> >+			goto out;
> >+		break;
> >+
> >+	case NET_FLOW_TABLE_CMD_DEL_FLOWS:
> >+		if (!dev->netdev_ops->ndo_flow_del_flows)
> >+			goto out;
> >+		break;
> >+
> >+	default:
> >  		goto out;
> >+	}
> >
> >  	if (!info->attrs[NET_FLOW_IDENTIFIER_TYPE] ||
> >  	    !info->attrs[NET_FLOW_IDENTIFIER] ||
> >
> 
> 
> -- 
> John Fastabend         Intel Corporation