[PATCH bpf-next 0/2] netkit: two minor cleanups

[PATCH bpf-next 0/2] netkit: two minor cleanups

[Nikolay Aleksandrov]

Hi,
This set does two minor cleanups mentioned by Jiri. The first patch
removes explicit NULLing of primary/peer pointers and relies on the
implicit mem zeroing done at net device alloc. The second patch switches
netkit's mode and primary/peer policy netlink attributes to use
NLA_POLICY_VALIDATE_FN() type and sets the custom validate function to
return better user errors. This way netlink's policy is used to validate
the attributes and simplifies the code a bit. No functional changes are
intended.

Thanks,
 Nik


Nikolay Aleksandrov (2):
  netkit: remove explicit active/peer ptr initialization
  netkit: use netlink policy for mode and policy attributes validation

 drivers/net/netkit.c | 70 ++++++++++++++------------------------------
 1 file changed, 22 insertions(+), 48 deletions(-)

-- 
2.38.1

[PATCH bpf-next 1/2] netkit: remove explicit active/peer ptr initialization

[Nikolay Aleksandrov]

Remove the explicit NULLing of active/peer pointers and rely on the
implicit one done at net device allocation.

Suggested-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 drivers/net/netkit.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c
index 7e484f9fd3ae..5a0f86f38f09 100644
--- a/drivers/net/netkit.c
+++ b/drivers/net/netkit.c
@@ -371,8 +371,6 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 	nk->policy = default_peer;
 	nk->mode = mode;
 	bpf_mprog_bundle_init(&nk->bundle);
-	RCU_INIT_POINTER(nk->active, NULL);
-	RCU_INIT_POINTER(nk->peer, NULL);
 
 	err = register_netdevice(peer);
 	put_net(net);
@@ -398,8 +396,6 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 	nk->policy = default_prim;
 	nk->mode = mode;
 	bpf_mprog_bundle_init(&nk->bundle);
-	RCU_INIT_POINTER(nk->active, NULL);
-	RCU_INIT_POINTER(nk->peer, NULL);
 
 	err = register_netdevice(dev);
 	if (err < 0)
-- 
2.38.1

Re: [PATCH bpf-next 1/2] netkit: remove explicit active/peer ptr initialization

[Daniel Borkmann]

On 10/26/23 11:41 AM, Nikolay Aleksandrov wrote:
> Remove the explicit NULLing of active/peer pointers and rely on the
> implicit one done at net device allocation.
> 
> Suggested-by: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH bpf-next 1/2] netkit: remove explicit active/peer ptr initialization

[Jiri Pirko]

Thu, Oct 26, 2023 at 11:41:05AM CEST, razor@blackwall.org wrote:
>Remove the explicit NULLing of active/peer pointers and rely on the
>implicit one done at net device allocation.
>
>Suggested-by: Jiri Pirko <jiri@resnulli.us>
>Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

[PATCH bpf-next 2/2] netkit: use netlink policy for mode and policy attributes validation

[Nikolay Aleksandrov]

Use netlink's NLA_POLICY_VALIDATE_FN() type for mode and primary/peer
policy with custom validation functions to return better errors. This
simplifies the logic a bit and relies on netlink's policy validation.
We don't have to specify len because the type is NLA_U32 and attribute
length is enforced by netlink.

Suggested-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 drivers/net/netkit.c | 66 +++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 44 deletions(-)

diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c
index 5a0f86f38f09..1ce116e68f95 100644
--- a/drivers/net/netkit.c
+++ b/drivers/net/netkit.c
@@ -247,29 +247,29 @@ static struct net *netkit_get_link_net(const struct net_device *dev)
 	return peer ? dev_net(peer) : dev_net(dev);
 }
 
