[net-next,2/7] net: nexthop: Add NHA_OP_FLAGS

Commit Message

Petr Machata Feb. 27, 2024, 6:17 p.m. UTC

In order to add per-nexthop statistics, but still not increase netlink
message size for consumers that do not care about them, there needs to be a
toggle through which the user indicates their desire to get the statistics.
To that end, add a new attribute, NHA_OP_FLAGS. The idea is to be able to
use the attribute for carrying of arbitrary operation-specific flags, i.e.
not make it specific for get / dump.

Add the new attribute to get and dump policies, but do not actually allow
any flags yet -- those will come later as the flags themselves are defined.
Add the necessary parsing code.

Signed-off-by: Petr Machata <[e-mail]>
Reviewed-by: Ido Schimmel <[e-mail]>
---
 include/uapi/linux/nexthop.h |  3 +++
 net/ipv4/nexthop.c           | 24 ++++++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

Comments

Jakub Kicinski Feb. 28, 2024, 3:34 a.m. UTC | #1

On Tue, 27 Feb 2024 19:17:27 +0100 Petr Machata wrote:
> +	/* bitfield32; operation-specific flags */
> +	NHA_OP_FLAGS,

>  static const struct nla_policy rtm_nh_policy_get[] = {
>  	[NHA_ID]		= { .type = NLA_U32 },
> +	[NHA_OP_FLAGS]		= NLA_POLICY_BITFIELD32(0),

Why bitfiled? You never use the mask.
bitfield gives you the ability to do RMW "atomically" on object fields.
For op flags I don't think it makes much sense.

Petr Machata Feb. 28, 2024, 10:50 a.m. UTC | #2

Jakub Kicinski <[e-mail]> writes:

> On Tue, 27 Feb 2024 19:17:27 +0100 Petr Machata wrote:
>> +	/* bitfield32; operation-specific flags */
>> +	NHA_OP_FLAGS,
>
>>  static const struct nla_policy rtm_nh_policy_get[] = {
>>  	[NHA_ID]		= { .type = NLA_U32 },
>> +	[NHA_OP_FLAGS]		= NLA_POLICY_BITFIELD32(0),
>
> Why bitfiled? You never use the mask.
> bitfield gives you the ability to do RMW "atomically" on object fields.
> For op flags I don't think it makes much sense.

Mostly because we get flag validation for free, whereas it would need to
be hand-rolled for u32. But also I don't know what will be useful in the
future. It would be silly to have to add another flags attribute as
bitfield because this time we actually care about toggling single bits
of an object.

Jakub Kicinski Feb. 28, 2024, 2:48 p.m. UTC | #3

On Wed, 28 Feb 2024 11:50:33 +0100 Petr Machata wrote:
> > Why bitfiled? You never use the mask.
> > bitfield gives you the ability to do RMW "atomically" on object fields.
> > For op flags I don't think it makes much sense.  
> 
> Mostly because we get flag validation for free, whereas it would need to
> be hand-rolled for u32. 

NLA_POLICY_MASK() ?

> But also I don't know what will be useful in the
> future. It would be silly to have to add another flags attribute as
> bitfield because this time we actually care about toggling single bits
> of an object.

IDK how you can do RMW on operation flags, that only makes sense if
you're modifying something. Besides you're not using BITFIELD right,
you're ignoring the mask completely now.

Jakub Kicinski Feb. 28, 2024, 3:16 p.m. UTC | #4

On Wed, 28 Feb 2024 06:48:59 -0800 Jakub Kicinski wrote:
> > But also I don't know what will be useful in the
> > future. It would be silly to have to add another flags attribute as
> > bitfield because this time we actually care about toggling single bits
> > of an object.  
> 
> IDK how you can do RMW on operation flags, that only makes sense if
> you're modifying something. Besides you're not using BITFIELD right,
> you're ignoring the mask completely now.

Let me rephrase this a bit since I've had my coffee now :)
BITFILED is designed to do:

	object->flags = object->flags & ~bf->mask | bf->flags;

since there's no object, there's nothing to & the mask with.
Plus if we do have some object flags at some point, chances 
are we'd want the uAPI flags to mirror the recorded object flags
so that we don't have to translate bit positions, so new attr
will be cleaner.

That's just in the way of clarifying my thinking, your call..

Petr Machata Feb. 28, 2024, 3:58 p.m. UTC | #5

Jakub Kicinski <[e-mail]> writes:

> On Wed, 28 Feb 2024 06:48:59 -0800 Jakub Kicinski wrote:
>> > But also I don't know what will be useful in the
>> > future. It would be silly to have to add another flags attribute as
>> > bitfield because this time we actually care about toggling single bits
>> > of an object.  
>> 
>> IDK how you can do RMW on operation flags, that only makes sense if
>> you're modifying something. Besides you're not using BITFIELD right,
>> you're ignoring the mask completely now.
>
> Let me rephrase this a bit since I've had my coffee now :)
> BITFILED is designed to do:
>
> 	object->flags = object->flags & ~bf->mask | bf->flags;
>
> since there's no object, there's nothing to & the mask with.
> Plus if we do have some object flags at some point, chances 
> are we'd want the uAPI flags to mirror the recorded object flags
> so that we don't have to translate bit positions, so new attr
> will be cleaner.
>
> That's just in the way of clarifying my thinking, your call..

Oh, I see, it wouldn't be useful as an attribute in isolation, and if we
ever introduce flags field for the NH objects, we would want a separate
attribute for it anyway. So whatever new uses the OP_FLAGS attribute
would be put to, we know we won't need the mask.

Um, so as I said, I mostly figured let's use bitfield because of the
validation. I really like how it's all part of the policy and there's no
explicit checking code. So I'd keep it as it is.

Jakub Kicinski Feb. 28, 2024, 4:42 p.m. UTC | #6

On Wed, 28 Feb 2024 16:58:24 +0100 Petr Machata wrote:
> Um, so as I said, I mostly figured let's use bitfield because of the
> validation. I really like how it's all part of the policy and there's no
> explicit checking code. So I'd keep it as it is.

FWIW the same validation is possible for bare unsigned types with
NLA_POLICY_MASK().

Petr Machata Feb. 29, 2024, 2:03 p.m. UTC | #7

Jakub Kicinski <[e-mail]> writes:

> On Wed, 28 Feb 2024 16:58:24 +0100 Petr Machata wrote:
>> Um, so as I said, I mostly figured let's use bitfield because of the
>> validation. I really like how it's all part of the policy and there's no
>> explicit checking code. So I'd keep it as it is.
>
> FWIW the same validation is possible for bare unsigned types with
> NLA_POLICY_MASK().

Awesome, that's what I need.

Patch

diff --git a/include/uapi/linux/nexthop.h b/include/uapi/linux/nexthop.h
index d8ffa8c9ca78..0d7969911fd2 100644
--- a/include/uapi/linux/nexthop.h
+++ b/include/uapi/linux/nexthop.h
@@ -60,6 +60,9 @@  enum {
 	/* nested; nexthop bucket attributes */
 	NHA_RES_BUCKET,
 
+	/* bitfield32; operation-specific flags */
+	NHA_OP_FLAGS,
+
 	__NHA_MAX,
 };
 
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index bcd4df2f1cad..e41e4ac69aee 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -41,6 +41,7 @@  static const struct nla_policy rtm_nh_policy_new[] = {
 
 static const struct nla_policy rtm_nh_policy_get[] = {
 	[NHA_ID]		= { .type = NLA_U32 },
+	[NHA_OP_FLAGS]		= NLA_POLICY_BITFIELD32(0),
 };
 
 static const struct nla_policy rtm_nh_policy_del[] = {
@@ -52,6 +53,7 @@  static const struct nla_policy rtm_nh_policy_dump[] = {
 	[NHA_GROUPS]		= { .type = NLA_FLAG },
 	[NHA_MASTER]		= { .type = NLA_U32 },
 	[NHA_FDB]		= { .type = NLA_FLAG },
+	[NHA_OP_FLAGS]		= NLA_POLICY_BITFIELD32(0),
 };
 
 static const struct nla_policy rtm_nh_res_policy_new[] = {
@@ -2971,7 +2973,7 @@  static int rtm_new_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 }
 
 static int nh_valid_get_del_req(const struct nlmsghdr *nlh,
-				struct nlattr **tb, u32 *id,
+				struct nlattr **tb, u32 *id, u32 *op_flags,
 				struct netlink_ext_ack *extack)
 {
 	struct nhmsg *nhm = nlmsg_data(nlh);
@@ -2992,6 +2994,11 @@  static int nh_valid_get_del_req(const struct nlmsghdr *nlh,
 		return -EINVAL;
 	}
 
+	if (tb[NHA_OP_FLAGS])
+		*op_flags = nla_get_bitfield32(tb[NHA_OP_FLAGS]).value;
+	else
+		*op_flags = 0;
+
 	return 0;
 }
 
@@ -3007,6 +3014,7 @@  static int rtm_del_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 		.portid = NETLINK_CB(skb).portid,
 	};
 	struct nexthop *nh;
+	u32 op_flags;
 	int err;
 	u32 id;
 
@@ -3015,7 +3023,7 @@  static int rtm_del_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
-	err = nh_valid_get_del_req(nlh, tb, &id, extack);
+	err = nh_valid_get_del_req(nlh, tb, &id, &op_flags, extack);
 	if (err)
 		return err;
 
@@ -3036,6 +3044,7 @@  static int rtm_get_nexthop(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 	struct nlattr *tb[NHA_MAX + 1];
 	struct sk_buff *skb = NULL;
 	struct nexthop *nh;
+	u32 op_flags;
 	int err;
 	u32 id;
 
@@ -3044,7 +3053,7 @@  static int rtm_get_nexthop(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
-	err = nh_valid_get_del_req(nlh, tb, &id, extack);
+	err = nh_valid_get_del_req(nlh, tb, &id, &op_flags, extack);
 	if (err)
 		return err;
 
@@ -3080,6 +3089,7 @@  struct nh_dump_filter {
 	bool group_filter;
 	bool fdb_filter;
 	u32 res_bucket_nh_id;
+	u32 op_flags;
 };
 
 static bool nh_dump_filtered(struct nexthop *nh,
@@ -3151,6 +3161,11 @@  static int __nh_valid_dump_req(const struct nlmsghdr *nlh, struct nlattr **tb,
 		return -EINVAL;
 	}
 
+	if (tb[NHA_OP_FLAGS])
+		filter->op_flags = nla_get_bitfield32(tb[NHA_OP_FLAGS]).value;
+	else
+		filter->op_flags = 0;
+
 	return 0;
 }
 
@@ -3474,6 +3489,7 @@  static int nh_valid_get_bucket_req(const struct nlmsghdr *nlh,
 				   struct netlink_ext_ack *extack)
 {
 	struct nlattr *tb[NHA_MAX + 1];
+	u32 op_flags;
 	int err;
 
 	err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb, NHA_MAX,
@@ -3481,7 +3497,7 @@  static int nh_valid_get_bucket_req(const struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
-	err = nh_valid_get_del_req(nlh, tb, id, extack);
+	err = nh_valid_get_del_req(nlh, tb, id, &op_flags, extack);
 	if (err)
 		return err;