[PATCH net-next 0/2] net: sched: act_ctinfo: fixes

[PATCH net-next 0/2] net: sched: act_ctinfo: fixes

[Kevin Darbyshire-Bryant]

This is first attempt at sending a small series.  Order is important
because one bug (policy validation) prevents us from encountering the
more important 'OOPS' generating bug in action creation.  Fix the OOPS
first.

Confession time: Until very recently, development of this module has
been done on 'net-next' tree to 'clean compile' level with run-time
testing on backports to 4.14 & 4.19 kernels under openwrt.  It turns out
that sched: action: based code has been under more active change than I
realised.

During the back & forward porting during development & testing, the
critical ACT_P_CREATED return code got missed despite being in the 4.14
& 4.19 backports.  I have now gone through the init functions, using
act_csum as reference with a fine toothed comb and am happy they do the
same things.

This issue hadn't been caught till now due to another issue caused by
new strict nla_parse_nested function failing parsing validation before
action creation.

Thanks to Marcelo Leitner <marcelo.leitner@gmail.com> for flagging
extack deficiency (fixed in 733f0766c3de sched: act_ctinfo: use extack
error reporting) which led to b424e432e770 ("netlink: add validation of
NLA_F_NESTED flag") and 8cb081746c03 ("netlink: make validation more
configurable for future strictness”) which led to the policy validation
fix, which then led to the action creation fix both contained in this
series.

If I ever get to a developer conference please feel free to
tar/feather/apply cone of shame.

Kevin Darbyshire-Bryant (2):
  net: sched: act_ctinfo: fix action creation
  net: sched: act_ctinfo: fix policy validation

 net/sched/act_ctinfo.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.20.1 (Apple Git-117)

[PATCH net-next 1/2] net: sched: act_ctinfo: fix action creation

[Kevin Darbyshire-Bryant]

Use correct return value on action creation: ACT_P_CREATED.

The use of incorrect return value could result in a situation where the
system thought a ctinfo module was listening but actually wasn't
instantiated correctly leading to an OOPS in tcf_generic_walker().

Confession time: Until very recently, development of this module has
been done on 'net-next' tree to 'clean compile' level with run-time
testing on backports to 4.14 & 4.19 kernels under openwrt.  During the
back & forward porting during development & testing, the critical
ACT_P_CREATED return code got missed despite being in the 4.14 & 4.19
backports.  I have now gone through the init functions, using act_csum
as reference with a fine toothed comb.  Bonus, no more OOPSes.  I
managed to also miss this issue till now due to the new strict
nla_parse_nested function failing validation before action creation.

As an inexperienced developer I've learned that
copy/pasting/backporting/forward porting code correctly is hard.  If I
ever get to a developer conference I shall don the cone of shame.

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 net/sched/act_ctinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index a7d3679d7e2e..2c17f6843107 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -213,6 +213,7 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
 			tcf_idr_cleanup(tn, actparm->index);
 			return ret;
 		}
+		ret = ACT_P_CREATED;
 	} else if (err > 0) {
 		if (bind) /* don't override defaults */
 			return 0;
-- 
2.20.1 (Apple Git-117)

[PATCH net-next 2/2] net: sched: act_ctinfo: fix policy validation

[Kevin Darbyshire-Bryant]

Fix nla_policy definition by specifying an exact length type attribute
to CTINFO action paraneter block structure.  Without this change,
netlink parsing will fail validation and the action will not be
instantiated.

8cb081746c03 ("netlink: make validation more configurable for future")
introduced much stricter checking to attributes being passed via
netlink.  Existing actions were updated to use less restrictive
deprecated versions of nla_parse_nested.

As a new module, act_ctinfo should be designed to use the strict
checking model otherwise, well, what was the point of implementing it.

Confession time: Until very recently, development of this module has
been done on 'net-next' tree to 'clean compile' level with run-time
testing on backports to 4.14 & 4.19 kernels under openwrt.  This is how
I managed to miss the run-time impacts of the new strict
nla_parse_nested function.  I hopefully have learned something from this
(glances toward laptop running a net-next kernel)

There is however a still outstanding implication on iproute2 user space
in that it needs to be told to pass nested netlink messages with the
nested attribute actually set.  So even with this kernel fix to do
things correctly you still cannot instantiate a new 'strict'
nla_parse_nested based action such as act_ctinfo with iproute2's tc.

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 net/sched/act_ctinfo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index 2c17f6843107..10eb2bb99861 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -141,7 +141,8 @@ static int tcf_ctinfo_act(struct sk_buff *skb, const struct tc_action *a,
 }
 
 static const struct nla_policy ctinfo_policy[TCA_CTINFO_MAX + 1] = {
-	[TCA_CTINFO_ACT]		  = { .len = sizeof(struct
+	[TCA_CTINFO_ACT]		  = { .type = NLA_EXACT_LEN,
+					      .len = sizeof(struct
 							    tc_ctinfo) },
 	[TCA_CTINFO_ZONE]		  = { .type = NLA_U16 },
 	[TCA_CTINFO_PARMS_DSCP_MASK]	  = { .type = NLA_U32 },
-- 
2.20.1 (Apple Git-117)

Re: [PATCH net-next 0/2] net: sched: act_ctinfo: fixes

[David Miller]

From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Date: Mon, 17 Jun 2019 11:03:25 +0100

> This is first attempt at sending a small series.  Order is important
> because one bug (policy validation) prevents us from encountering the
> more important 'OOPS' generating bug in action creation.  Fix the OOPS
> first.
> 
> Confession time: Until very recently, development of this module has
> been done on 'net-next' tree to 'clean compile' level with run-time
> testing on backports to 4.14 & 4.19 kernels under openwrt.  It turns out
> that sched: action: based code has been under more active change than I
> realised.
> 
> During the back & forward porting during development & testing, the
> critical ACT_P_CREATED return code got missed despite being in the 4.14
> & 4.19 backports.  I have now gone through the init functions, using
> act_csum as reference with a fine toothed comb and am happy they do the
> same things.
> 
> This issue hadn't been caught till now due to another issue caused by
> new strict nla_parse_nested function failing parsing validation before
> action creation.
> 
> Thanks to Marcelo Leitner <marcelo.leitner@gmail.com> for flagging
> extack deficiency (fixed in 733f0766c3de sched: act_ctinfo: use extack
> error reporting) which led to b424e432e770 ("netlink: add validation of
> NLA_F_NESTED flag") and 8cb081746c03 ("netlink: make validation more
> configurable for future strictness”) which led to the policy validation
> fix, which then led to the action creation fix both contained in this
> series.
> 
> If I ever get to a developer conference please feel free to
> tar/feather/apply cone of shame.

:-)  In kernel networking development we prefer brown paper bags over
cones of shame, just FYI :) :) :)

Series applied, thanks.

Re: [PATCH net-next 0/2] net: sched: act_ctinfo: fixes

[Kevin 'ldir' Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]



> On 17 Jun 2019, at 22:01, David Miller <davem@davemloft.net> wrote:
> 
> From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
> Date: Mon, 17 Jun 2019 11:03:25 +0100
> 
>> 
<snipped>
>> If I ever get to a developer conference please feel free to
>> tar/feather/apply cone of shame.
> 
> :-)  In kernel networking development we prefer brown paper bags over
> cones of shame, just FYI :) :) :)

LOL - I’ll bear that in mind :-)  I thought fixing the code and admitting
my incompetence was the best policy…I’d be found out at some point anyway :-)

> 
> Series applied, thanks.

Excellent.  Hopefully that will be it.


Cheers,

Kevin D-B

gpg: 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]