From: Petr Machata <petrm@nvidia.com> Date: 2021-01-18 14:07:27
From: Petr Machata <redacted>
There is currently one policy that covers all attributes for next hop
object management. Actual validation is then done in code, which makes it
unobvious which attributes are acceptable when, and indeed that everything
is rejected as necessary.
In this series, split rtm_nh_policy to several policies that cover various
aspects of the next hop object configuration, and instead of open-coding
the validation, defer to nlmsg_parse(). This should make extending the next
hop code simpler as well, which will be relevant in near future for
resilient hashing implementation.
This was tested by running tools/testing/selftests/net/fib_nexthops.sh.
Additionally iproute2 was tweaked to issue "nexthop list id" as an
RTM_GETNEXTHOP dump request, instead of a straight get to test that
unexpected attributes are indeed rejected.
In patch #1, convert attribute validation in nh_valid_get_del_req().
In patch #2, convert nh_valid_dump_req().
In patch #3, rtm_nh_policy is cleaned up and renamed to rtm_nh_policy_new,
because after the above two patches, that is the only context that it is
used in.
Petr Machata (3):
nexthop: Use a dedicated policy for nh_valid_get_del_req()
nexthop: Use a dedicated policy for nh_valid_dump_req()
nexthop: Specialize rtm_nh_policy
net/ipv4/nexthop.c | 85 +++++++++++++++++-----------------------------
1 file changed, 32 insertions(+), 53 deletions(-)
--
2.26.2
From: Petr Machata <petrm@nvidia.com> Date: 2021-01-18 14:07:38
This function uses the global nexthop policy, but only accepts four
particular attributes. Create a new policy that only includes the four
supported attributes, and use it. Convert the loop to a series of ifs.
Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
net/ipv4/nexthop.c | 57 +++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 31 deletions(-)
From: Petr Machata <petrm@nvidia.com> Date: 2021-01-18 14:08:13
This policy is currently only used for creation of new next hops and new
next hop groups. Rename it accordingly and remove the two attributes that
are not valid in that context: NHA_GROUPS and NHA_MASTER.
Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
net/ipv4/nexthop.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
@@ -1657,7 +1655,7 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,structnlattr*tb[NHA_MAX+1];interr;-err=nlmsg_parse(nlh,sizeof(*nhm),tb,NHA_MAX,rtm_nh_policy,+err=nlmsg_parse(nlh,sizeof(*nhm),tb,NHA_MAX,rtm_nh_policy_new,extack);if(err<0)returnerr;
@@ -1685,11 +1683,6 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,gotoout;}-if(tb[NHA_GROUPS]||tb[NHA_MASTER]){-NL_SET_ERR_MSG(extack,"Invalid attributes in request");-gotoout;-}-memset(cfg,0,sizeof(*cfg));cfg->nlflags=nlh->nlmsg_flags;cfg->nlinfo.portid=NETLINK_CB(skb).portid;
From: Petr Machata <petrm@nvidia.com> Date: 2021-01-18 14:08:13
This function uses the global nexthop policy only to then bounce all
arguments except for NHA_ID. Instead, just create a new policy that
only includes the one allowed attribute.
Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
net/ipv4/nexthop.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
From: David Ahern <hidden> Date: 2021-01-18 17:43:07
On 1/18/21 7:05 AM, Petr Machata wrote:
This function uses the global nexthop policy only to then bounce all
arguments except for NHA_ID. Instead, just create a new policy that
only includes the one allowed attribute.
Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
net/ipv4/nexthop.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
From: David Ahern <hidden> Date: 2021-01-18 17:43:28
On 1/18/21 7:05 AM, Petr Machata wrote:
This function uses the global nexthop policy, but only accepts four
particular attributes. Create a new policy that only includes the four
supported attributes, and use it. Convert the loop to a series of ifs.
Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
net/ipv4/nexthop.c | 57 +++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 31 deletions(-)
From: David Ahern <hidden> Date: 2021-01-18 17:43:28
On 1/18/21 7:05 AM, Petr Machata wrote:
This policy is currently only used for creation of new next hops and new
next hop groups. Rename it accordingly and remove the two attributes that
are not valid in that context: NHA_GROUPS and NHA_MASTER.
Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
net/ipv4/nexthop.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
From: David Ahern <hidden> Date: 2021-01-18 17:44:46
On 1/18/21 7:05 AM, Petr Machata wrote:
From: Petr Machata <redacted>
There is currently one policy that covers all attributes for next hop
object management. Actual validation is then done in code, which makes it
unobvious which attributes are acceptable when, and indeed that everything
is rejected as necessary.
In this series, split rtm_nh_policy to several policies that cover various
aspects of the next hop object configuration, and instead of open-coding
the validation, defer to nlmsg_parse(). This should make extending the next
hop code simpler as well, which will be relevant in near future for
resilient hashing implementation.
This was tested by running tools/testing/selftests/net/fib_nexthops.sh.
Additionally iproute2 was tweaked to issue "nexthop list id" as an
RTM_GETNEXTHOP dump request, instead of a straight get to test that
unexpected attributes are indeed rejected.
In patch #1, convert attribute validation in nh_valid_get_del_req().
In patch #2, convert nh_valid_dump_req().
In patch #3, rtm_nh_policy is cleaned up and renamed to rtm_nh_policy_new,
because after the above two patches, that is the only context that it is
used in.
Petr Machata (3):
nexthop: Use a dedicated policy for nh_valid_get_del_req()
nexthop: Use a dedicated policy for nh_valid_dump_req()
nexthop: Specialize rtm_nh_policy
net/ipv4/nexthop.c | 85 +++++++++++++++++-----------------------------
1 file changed, 32 insertions(+), 53 deletions(-)
good cleanup. thanks for doing this. Did you run fib_nexthops.sh
selftests on the change? Seems right, but always good to run that script
which has functional tests about valid attribute combinations.
On Mon, Jan 18, 2021 at 10:43:22AM -0700, David Ahern wrote:
On 1/18/21 7:05 AM, Petr Machata wrote:
quoted
From: Petr Machata <redacted>
There is currently one policy that covers all attributes for next hop
object management. Actual validation is then done in code, which makes it
unobvious which attributes are acceptable when, and indeed that everything
is rejected as necessary.
In this series, split rtm_nh_policy to several policies that cover various
aspects of the next hop object configuration, and instead of open-coding
the validation, defer to nlmsg_parse(). This should make extending the next
hop code simpler as well, which will be relevant in near future for
resilient hashing implementation.
This was tested by running tools/testing/selftests/net/fib_nexthops.sh.
Additionally iproute2 was tweaked to issue "nexthop list id" as an
RTM_GETNEXTHOP dump request, instead of a straight get to test that
unexpected attributes are indeed rejected.
In patch #1, convert attribute validation in nh_valid_get_del_req().
In patch #2, convert nh_valid_dump_req().
In patch #3, rtm_nh_policy is cleaned up and renamed to rtm_nh_policy_new,
because after the above two patches, that is the only context that it is
used in.
Petr Machata (3):
nexthop: Use a dedicated policy for nh_valid_get_del_req()
nexthop: Use a dedicated policy for nh_valid_dump_req()
nexthop: Specialize rtm_nh_policy
net/ipv4/nexthop.c | 85 +++++++++++++++++-----------------------------
1 file changed, 32 insertions(+), 53 deletions(-)
good cleanup. thanks for doing this. Did you run fib_nexthops.sh
selftests on the change? Seems right, but always good to run that script
which has functional tests about valid attribute combinations.
"This was tested by running tools/testing/selftests/net/fib_nexthops.sh"
:)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-01-19 20:56:50
On Mon, 18 Jan 2021 15:05:23 +0100 Petr Machata wrote:
quoted hunk
This function uses the global nexthop policy only to then bounce all
arguments except for NHA_ID. Instead, just create a new policy that
only includes the one allowed attribute.
Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
net/ipv4/nexthop.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
This is an unnecessary waste of memory if you ask me.
NHA_ID is 1, so we're creating an array of 10 extra NULL elements.
Can you leave the size to the compiler and use ARRAY_SIZE() below?
This is an unnecessary waste of memory if you ask me.
NHA_ID is 1, so we're creating an array of 10 extra NULL elements.
Can you leave the size to the compiler and use ARRAY_SIZE() below?
interesting suggestion in general for netlink attributes.
According to tags on commit ff419afa4310 ("ethtool: trim policy
tables") the credit goes to Johannes :)
This is an unnecessary waste of memory if you ask me.
NHA_ID is 1, so we're creating an array of 10 extra NULL elements.
Can you leave the size to the compiler and use ARRAY_SIZE() below?
interesting suggestion in general for netlink attributes.
This is an unnecessary waste of memory if you ask me.
NHA_ID is 1, so we're creating an array of 10 extra NULL elements.
Can you leave the size to the compiler and use ARRAY_SIZE() below?
interesting suggestion in general for netlink attributes.