nexthop: More fine-grained policies for netlink message validation

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 <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 net/ipv4/nexthop.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

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 <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 net/ipv4/nexthop.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

net/ipv4/nexthop.c +0 -0

index e53e43aef785..d5d88f7c5c11 100644

net/ipv4/nexthop.c +5 -15

+	[NHA_ID]		= { .type = NLA_U32 },
+};
+
 static bool nexthop_notifiers_is_empty(struct net *net)
 {
 	return !net->nexthop.notifier_chain.head;

@@ -1843,27 +1847,14 @@ static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id,
 {
 	struct nhmsg *nhm = nlmsg_data(nlh);
 	struct nlattr *tb[NHA_MAX + 1];
-	int err, i;
+	int err;
 
-	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy,
+	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy_get,
 			  extack);
 	if (err < 0)
 		return err;
 
 	err = -EINVAL;
-	for (i = 0; i < __NHA_MAX; ++i) {
-		if (!tb[i])
-			continue;
-
-		switch (i) {
-		case NHA_ID:
-			break;
-		default:
-			NL_SET_ERR_MSG_ATTR(extack, tb[i],
-					    "Unexpected attribute in request");
-			goto out;
-		}
-	}
 	if (nhm->nh_protocol || nhm->resvd || nhm->nh_scope || nhm->nh_flags) {
 		NL_SET_ERR_MSG(extack, "Invalid values in header");
 		goto out;

Quoted 8 lines

net/ipv4/nexthop.c +0 -0

index e53e43aef785..d5d88f7c5c11 100644
@@ -36,6 +36,10 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = {
 	[NHA_FDB]		= { .type = NLA_FLAG },
 };
 

net/ipv4/nexthop.c static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = { · line 39

Quoted 1 line

net/ipv4/nexthop.c +1 -0

+static const struct nla_policy rtm_nh_policy_get[NHA_MAX + 1] = {

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?

net/ipv4/nexthop.c static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id, · line 1849

Quoted 11 lines

net/ipv4/nexthop.c +3 -0

+	[NHA_ID]		= { .type = NLA_U32 },
+};
+
 static bool nexthop_notifiers_is_empty(struct net *net)
 {
 	return !net->nexthop.notifier_chain.head;

@@ -1843,27 +1847,14 @@ static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id,
 {
 	struct nhmsg *nhm = nlmsg_data(nlh);
 	struct nlattr *tb[NHA_MAX + 1];

Quoted 26 lines

net/ipv4/nexthop.c +2 -15

-	int err, i;
+	int err;
 
-	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy,
+	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy_get,
 			  extack);
 	if (err < 0)
 		return err;
 
 	err = -EINVAL;
-	for (i = 0; i < __NHA_MAX; ++i) {
-		if (!tb[i])
-			continue;
-
-		switch (i) {
-		case NHA_ID:
-			break;
-		default:
-			NL_SET_ERR_MSG_ATTR(extack, tb[i],
-					    "Unexpected attribute in request");
-			goto out;
-		}
-	}
 	if (nhm->nh_protocol || nhm->resvd || nhm->nh_scope || nhm->nh_flags) {
 		NL_SET_ERR_MSG(extack, "Invalid values in header");
 		goto out;

Quoted 12 lines · Jakub Kicinski

net/ipv4/nexthop.c static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = { · line 39

Quoted 9 lines

net/ipv4/nexthop.c +1 -0

index e53e43aef785..d5d88f7c5c11 100644
@@ -36,6 +36,10 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = {
 	[NHA_FDB]		= { .type = NLA_FLAG },
 };
 
+static const struct nla_policy rtm_nh_policy_get[NHA_MAX + 1] = {  

This is an unnecessary waste of memory if you ask me.

Quoted 3 lines

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.

Quoted 8 lines · Jakub Kicinski

Quoted 8 lines

net/ipv4/nexthop.c +0 -0

index e53e43aef785..d5d88f7c5c11 100644
@@ -36,6 +36,10 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = {
 	[NHA_FDB]		= { .type = NLA_FLAG },
 };
 

Quoted 7 lines

net/ipv4/nexthop.c static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = { · line 39

Quoted 1 line

net/ipv4/nexthop.c +1 -0

+static const struct nla_policy rtm_nh_policy_get[NHA_MAX + 1] = {

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.

Quoted 11 lines

net/ipv4/nexthop.c static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id, · line 1849

Quoted 11 lines

net/ipv4/nexthop.c +3 -0

+	[NHA_ID]		= { .type = NLA_U32 },
+};
+
 static bool nexthop_notifiers_is_empty(struct net *net)
 {
 	return !net->nexthop.notifier_chain.head;

@@ -1843,27 +1847,14 @@ static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id,
 {
 	struct nhmsg *nhm = nlmsg_data(nlh);
 	struct nlattr *tb[NHA_MAX + 1];

This tb array too could be sized to just the highest indexed expected - NHA_ID in this case.

Quoted 6 lines

net/ipv4/nexthop.c static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id, · line 1853

Quoted 6 lines

net/ipv4/nexthop.c +2 -2

-	int err, i;
+	int err;
 
-	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy,
+	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy_get,
 			  extack);

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 <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 net/ipv4/nexthop.c | 57 +++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

net/ipv4/nexthop.c static int nh_valid_dump_req(const struct nlmsghdr *nlh, int *dev_idx, · line 2021

Quoted 4 lines · On Mon, 18 Jan 2021 15:05:24 +0100 Petr Machata wrote:

net/ipv4/nexthop.c +4 -0

+	if (tb[NHA_GROUPS])
+		*group_filter = true;
+	if (tb[NHA_FDB])
+		*fdb_filter = true;

nla_get_flag()

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 <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 net/ipv4/nexthop.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

From: Petr Machata <[email protected]>

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(-)

Quoted 31 lines · Petr Machata

From: Petr Machata <[email protected]>

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

Quoted 3 lines

 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.