| Message ID | 1420440610-20621-2-git-send-email-simon.horman@netronome.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)