-static int netkit_check_policy(int policy, struct nlattr *tb,
+static int netkit_check_policy(const struct nlattr *attr,
 			       struct netlink_ext_ack *extack)
 {
-	switch (policy) {
+	switch (nla_get_u32(attr)) {
 	case NETKIT_PASS:
 	case NETKIT_DROP:
 		return 0;
 	default:
-		NL_SET_ERR_MSG_ATTR(extack, tb,
+		NL_SET_ERR_MSG_ATTR(extack, attr,
 				    "Provided default xmit policy not supported");
 		return -EINVAL;
 	}
 }
 
-static int netkit_check_mode(int mode, struct nlattr *tb,
+static int netkit_check_mode(const struct nlattr *attr,
 			     struct netlink_ext_ack *extack)
 {
-	switch (mode) {
+	switch (nla_get_u32(attr)) {
 	case NETKIT_L2:
 	case NETKIT_L3:
 		return 0;
 	default:
-		NL_SET_ERR_MSG_ATTR(extack, tb,
+		NL_SET_ERR_MSG_ATTR(extack, attr,
 				    "Provided device mode can only be L2 or L3");
 		return -EINVAL;
 	}
@@ -306,13 +306,8 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 	int err;
 
 	if (data) {
-		if (data[IFLA_NETKIT_MODE]) {
-			attr = data[IFLA_NETKIT_MODE];
-			mode = nla_get_u32(attr);
-			err = netkit_check_mode(mode, attr, extack);
-			if (err < 0)
-				return err;
-		}
+		if (data[IFLA_NETKIT_MODE])
+			mode = nla_get_u32(data[IFLA_NETKIT_MODE]);
 		if (data[IFLA_NETKIT_PEER_INFO]) {
 			attr = data[IFLA_NETKIT_PEER_INFO];
 			ifmp = nla_data(attr);
@@ -324,20 +319,10 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 				return err;
 			tbp = peer_tb;
 		}
-		if (data[IFLA_NETKIT_POLICY]) {
-			attr = data[IFLA_NETKIT_POLICY];
-			default_prim = nla_get_u32(attr);
-			err = netkit_check_policy(default_prim, attr, extack);
-			if (err < 0)
-				return err;
-		}
-		if (data[IFLA_NETKIT_PEER_POLICY]) {
-			attr = data[IFLA_NETKIT_PEER_POLICY];
-			default_peer = nla_get_u32(attr);
-			err = netkit_check_policy(default_peer, attr, extack);
-			if (err < 0)
-				return err;
-		}
+		if (data[IFLA_NETKIT_POLICY])
+			default_prim = nla_get_u32(data[IFLA_NETKIT_POLICY]);
+		if (data[IFLA_NETKIT_PEER_POLICY])
+			default_peer = nla_get_u32(data[IFLA_NETKIT_PEER_POLICY]);
 	}
 
 	if (ifmp && tbp[IFLA_IFNAME]) {
@@ -818,8 +803,6 @@ static int netkit_change_link(struct net_device *dev, struct nlattr *tb[],
 	struct netkit *nk = netkit_priv(dev);
 	struct net_device *peer = rtnl_dereference(nk->peer);
 	enum netkit_action policy;
-	struct nlattr *attr;
-	int err;
 
 	if (!nk->primary) {
 		NL_SET_ERR_MSG(extack,
@@ -834,22 +817,14 @@ static int netkit_change_link(struct net_device *dev, struct nlattr *tb[],
 	}
 
 	if (data[IFLA_NETKIT_POLICY]) {
-		attr = data[IFLA_NETKIT_POLICY];
-		policy = nla_get_u32(attr);
-		err = netkit_check_policy(policy, attr, extack);
-		if (err)
-			return err;
+		policy = nla_get_u32(data[IFLA_NETKIT_POLICY]);
 		WRITE_ONCE(nk->policy, policy);
 	}
 
 	if (data[IFLA_NETKIT_PEER_POLICY]) {
-		err = -EOPNOTSUPP;
-		attr = data[IFLA_NETKIT_PEER_POLICY];
-		policy = nla_get_u32(attr);
-		if (peer)
-			err = netkit_check_policy(policy, attr, extack);
-		if (err)
-			return err;
+		if (!peer)
+			return -EOPNOTSUPP;
+		policy = nla_get_u32(data[IFLA_NETKIT_PEER_POLICY]);
 		nk = netkit_priv(peer);
 		WRITE_ONCE(nk->policy, policy);
 	}
@@ -889,9 +864,12 @@ static int netkit_fill_info(struct sk_buff *skb, const struct net_device *dev)
 
 static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = {
 	[IFLA_NETKIT_PEER_INFO]		= { .len = sizeof(struct ifinfomsg) },
-	[IFLA_NETKIT_POLICY]		= { .type = NLA_U32 },
-	[IFLA_NETKIT_MODE]		= { .type = NLA_U32 },
-	[IFLA_NETKIT_PEER_POLICY]	= { .type = NLA_U32 },
+	[IFLA_NETKIT_POLICY]		= NLA_POLICY_VALIDATE_FN(NLA_U32,
+								 netkit_check_policy),
+	[IFLA_NETKIT_MODE]		= NLA_POLICY_VALIDATE_FN(NLA_U32,
+								 netkit_check_mode),
+	[IFLA_NETKIT_PEER_POLICY]	= NLA_POLICY_VALIDATE_FN(NLA_U32,
+								 netkit_check_policy),
 	[IFLA_NETKIT_PRIMARY]		= { .type = NLA_REJECT,
 					    .reject_message = "Primary attribute is read-only" },
 };
-- 
2.38.1

Re: [PATCH bpf-next 2/2] netkit: use netlink policy for mode and policy attributes validation

[Daniel Borkmann]

On 10/26/23 11:41 AM, Nikolay Aleksandrov wrote:
> Use netlink's NLA_POLICY_VALIDATE_FN() type for mode and primary/peer
> policy with custom validation functions to return better errors. This
> simplifies the logic a bit and relies on netlink's policy validation.
> We don't have to specify len because the type is NLA_U32 and attribute
> length is enforced by netlink.
> 
> Suggested-by: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

> ---
>   drivers/net/netkit.c | 66 +++++++++++++++-----------------------------
>   1 file changed, 22 insertions(+), 44 deletions(-)

Looks better indeed, shrinks code a bit, and passes the selftests, thanks
for the suggestion!

Thanks,
Daniel

Re: [PATCH bpf-next 2/2] netkit: use netlink policy for mode and policy attributes validation

[Jiri Pirko]

Thu, Oct 26, 2023 at 11:41:06AM CEST, razor@blackwall.org wrote:
>Use netlink's NLA_POLICY_VALIDATE_FN() type for mode and primary/peer
>policy with custom validation functions to return better errors. This
>simplifies the logic a bit and relies on netlink's policy validation.
>We don't have to specify len because the type is NLA_U32 and attribute
>length is enforced by netlink.
>
>Suggested-by: Jiri Pirko <jiri@resnulli.us>
>Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH bpf-next 2/2] netkit: use netlink policy for mode and policy attributes validation

[Ido Schimmel]

On Thu, Oct 26, 2023 at 12:41:06PM +0300, Nikolay Aleksandrov wrote:
>  static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = {
>  	[IFLA_NETKIT_PEER_INFO]		= { .len = sizeof(struct ifinfomsg) },
> -	[IFLA_NETKIT_POLICY]		= { .type = NLA_U32 },
> -	[IFLA_NETKIT_MODE]		= { .type = NLA_U32 },
> -	[IFLA_NETKIT_PEER_POLICY]	= { .type = NLA_U32 },
> +	[IFLA_NETKIT_POLICY]		= NLA_POLICY_VALIDATE_FN(NLA_U32,
> +								 netkit_check_policy),

Nik, it's problematic to use NLA_POLICY_VALIDATE_FN() with anything
other than NLA_BINARY. See commit 9e17f99220d1 ("net/sched: act_mpls:
Fix warning during failed attribute validation").

> +	[IFLA_NETKIT_MODE]		= NLA_POLICY_VALIDATE_FN(NLA_U32,
> +								 netkit_check_mode),
> +	[IFLA_NETKIT_PEER_POLICY]	= NLA_POLICY_VALIDATE_FN(NLA_U32,
> +								 netkit_check_policy),
>  	[IFLA_NETKIT_PRIMARY]		= { .type = NLA_REJECT,
>  					    .reject_message = "Primary attribute is read-only" },
>  };
> -- 
> 2.38.1
> 
> 

Re: [PATCH bpf-next 2/2] netkit: use netlink policy for mode and policy attributes validation

[Nikolay Aleksandrov]

On 10/26/23 17:11, Ido Schimmel wrote:
> On Thu, Oct 26, 2023 at 12:41:06PM +0300, Nikolay Aleksandrov wrote:
>>   static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = {
>>   	[IFLA_NETKIT_PEER_INFO]		= { .len = sizeof(struct ifinfomsg) },
>> -	[IFLA_NETKIT_POLICY]		= { .type = NLA_U32 },
>> -	[IFLA_NETKIT_MODE]		= { .type = NLA_U32 },
>> -	[IFLA_NETKIT_PEER_POLICY]	= { .type = NLA_U32 },
>> +	[IFLA_NETKIT_POLICY]		= NLA_POLICY_VALIDATE_FN(NLA_U32,
>> +								 netkit_check_policy),
> 
> Nik, it's problematic to use NLA_POLICY_VALIDATE_FN() with anything
> other than NLA_BINARY. See commit 9e17f99220d1 ("net/sched: act_mpls:
> Fix warning during failed attribute validation").
> 

But how is that code called at all? The validation type is 
NLA_VALIDATE_FUNCTION(), not NLA_VALIDATE_MIN/MAX/RANGE/RANGE_WARN...
nla_validate_int_range() is called only on:
         case NLA_VALIDATE_RANGE_PTR:
         case NLA_VALIDATE_RANGE:
         case NLA_VALIDATE_RANGE_WARN_TOO_LONG:
         case NLA_VALIDATE_MIN:
         case NLA_VALIDATE_MAX:

Anyway, I'll switch to NLA_BINARY in a bit to make sure it's ok. Thanks 
for the pointer.

>> +	[IFLA_NETKIT_MODE]		= NLA_POLICY_VALIDATE_FN(NLA_U32,
>> +								 netkit_check_mode),
>> +	[IFLA_NETKIT_PEER_POLICY]	= NLA_POLICY_VALIDATE_FN(NLA_U32,
>> +								 netkit_check_policy),
>>   	[IFLA_NETKIT_PRIMARY]		= { .type = NLA_REJECT,
>>   					    .reject_message = "Primary attribute is read-only" },
>>   };
>> -- 
>> 2.38.1
>>
>>

Re: [PATCH bpf-next 2/2] netkit: use netlink policy for mode and policy attributes validation

[Daniel Borkmann]

On 10/26/23 4:23 PM, Nikolay Aleksandrov wrote:
> On 10/26/23 17:11, Ido Schimmel wrote:
>> On Thu, Oct 26, 2023 at 12:41:06PM +0300, Nikolay Aleksandrov wrote:
>>>   static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = {
>>>       [IFLA_NETKIT_PEER_INFO]        = { .len = sizeof(struct ifinfomsg) },
>>> -    [IFLA_NETKIT_POLICY]        = { .type = NLA_U32 },
>>> -    [IFLA_NETKIT_MODE]        = { .type = NLA_U32 },
>>> -    [IFLA_NETKIT_PEER_POLICY]    = { .type = NLA_U32 },
>>> +    [IFLA_NETKIT_POLICY]        = NLA_POLICY_VALIDATE_FN(NLA_U32,
>>> +                                 netkit_check_policy),
>>
>> Nik, it's problematic to use NLA_POLICY_VALIDATE_FN() with anything
>> other than NLA_BINARY. See commit 9e17f99220d1 ("net/sched: act_mpls:
>> Fix warning during failed attribute validation").
> 
> But how is that code called at all? The validation type is NLA_VALIDATE_FUNCTION(), not NLA_VALIDATE_MIN/MAX/RANGE/RANGE_WARN...
> nla_validate_int_range() is called only on:
>          case NLA_VALIDATE_RANGE_PTR:
>          case NLA_VALIDATE_RANGE:
>          case NLA_VALIDATE_RANGE_WARN_TOO_LONG:
>          case NLA_VALIDATE_MIN:
>          case NLA_VALIDATE_MAX:
> 
> Anyway, I'll switch to NLA_BINARY in a bit to make sure it's ok. Thanks for the pointer.

Sg, I took in the NULL removal one for now.

Thanks,
Daniel

Re: [PATCH bpf-next 2/2] netkit: use netlink policy for mode and policy attributes validation

[Nikolay Aleksandrov]

On 10/26/23 17:23, Nikolay Aleksandrov wrote:
> On 10/26/23 17:11, Ido Schimmel wrote:
>> On Thu, Oct 26, 2023 at 12:41:06PM +0300, Nikolay Aleksandrov wrote:
>>>   static const struct nla_policy netkit_policy[IFLA_NETKIT_MAX + 1] = {
>>>       [IFLA_NETKIT_PEER_INFO]        = { .len = sizeof(struct 
>>> ifinfomsg) },
>>> -    [IFLA_NETKIT_POLICY]        = { .type = NLA_U32 },
>>> -    [IFLA_NETKIT_MODE]        = { .type = NLA_U32 },
>>> -    [IFLA_NETKIT_PEER_POLICY]    = { .type = NLA_U32 },
>>> +    [IFLA_NETKIT_POLICY]        = NLA_POLICY_VALIDATE_FN(NLA_U32,
>>> +                                 netkit_check_policy),
>>
>> Nik, it's problematic to use NLA_POLICY_VALIDATE_FN() with anything
>> other than NLA_BINARY. See commit 9e17f99220d1 ("net/sched: act_mpls:
>> Fix warning during failed attribute validation").
>>
> 
> But how is that code called at all? The validation type is 
> NLA_VALIDATE_FUNCTION(), not NLA_VALIDATE_MIN/MAX/RANGE/RANGE_WARN...
> nla_validate_int_range() is called only on:
>          case NLA_VALIDATE_RANGE_PTR:
>          case NLA_VALIDATE_RANGE:
>          case NLA_VALIDATE_RANGE_WARN_TOO_LONG:
>          case NLA_VALIDATE_MIN:
>          case NLA_VALIDATE_MAX:
> 

Ah, I'm looking at the wrong thing.. I saw the problem. :)

> Anyway, I'll switch to NLA_BINARY in a bit to make sure it's ok. Thanks 
> for the pointer.
> 
>>> +    [IFLA_NETKIT_MODE]        = NLA_POLICY_VALIDATE_FN(NLA_U32,
>>> +                                 netkit_check_mode),
>>> +    [IFLA_NETKIT_PEER_POLICY]    = NLA_POLICY_VALIDATE_FN(NLA_U32,
>>> +                                 netkit_check_policy),
>>>       [IFLA_NETKIT_PRIMARY]        = { .type = NLA_REJECT,
>>>                           .reject_message = "Primary attribute is 
>>> read-only" },
>>>   };
>>> -- 
>>> 2.38.1
>>>
>>>
> 

Re: [PATCH bpf-next 0/2] netkit: two minor cleanups

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Thu, 26 Oct 2023 12:41:04 +0300 you wrote:
> Hi,
> This set does two minor cleanups mentioned by Jiri. The first patch
> removes explicit NULLing of primary/peer pointers and relies on the
> implicit mem zeroing done at net device alloc. The second patch switches
> netkit's mode and primary/peer policy netlink attributes to use
> NLA_POLICY_VALIDATE_FN() type and sets the custom validate function to
> return better user errors. This way netlink's policy is used to validate
> the attributes and simplifies the code a bit. No functional changes are
> intended.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/2] netkit: remove explicit active/peer ptr initialization
    https://git.kernel.org/bpf/bpf-next/c/ea41b880cc85
  - [bpf-next,2/2] netkit: use netlink policy for mode and policy attributes validation
    https://git.kernel.org/bpf/bpf-next/c/3de07b963ab8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html