netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation
@ 2021-01-18 14:05 Petr Machata
  2021-01-18 14:05 ` [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req() Petr Machata
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Petr Machata @ 2021-01-18 14:05 UTC (permalink / raw)
  To: netdev
  Cc: David Ahern, David S. Miller, Jakub Kicinski, Ido Schimmel, Petr Machata

From: Petr Machata <petrm@nvidia.org>

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req()
  2021-01-18 14:05 [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation Petr Machata
@ 2021-01-18 14:05 ` Petr Machata
  2021-01-18 17:41   ` David Ahern
  2021-01-19 20:55   ` Jakub Kicinski
  2021-01-18 14:05 ` [PATCH net-next 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req() Petr Machata
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 15+ messages in thread
From: Petr Machata @ 2021-01-18 14:05 UTC (permalink / raw)
  To: netdev
  Cc: David Ahern, David S. Miller, Jakub Kicinski, Ido Schimmel, Petr Machata

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

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index e53e43aef785..d5d88f7c5c11 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -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] = {
+	[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;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH net-next 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req()
  2021-01-18 14:05 [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation Petr Machata
  2021-01-18 14:05 ` [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req() Petr Machata
@ 2021-01-18 14:05 ` Petr Machata
  2021-01-18 17:41   ` David Ahern
  2021-01-19 20:55   ` Jakub Kicinski
  2021-01-18 14:05 ` [PATCH net-next 3/3] nexthop: Specialize rtm_nh_policy Petr Machata
  2021-01-18 17:43 ` [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation David Ahern
  3 siblings, 2 replies; 15+ messages in thread
From: Petr Machata @ 2021-01-18 14:05 UTC (permalink / raw)
  To: netdev
  Cc: David Ahern, David S. Miller, Jakub Kicinski, Ido Schimmel, Petr Machata

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

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index d5d88f7c5c11..226d73cbc468 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -40,6 +40,13 @@ static const struct nla_policy rtm_nh_policy_get[NHA_MAX + 1] = {
 	[NHA_ID]		= { .type = NLA_U32 },
 };
 
+static const struct nla_policy rtm_nh_policy_dump[NHA_MAX + 1] = {
+	[NHA_OIF]		= { .type = NLA_U32 },
+	[NHA_GROUPS]		= { .type = NLA_FLAG },
+	[NHA_MASTER]		= { .type = NLA_U32 },
+	[NHA_FDB]		= { .type = NLA_FLAG },
+};
+
 static bool nexthop_notifiers_is_empty(struct net *net)
 {
 	return !net->nexthop.notifier_chain.head;
@@ -1984,46 +1991,34 @@ static int nh_valid_dump_req(const struct nlmsghdr *nlh, int *dev_idx,
 	struct netlink_ext_ack *extack = cb->extack;
 	struct nlattr *tb[NHA_MAX + 1];
 	struct nhmsg *nhm;
-	int err, i;
+	int err;
 	u32 idx;
 
-	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy,
+	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy_dump,
 			  NULL);
 	if (err < 0)
 		return err;
 
-	for (i = 0; i <= NHA_MAX; ++i) {
-		if (!tb[i])
-			continue;
-
-		switch (i) {
-		case NHA_OIF:
-			idx = nla_get_u32(tb[i]);
-			if (idx > INT_MAX) {
-				NL_SET_ERR_MSG(extack, "Invalid device index");
-				return -EINVAL;
-			}
-			*dev_idx = idx;
-			break;
-		case NHA_MASTER:
-			idx = nla_get_u32(tb[i]);
-			if (idx > INT_MAX) {
-				NL_SET_ERR_MSG(extack, "Invalid master device index");
-				return -EINVAL;
-			}
-			*master_idx = idx;
-			break;
-		case NHA_GROUPS:
-			*group_filter = true;
-			break;
-		case NHA_FDB:
-			*fdb_filter = true;
-			break;
-		default:
-			NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
+	if (tb[NHA_OIF]) {
+		idx = nla_get_u32(tb[NHA_OIF]);
+		if (idx > INT_MAX) {
+			NL_SET_ERR_MSG(extack, "Invalid device index");
+			return -EINVAL;
+		}
+		*dev_idx = idx;
+	}
+	if (tb[NHA_MASTER]) {
+		idx = nla_get_u32(tb[NHA_MASTER]);
+		if (idx > INT_MAX) {
+			NL_SET_ERR_MSG(extack, "Invalid master device index");
 			return -EINVAL;
 		}
+		*master_idx = idx;
 	}
+	if (tb[NHA_GROUPS])
+		*group_filter = true;
+	if (tb[NHA_FDB])
+		*fdb_filter = true;
 
 	nhm = nlmsg_data(nlh);
 	if (nhm->nh_protocol || nhm->resvd || nhm->nh_scope || nhm->nh_flags) {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH net-next 3/3] nexthop: Specialize rtm_nh_policy
  2021-01-18 14:05 [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation Petr Machata
  2021-01-18 14:05 ` [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req() Petr Machata
  2021-01-18 14:05 ` [PATCH net-next 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req() Petr Machata
@ 2021-01-18 14:05 ` Petr Machata
  2021-01-18 17:42   ` David Ahern
  2021-01-18 17:43 ` [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation David Ahern
  3 siblings, 1 reply; 15+ messages in thread
From: Petr Machata @ 2021-01-18 14:05 UTC (permalink / raw)
  To: netdev
  Cc: David Ahern, David S. Miller, Jakub Kicinski, Ido Schimmel, Petr Machata

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

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 226d73cbc468..0e5a574a4070 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -22,7 +22,7 @@ static void remove_nexthop(struct net *net, struct nexthop *nh,
 #define NH_DEV_HASHBITS  8
 #define NH_DEV_HASHSIZE (1U << NH_DEV_HASHBITS)
 
-static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = {
+static const struct nla_policy rtm_nh_policy_new[NHA_MAX + 1] = {
 	[NHA_ID]		= { .type = NLA_U32 },
 	[NHA_GROUP]		= { .type = NLA_BINARY },
 	[NHA_GROUP_TYPE]	= { .type = NLA_U16 },
@@ -31,8 +31,6 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = {
 	[NHA_GATEWAY]		= { .type = NLA_BINARY },
 	[NHA_ENCAP_TYPE]	= { .type = NLA_U16 },
 	[NHA_ENCAP]		= { .type = NLA_NESTED },
-	[NHA_GROUPS]		= { .type = NLA_FLAG },
-	[NHA_MASTER]		= { .type = NLA_U32 },
 	[NHA_FDB]		= { .type = NLA_FLAG },
 };
 
@@ -1657,7 +1655,7 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 	struct nlattr *tb[NHA_MAX + 1];
 	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_new,
 			  extack);
 	if (err < 0)
 		return err;
@@ -1685,11 +1683,6 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 		goto out;
 	}
 
-	if (tb[NHA_GROUPS] || tb[NHA_MASTER]) {
-		NL_SET_ERR_MSG(extack, "Invalid attributes in request");
-		goto out;
-	}
-
 	memset(cfg, 0, sizeof(*cfg));
 	cfg->nlflags = nlh->nlmsg_flags;
 	cfg->nlinfo.portid = NETLINK_CB(skb).portid;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req()
  2021-01-18 14:05 ` [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req() Petr Machata
@ 2021-01-18 17:41   ` David Ahern
  2021-01-19 20:55   ` Jakub Kicinski
  1 sibling, 0 replies; 15+ messages in thread
From: David Ahern @ 2021-01-18 17:41 UTC (permalink / raw)
  To: Petr Machata, netdev
  Cc: David Ahern, David S. Miller, Jakub Kicinski, Ido Schimmel

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

Reviewed-by: David Ahern <dsahern@kernel.org>



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req()
  2021-01-18 14:05 ` [PATCH net-next 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req() Petr Machata
@ 2021-01-18 17:41   ` David Ahern
  2021-01-19 20:55   ` Jakub Kicinski
  1 sibling, 0 replies; 15+ messages in thread
From: David Ahern @ 2021-01-18 17:41 UTC (permalink / raw)
  To: Petr Machata, netdev
  Cc: David Ahern, David S. Miller, Jakub Kicinski, Ido Schimmel

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

Reviewed-by: David Ahern <dsahern@kernel.org>



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 3/3] nexthop: Specialize rtm_nh_policy
  2021-01-18 14:05 ` [PATCH net-next 3/3] nexthop: Specialize rtm_nh_policy Petr Machata
@ 2021-01-18 17:42   ` David Ahern
  0 siblings, 0 replies; 15+ messages in thread
From: David Ahern @ 2021-01-18 17:42 UTC (permalink / raw)
  To: Petr Machata, netdev
  Cc: David Ahern, David S. Miller, Jakub Kicinski, Ido Schimmel

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

Reviewed-by: David Ahern <dsahern@kernel.org>



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation
  2021-01-18 14:05 [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation Petr Machata
                   ` (2 preceding siblings ...)
  2021-01-18 14:05 ` [PATCH net-next 3/3] nexthop: Specialize rtm_nh_policy Petr Machata
@ 2021-01-18 17:43 ` David Ahern
  2021-01-18 18:29   ` Ido Schimmel
  3 siblings, 1 reply; 15+ messages in thread
From: David Ahern @ 2021-01-18 17:43 UTC (permalink / raw)
  To: Petr Machata, netdev
  Cc: David Ahern, David S. Miller, Jakub Kicinski, Ido Schimmel, Petr Machata

On 1/18/21 7:05 AM, Petr Machata wrote:
> From: Petr Machata <petrm@nvidia.org>
> 
> 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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation
  2021-01-18 17:43 ` [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation David Ahern
@ 2021-01-18 18:29   ` Ido Schimmel
  0 siblings, 0 replies; 15+ messages in thread
From: Ido Schimmel @ 2021-01-18 18:29 UTC (permalink / raw)
  To: David Ahern
  Cc: Petr Machata, netdev, David Ahern, David S. Miller,
	Jakub Kicinski, Ido Schimmel, Petr Machata

On Mon, Jan 18, 2021 at 10:43:22AM -0700, David Ahern wrote:
> On 1/18/21 7:05 AM, Petr Machata wrote:
> > From: Petr Machata <petrm@nvidia.org>
> > 
> > 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"
:)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req()
  2021-01-18 14:05 ` [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req() Petr Machata
  2021-01-18 17:41   ` David Ahern
@ 2021-01-19 20:55   ` Jakub Kicinski
  2021-01-20  2:28     ` David Ahern
  1 sibling, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2021-01-19 20:55 UTC (permalink / raw)
  To: Petr Machata; +Cc: netdev, David Ahern, David S. Miller, Ido Schimmel

On Mon, 18 Jan 2021 15:05:23 +0100 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(-)
> 
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index e53e43aef785..d5d88f7c5c11 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -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.

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?

> +	[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;


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req()
  2021-01-18 14:05 ` [PATCH net-next 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req() Petr Machata
  2021-01-18 17:41   ` David Ahern
@ 2021-01-19 20:55   ` Jakub Kicinski
  2021-01-20 10:46     ` Petr Machata
  1 sibling, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2021-01-19 20:55 UTC (permalink / raw)
  To: Petr Machata; +Cc: netdev, David Ahern, David S. Miller, Ido Schimmel

On Mon, 18 Jan 2021 15:05:24 +0100 Petr Machata wrote:
> +	if (tb[NHA_GROUPS])
> +		*group_filter = true;
> +	if (tb[NHA_FDB])
> +		*fdb_filter = true;

nla_get_flag()

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req()
  2021-01-19 20:55   ` Jakub Kicinski
@ 2021-01-20  2:28     ` David Ahern
  2021-01-20  2:35       ` Jakub Kicinski
  2021-01-20 10:45       ` Petr Machata
  0 siblings, 2 replies; 15+ messages in thread
From: David Ahern @ 2021-01-20  2:28 UTC (permalink / raw)
  To: Jakub Kicinski, Petr Machata
  Cc: netdev, David Ahern, David S. Miller, Ido Schimmel

On 1/19/21 1:55 PM, Jakub Kicinski wrote:
>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>> index e53e43aef785..d5d88f7c5c11 100644
>> --- a/net/ipv4/nexthop.c
>> +++ b/net/ipv4/nexthop.c
>> @@ -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.
> 
> 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.

> 
>> +	[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.

>> -	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;
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req()
  2021-01-20  2:28     ` David Ahern
@ 2021-01-20  2:35       ` Jakub Kicinski
  2021-01-20 10:45       ` Petr Machata
  1 sibling, 0 replies; 15+ messages in thread
From: Jakub Kicinski @ 2021-01-20  2:35 UTC (permalink / raw)
  To: David Ahern
  Cc: Petr Machata, netdev, David Ahern, David S. Miller, Ido Schimmel

On Tue, 19 Jan 2021 19:28:40 -0700 David Ahern wrote:
> On 1/19/21 1:55 PM, Jakub Kicinski wrote:
> >> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> >> index e53e43aef785..d5d88f7c5c11 100644
> >> --- a/net/ipv4/nexthop.c
> >> +++ b/net/ipv4/nexthop.c
> >> @@ -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.
> > 
> > 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 :)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req()
  2021-01-20  2:28     ` David Ahern
  2021-01-20  2:35       ` Jakub Kicinski
@ 2021-01-20 10:45       ` Petr Machata
  1 sibling, 0 replies; 15+ messages in thread
From: Petr Machata @ 2021-01-20 10:45 UTC (permalink / raw)
  To: David Ahern
  Cc: Jakub Kicinski, Petr Machata, netdev, David Ahern,
	David S. Miller, Ido Schimmel


David Ahern <dsahern@gmail.com> writes:

> On 1/19/21 1:55 PM, Jakub Kicinski wrote:
>>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>>> index e53e43aef785..d5d88f7c5c11 100644
>>> --- a/net/ipv4/nexthop.c
>>> +++ b/net/ipv4/nexthop.c
>>> @@ -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.
>> 
>> 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.
>
>> 
>>> +	[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.
>
>>> -	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);

OK, I'll send a v2 that uses ARRAY_SIZE instead of hard-coding the max
and size.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req()
  2021-01-19 20:55   ` Jakub Kicinski
@ 2021-01-20 10:46     ` Petr Machata
  0 siblings, 0 replies; 15+ messages in thread
From: Petr Machata @ 2021-01-20 10:46 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Petr Machata, netdev, David Ahern, David S. Miller, Ido Schimmel


Jakub Kicinski <kuba@kernel.org> writes:

> On Mon, 18 Jan 2021 15:05:24 +0100 Petr Machata wrote:
>> +	if (tb[NHA_GROUPS])
>> +		*group_filter = true;
>> +	if (tb[NHA_FDB])
>> +		*fdb_filter = true;
>
> nla_get_flag()

OK.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-01-20 11:21 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-18 14:05 [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation Petr Machata
2021-01-18 14:05 ` [PATCH net-next 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req() Petr Machata
2021-01-18 17:41   ` David Ahern
2021-01-19 20:55   ` Jakub Kicinski
2021-01-20  2:28     ` David Ahern
2021-01-20  2:35       ` Jakub Kicinski
2021-01-20 10:45       ` Petr Machata
2021-01-18 14:05 ` [PATCH net-next 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req() Petr Machata
2021-01-18 17:41   ` David Ahern
2021-01-19 20:55   ` Jakub Kicinski
2021-01-20 10:46     ` Petr Machata
2021-01-18 14:05 ` [PATCH net-next 3/3] nexthop: Specialize rtm_nh_policy Petr Machata
2021-01-18 17:42   ` David Ahern
2021-01-18 17:43 ` [PATCH net-next 0/3] nexthop: More fine-grained policies for netlink message validation David Ahern
2021-01-18 18:29   ` Ido Schimmel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).