[ovs-dev] [PATCH 1/2] datapath: Fix Flow dump operation.

[Ben Pfaff]

On Sun, Jan 20, 2013 at 11:28:24PM -0800, Pravin Shelar wrote:
> On Sun, Jan 20, 2013 at 9:02 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Sat, Jan 19, 2013 at 08:15:56PM -0800, Pravin B Shelar wrote:
> >> Following patch adds null check while inserting new netlink attribute.
> >> This was introduced by commit 9b405f1aa8d175d (datapath: More
> >> flexible kernel/userspace tunneling attribute.)
> >>
> >> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
> >> Bug #14767
> >
> > I agree that this fixes a null pointer dereference.  On that basis:
> >
> > Acked-by: Ben Pfaff <blp at nicira.com>
> >
> Thanks I pushed patch to master and 1.9.
> 
> > But: it looks like to me that we can end up with a partial set of
> > actions in the message, both with and without this patch.  I think that
> > if only some of the actions can be included, then we should omit the
> > attribute entirely, because I believe that this was the original and
> > intended behavior before commit 9b405f1aa8d175d.
> 
> ok, To check for space I can use actions_len which is pretty close to
> memory needed.

I was thinking of something like this, though I have not tested it:

diff --git a/datapath/datapath.c b/datapath/datapath.c
index ed3dd09..0f4796e 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1111,9 +1111,14 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
 	start = nla_nest_start(skb, OVS_FLOW_ATTR_ACTIONS);
 	if (start) {
 		err = actions_to_attr(sf_acts->actions, sf_acts->actions_len, skb);
-		if (err < 0 && skb_orig_len)
-			goto error;
-		nla_nest_end(skb, start);
+		if (!err)
+			nla_nest_end(skb, start);
+		else {
+			if (skb_orig_len)
+				goto error;
+
+			nla_nest_cancel(skb, start);
+		}
 	} else if (skb_orig_len) {
 		err = -ENOMEM;
 		goto error;