diff mbox series

[net-next,1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req()

Message ID ec93d227609126c98805e52ba3821b71f8bb338d.1610978306.git.petrm@nvidia.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series nexthop: More fine-grained policies for netlink message validation | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: yoshfuji@linux-ipv6.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 39 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Petr Machata Jan. 18, 2021, 2:05 p.m. UTC 
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(-)

Comments

David Ahern Jan. 18, 2021, 5:41 p.m. UTC | #1 
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>
Jakub Kicinski Jan. 19, 2021, 8:55 p.m. UTC | #2 
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;
David Ahern Jan. 20, 2021, 2:28 a.m. UTC | #3 
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;
>
Jakub Kicinski Jan. 20, 2021, 2:35 a.m. UTC | #4 
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 :)
Petr Machata Jan. 20, 2021, 10:45 a.m. UTC | #5 
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.
diff mbox series

Patch

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;