[PATCH net-next 0/5] devlink: sanitize variable typed attributes

[PATCH net-next 0/5] devlink: sanitize variable typed attributes

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

This is continuation based on first two patches
of https://lore.kernel.org/netdev/20250425214808.507732-1-saeed@kernel.org/

Better to take it as a separate patchset, as the rest of the original
patchset is probably settled.

This patchset is taking care of incorrect usage of internal NLA_* values
in uapi, introduces new enum (in patch #3) that shadows NLA_* values and
makes that part of UAPI.

The last two patches removes unnecessary translations with maintaining
clear devlink param driver api. I hope this might be acceptable.

Please check and merge to get this over with :)

Jiri Pirko (5):
  tools: ynl-gen: extend block_start/end by noind arg
  tools: ynl-gen: allow noncontiguous enums
  devlink: define enum for attr types of dynamic attributes
  devlink: avoid param type value translations
  devlink: use DEVLINK_VAR_ATTR_TYPE_* instead of NLA_* in fmsg

 Documentation/netlink/specs/devlink.yaml | 24 +++++++++++
 include/net/devlink.h                    | 10 ++---
 include/uapi/linux/devlink.h             | 17 ++++++++
 net/devlink/health.c                     | 52 +++++++++-------------
 net/devlink/netlink_gen.c                | 21 ++++++++-
 net/devlink/param.c                      | 46 +-------------------
 tools/net/ynl/pyynl/ynl_gen_c.py         | 55 +++++++++++++++++++++---
 7 files changed, 137 insertions(+), 88 deletions(-)

-- 
2.49.0

[PATCH net-next 1/5] tools: ynl-gen: extend block_start/end by noind arg

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

In order to allow block with no indentation, like switch-case, extend
block_start/end by "noind" arg allowing caller to instruct writer to
do no indentation.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 9613a6135003..b4889974f645 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -1345,16 +1345,18 @@ class CodeWriter:
     def nl(self):
         self._nl = True
 
-    def block_start(self, line=''):
+    def block_start(self, line='', noind=False):
         if line:
             line = line + ' '
         self.p(line + '{')
-        self._ind += 1
+        if not noind:
+            self._ind += 1
 
-    def block_end(self, line=''):
+    def block_end(self, line='', noind=False):
         if line and line[0] not in {';', ','}:
             line = ' ' + line
-        self._ind -= 1
+        if not noind:
+            self._ind -= 1
         self._nl = False
         if not line:
             # Delay printing closing bracket in case "else" comes next
-- 
2.49.0

[PATCH net-next 2/5] tools: ynl-gen: allow noncontiguous enums

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

In case the enum has holes, instead of hard stop, generate a validation
callback to check valid enum values.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
Saeed's v3->v1:
- add validation callback generation
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 45 +++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index b4889974f645..c37551473923 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -358,11 +358,13 @@ class TypeScalar(Type):
         if 'enum' in self.attr:
             enum = self.family.consts[self.attr['enum']]
             low, high = enum.value_range()
-            if 'min' not in self.checks:
+            if low and 'min' not in self.checks:
                 if low != 0 or self.type[0] == 's':
                     self.checks['min'] = low
-            if 'max' not in self.checks:
+            if high and 'max' not in self.checks:
                 self.checks['max'] = high
+            if not low and not high:
+                self.checks['sparse'] = True
 
         if 'min' in self.checks and 'max' in self.checks:
             if self.get_limit('min') > self.get_limit('max'):
@@ -417,6 +419,8 @@ class TypeScalar(Type):
             return f"NLA_POLICY_MIN({policy}, {self.get_limit_str('min')})"
         elif 'max' in self.checks:
             return f"NLA_POLICY_MAX({policy}, {self.get_limit_str('max')})"
+        elif 'sparse' in self.checks:
+            return f"NLA_POLICY_VALIDATE_FN({policy}, &{c_lower(self.enum_name)}_validate)"
         return super()._attr_policy(policy)
 
     def _attr_typol(self):
@@ -862,7 +866,7 @@ class EnumSet(SpecEnumSet):
         high = max([x.value for x in self.entries.values()])
 
         if high - low + 1 != len(self.entries):
-            raise Exception("Can't get value range for a noncontiguous enum")
+            return None, None
 
         return low, high
 
@@ -2299,6 +2303,40 @@ def print_kernel_policy_ranges(family, cw):
             cw.nl()
 
 
+def print_kernel_policy_sparse_enum_validates(family, cw):
+    first = True
+    for _, attr_set in family.attr_sets.items():
+        if attr_set.subset_of:
+            continue
+
+        for _, attr in attr_set.items():
+            if not attr.request:
+                continue
+            if not attr.enum_name:
+                continue
+            if 'sparse' not in attr.checks:
+                continue
+
+            if first:
+                cw.p('/* Sparse enums validation callbacks */')
+                first = False
+
+            sign = '' if attr.type[0] == 'u' else '_signed'
+            suffix = 'ULL' if attr.type[0] == 'u' else 'LL'
+            cw.write_func_prot('static int', f'{c_lower(attr.enum_name)}_validate',
+                               ['const struct nlattr *attr', 'struct netlink_ext_ack *extack'])
+            cw.block_start()
+            cw.block_start(line=f'switch (nla_get_{attr["type"]}(attr))', noind=True)
+            enum = family.consts[attr['enum']]
+            for entry in enum.entries.values():
+                cw.p(f'case {entry.c_name}: return 0;')
+            cw.block_end(noind=True)
+            cw.p('NL_SET_ERR_MSG_ATTR(extack, attr, "invalid enum value");')
+            cw.p('return -EINVAL;')
+            cw.block_end()
+            cw.nl()
+
+
 def print_kernel_op_table_fwd(family, cw, terminate):
     exported = not kernel_can_gen_family_struct(family)
 
@@ -2965,6 +3003,7 @@ def main():
             print_kernel_family_struct_hdr(parsed, cw)
         else:
             print_kernel_policy_ranges(parsed, cw)
+            print_kernel_policy_sparse_enum_validates(parsed, cw)
 
             for _, struct in sorted(parsed.pure_nested_structs.items()):
                 if struct.request:
-- 
2.49.0

[PATCH net-next 3/5] devlink: define enum for attr types of dynamic attributes

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Devlink param and health reporter fmsg use attributes with dynamic type
which is determined according to a different type. Currently used values
are NLA_*. The problem is, they are not part of UAPI. They may change
which would cause a break.

To make this future safe, introduce a enum that shadows NLA_* values in
it and is part of UAPI.

Also, this allows to possibly carry types that are unrelated to NLA_*
values.

Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
Saeed's v3->v1:
- s/Dynamic/Variable/ in comment
- re-generated netlink_gen.c according to changes in previous patch
---
 Documentation/netlink/specs/devlink.yaml | 24 +++++++++++++++++++
 include/uapi/linux/devlink.h             | 17 ++++++++++++++
 net/devlink/health.c                     | 17 ++++++++++++--
 net/devlink/netlink_gen.c                | 21 ++++++++++++++++-
 net/devlink/param.c                      | 30 ++++++++++++------------
 5 files changed, 91 insertions(+), 18 deletions(-)

diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
index bd9726269b4f..05fee1b7fe19 100644
--- a/Documentation/netlink/specs/devlink.yaml
+++ b/Documentation/netlink/specs/devlink.yaml
@@ -202,6 +202,28 @@ definitions:
         name: exception
       -
         name: control
+  -
+    type: enum
+    name: var-attr-type
+    entries:
+      -
+        name: u8
+        value: 1
+      -
+        name: u16
+      -
+        name: u32
+      -
+        name: u64
+      -
+        name: string
+      -
+        name: flag
+      -
+        name: nul_string
+        value: 10
+      -
+        name: binary
 
 attribute-sets:
   -
@@ -498,6 +520,7 @@ attribute-sets:
       -
         name: param-type
         type: u8
+        enum: var-attr-type
 
       # TODO: fill in the attributes in between
 
@@ -592,6 +615,7 @@ attribute-sets:
       -
         name: fmsg-obj-value-type
         type: u8
+        enum: var-attr-type
 
       # TODO: fill in the attributes in between
 
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 9401aa343673..39a1216d15fe 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -385,6 +385,23 @@ enum devlink_linecard_state {
 	DEVLINK_LINECARD_STATE_MAX = __DEVLINK_LINECARD_STATE_MAX - 1
 };
 
+/**
+ * enum devlink_var_attr_type - Variable attribute type.
+ */
+enum devlink_var_attr_type {
+	/* Following values relate to the internal NLA_* values */
+	DEVLINK_VAR_ATTR_TYPE_U8 = 1,
+	DEVLINK_VAR_ATTR_TYPE_U16,
+	DEVLINK_VAR_ATTR_TYPE_U32,
+	DEVLINK_VAR_ATTR_TYPE_U64,
+	DEVLINK_VAR_ATTR_TYPE_STRING,
+	DEVLINK_VAR_ATTR_TYPE_FLAG,
+	DEVLINK_VAR_ATTR_TYPE_NUL_STRING = 10,
+	DEVLINK_VAR_ATTR_TYPE_BINARY,
+	__DEVLINK_VAR_ATTR_TYPE_CUSTOM_BASE = 0x80,
+	/* Any possible custom types, unrelated to NLA_* values go below */
+};
+
 enum devlink_attr {
 	/* don't change the order or add anything between, this is ABI! */
 	DEVLINK_ATTR_UNSPEC,
diff --git a/net/devlink/health.c b/net/devlink/health.c
index 57db6799722a..95a7a62d85a2 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -930,18 +930,31 @@ EXPORT_SYMBOL_GPL(devlink_fmsg_binary_pair_put);
 static int
 devlink_fmsg_item_fill_type(struct devlink_fmsg_item *msg, struct sk_buff *skb)
 {
+	enum devlink_var_attr_type var_attr_type;
+
 	switch (msg->nla_type) {
 	case NLA_FLAG:
+		var_attr_type = DEVLINK_VAR_ATTR_TYPE_FLAG;
+		break;
 	case NLA_U8:
+		var_attr_type = DEVLINK_VAR_ATTR_TYPE_U8;
+		break;
 	case NLA_U32:
+		var_attr_type = DEVLINK_VAR_ATTR_TYPE_U32;
+		break;
 	case NLA_U64:
+		var_attr_type = DEVLINK_VAR_ATTR_TYPE_U64;
+		break;
 	case NLA_NUL_STRING:
+		var_attr_type = DEVLINK_VAR_ATTR_TYPE_NUL_STRING;
+		break;
 	case NLA_BINARY:
-		return nla_put_u8(skb, DEVLINK_ATTR_FMSG_OBJ_VALUE_TYPE,
-				  msg->nla_type);
+		var_attr_type = DEVLINK_VAR_ATTR_TYPE_BINARY;
+		break;
 	default:
 		return -EINVAL;
 	}
+	return nla_put_u8(skb, DEVLINK_ATTR_FMSG_OBJ_VALUE_TYPE, var_attr_type);
 }
 
 static int
diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
index f9786d51f68f..4b4122140ce4 100644
--- a/net/devlink/netlink_gen.c
+++ b/net/devlink/netlink_gen.c
@@ -10,6 +10,25 @@
 
 #include <uapi/linux/devlink.h>
 
+/* Sparse enums validation callbacks */
+static int
+devlink_attr_param_type_validate(const struct nlattr *attr,
+				 struct netlink_ext_ack *extack)
+{
+	switch (nla_get_u8(attr)) {
+	case DEVLINK_VAR_ATTR_TYPE_U8: return 0;
+	case DEVLINK_VAR_ATTR_TYPE_U16: return 0;
+	case DEVLINK_VAR_ATTR_TYPE_U32: return 0;
+	case DEVLINK_VAR_ATTR_TYPE_U64: return 0;
+	case DEVLINK_VAR_ATTR_TYPE_STRING: return 0;
+	case DEVLINK_VAR_ATTR_TYPE_FLAG: return 0;
+	case DEVLINK_VAR_ATTR_TYPE_NUL_STRING: return 0;
+	case DEVLINK_VAR_ATTR_TYPE_BINARY: return 0;
+	}
+	NL_SET_ERR_MSG_ATTR(extack, attr, "invalid enum value");
+	return -EINVAL;
+}
+
 /* Common nested types */
 const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_FN_ATTR_CAPS + 1] = {
 	[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR] = { .type = NLA_BINARY, },
@@ -273,7 +292,7 @@ static const struct nla_policy devlink_param_set_nl_policy[DEVLINK_ATTR_PARAM_VA
 	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
 	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
 	[DEVLINK_ATTR_PARAM_NAME] = { .type = NLA_NUL_STRING, },
-	[DEVLINK_ATTR_PARAM_TYPE] = { .type = NLA_U8, },
+	[DEVLINK_ATTR_PARAM_TYPE] = NLA_POLICY_VALIDATE_FN(NLA_U8, &devlink_attr_param_type_validate),
 	[DEVLINK_ATTR_PARAM_VALUE_CMODE] = NLA_POLICY_MAX(NLA_U8, 2),
 };
 
diff --git a/net/devlink/param.c b/net/devlink/param.c
index dcf0d1ccebba..d0fb7c88bdb8 100644
--- a/net/devlink/param.c
+++ b/net/devlink/param.c
@@ -167,19 +167,19 @@ static int devlink_param_set(struct devlink *devlink,
 }
 
 static int
-devlink_param_type_to_nla_type(enum devlink_param_type param_type)
+devlink_param_type_to_var_attr_type(enum devlink_param_type param_type)
 {
 	switch (param_type) {
 	case DEVLINK_PARAM_TYPE_U8:
-		return NLA_U8;
+		return DEVLINK_VAR_ATTR_TYPE_U8;
 	case DEVLINK_PARAM_TYPE_U16:
-		return NLA_U16;
+		return DEVLINK_VAR_ATTR_TYPE_U16;
 	case DEVLINK_PARAM_TYPE_U32:
-		return NLA_U32;
+		return DEVLINK_VAR_ATTR_TYPE_U32;
 	case DEVLINK_PARAM_TYPE_STRING:
-		return NLA_STRING;
+		return DEVLINK_VAR_ATTR_TYPE_STRING;
 	case DEVLINK_PARAM_TYPE_BOOL:
-		return NLA_FLAG;
+		return DEVLINK_VAR_ATTR_TYPE_FLAG;
 	default:
 		return -EINVAL;
 	}
@@ -247,7 +247,7 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
 	struct devlink_param_gset_ctx ctx;
 	struct nlattr *param_values_list;
 	struct nlattr *param_attr;
-	int nla_type;
+	int var_attr_type;
 	void *hdr;
 	int err;
 	int i;
@@ -294,10 +294,10 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
 	if (param->generic && nla_put_flag(msg, DEVLINK_ATTR_PARAM_GENERIC))
 		goto param_nest_cancel;
 
-	nla_type = devlink_param_type_to_nla_type(param->type);
-	if (nla_type < 0)
+	var_attr_type = devlink_param_type_to_var_attr_type(param->type);
+	if (var_attr_type < 0)
 		goto param_nest_cancel;
-	if (nla_put_u8(msg, DEVLINK_ATTR_PARAM_TYPE, nla_type))
+	if (nla_put_u8(msg, DEVLINK_ATTR_PARAM_TYPE, var_attr_type))
 		goto param_nest_cancel;
 
 	param_values_list = nla_nest_start_noflag(msg,
@@ -420,19 +420,19 @@ devlink_param_type_get_from_info(struct genl_info *info,
 		return -EINVAL;
 
 	switch (nla_get_u8(info->attrs[DEVLINK_ATTR_PARAM_TYPE])) {
-	case NLA_U8:
+	case DEVLINK_VAR_ATTR_TYPE_U8:
 		*param_type = DEVLINK_PARAM_TYPE_U8;
 		break;
-	case NLA_U16:
+	case DEVLINK_VAR_ATTR_TYPE_U16:
 		*param_type = DEVLINK_PARAM_TYPE_U16;
 		break;
-	case NLA_U32:
+	case DEVLINK_VAR_ATTR_TYPE_U32:
 		*param_type = DEVLINK_PARAM_TYPE_U32;
 		break;
-	case NLA_STRING:
+	case DEVLINK_VAR_ATTR_TYPE_STRING:
 		*param_type = DEVLINK_PARAM_TYPE_STRING;
 		break;
-	case NLA_FLAG:
+	case DEVLINK_VAR_ATTR_TYPE_FLAG:
 		*param_type = DEVLINK_PARAM_TYPE_BOOL;
 		break;
 	default:
-- 
2.49.0

[PATCH net-next 4/5] devlink: avoid param type value translations

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Assign DEVLINK_PARAM_TYPE_* enum values to DEVLINK_VAR_ATTR_TYPE_* to
ensure the same values are used internally and in UAPI. Benefit from
that by removing the value translations.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/net/devlink.h | 10 +++++-----
 net/devlink/param.c   | 46 ++-----------------------------------------
 2 files changed, 7 insertions(+), 49 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index b8783126c1ed..0091f23a40f7 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -420,11 +420,11 @@ typedef u64 devlink_resource_occ_get_t(void *priv);
 
 #define __DEVLINK_PARAM_MAX_STRING_VALUE 32
 enum devlink_param_type {
-	DEVLINK_PARAM_TYPE_U8,
-	DEVLINK_PARAM_TYPE_U16,
-	DEVLINK_PARAM_TYPE_U32,
-	DEVLINK_PARAM_TYPE_STRING,
-	DEVLINK_PARAM_TYPE_BOOL,
+	DEVLINK_PARAM_TYPE_U8 = DEVLINK_VAR_ATTR_TYPE_U8,
+	DEVLINK_PARAM_TYPE_U16 = DEVLINK_VAR_ATTR_TYPE_U16,
+	DEVLINK_PARAM_TYPE_U32 = DEVLINK_VAR_ATTR_TYPE_U32,
+	DEVLINK_PARAM_TYPE_STRING = DEVLINK_VAR_ATTR_TYPE_STRING,
+	DEVLINK_PARAM_TYPE_BOOL = DEVLINK_VAR_ATTR_TYPE_FLAG,
 };
 
 union devlink_param_value {
diff --git a/net/devlink/param.c b/net/devlink/param.c
index d0fb7c88bdb8..b29abf8d3ed4 100644
--- a/net/devlink/param.c
+++ b/net/devlink/param.c
@@ -166,25 +166,6 @@ static int devlink_param_set(struct devlink *devlink,
 	return param->set(devlink, param->id, ctx, extack);
 }
 
-static int
-devlink_param_type_to_var_attr_type(enum devlink_param_type param_type)
-{
-	switch (param_type) {
-	case DEVLINK_PARAM_TYPE_U8:
-		return DEVLINK_VAR_ATTR_TYPE_U8;
-	case DEVLINK_PARAM_TYPE_U16:
-		return DEVLINK_VAR_ATTR_TYPE_U16;
-	case DEVLINK_PARAM_TYPE_U32:
-		return DEVLINK_VAR_ATTR_TYPE_U32;
-	case DEVLINK_PARAM_TYPE_STRING:
-		return DEVLINK_VAR_ATTR_TYPE_STRING;
-	case DEVLINK_PARAM_TYPE_BOOL:
-		return DEVLINK_VAR_ATTR_TYPE_FLAG;
-	default:
-		return -EINVAL;
-	}
-}
-
 static int
 devlink_nl_param_value_fill_one(struct sk_buff *msg,
 				enum devlink_param_type type,
@@ -247,7 +228,6 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
 	struct devlink_param_gset_ctx ctx;
 	struct nlattr *param_values_list;
 	struct nlattr *param_attr;
-	int var_attr_type;
 	void *hdr;
 	int err;
 	int i;
@@ -293,11 +273,7 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
 		goto param_nest_cancel;
 	if (param->generic && nla_put_flag(msg, DEVLINK_ATTR_PARAM_GENERIC))
 		goto param_nest_cancel;
-
-	var_attr_type = devlink_param_type_to_var_attr_type(param->type);
-	if (var_attr_type < 0)
-		goto param_nest_cancel;
-	if (nla_put_u8(msg, DEVLINK_ATTR_PARAM_TYPE, var_attr_type))
+	if (nla_put_u8(msg, DEVLINK_ATTR_PARAM_TYPE, param->type))
 		goto param_nest_cancel;
 
 	param_values_list = nla_nest_start_noflag(msg,
@@ -419,25 +395,7 @@ devlink_param_type_get_from_info(struct genl_info *info,
 	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_PARAM_TYPE))
 		return -EINVAL;
 
-	switch (nla_get_u8(info->attrs[DEVLINK_ATTR_PARAM_TYPE])) {
-	case DEVLINK_VAR_ATTR_TYPE_U8:
-		*param_type = DEVLINK_PARAM_TYPE_U8;
-		break;
-	case DEVLINK_VAR_ATTR_TYPE_U16:
-		*param_type = DEVLINK_PARAM_TYPE_U16;
-		break;
-	case DEVLINK_VAR_ATTR_TYPE_U32:
-		*param_type = DEVLINK_PARAM_TYPE_U32;
-		break;
-	case DEVLINK_VAR_ATTR_TYPE_STRING:
-		*param_type = DEVLINK_PARAM_TYPE_STRING;
-		break;
-	case DEVLINK_VAR_ATTR_TYPE_FLAG:
-		*param_type = DEVLINK_PARAM_TYPE_BOOL;
-		break;
-	default:
-		return -EINVAL;
-	}
+	*param_type = nla_get_u8(info->attrs[DEVLINK_ATTR_PARAM_TYPE]);
 
 	return 0;
 }
-- 
2.49.0

[PATCH net-next 5/5] devlink: use DEVLINK_VAR_ATTR_TYPE_* instead of NLA_* in fmsg

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Use newly introduced DEVLINK_VAR_ATTR_TYPE_* enum values instead of
internal NLA_* in fmsg health reporter code.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/health.c | 65 ++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 44 deletions(-)

diff --git a/net/devlink/health.c b/net/devlink/health.c
index 95a7a62d85a2..b3ce8ecbb7fb 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -735,7 +735,7 @@ static void devlink_fmsg_put_name(struct devlink_fmsg *fmsg, const char *name)
 		return;
 	}
 
-	item->nla_type = NLA_NUL_STRING;
+	item->nla_type = DEVLINK_VAR_ATTR_TYPE_NUL_STRING;
 	item->len = strlen(name) + 1;
 	item->attrtype = DEVLINK_ATTR_FMSG_OBJ_NAME;
 	memcpy(&item->value, name, item->len);
@@ -822,32 +822,37 @@ static void devlink_fmsg_put_value(struct devlink_fmsg *fmsg,
 static void devlink_fmsg_bool_put(struct devlink_fmsg *fmsg, bool value)
 {
 	devlink_fmsg_err_if_binary(fmsg);
-	devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_FLAG);
+	devlink_fmsg_put_value(fmsg, &value, sizeof(value),
+			       DEVLINK_VAR_ATTR_TYPE_FLAG);
 }
 
 static void devlink_fmsg_u8_put(struct devlink_fmsg *fmsg, u8 value)
 {
 	devlink_fmsg_err_if_binary(fmsg);
-	devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_U8);
+	devlink_fmsg_put_value(fmsg, &value, sizeof(value),
+			       DEVLINK_VAR_ATTR_TYPE_U8);
 }
 
 void devlink_fmsg_u32_put(struct devlink_fmsg *fmsg, u32 value)
 {
 	devlink_fmsg_err_if_binary(fmsg);
-	devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_U32);
+	devlink_fmsg_put_value(fmsg, &value, sizeof(value),
+			       DEVLINK_VAR_ATTR_TYPE_U32);
 }
 EXPORT_SYMBOL_GPL(devlink_fmsg_u32_put);
 
 static void devlink_fmsg_u64_put(struct devlink_fmsg *fmsg, u64 value)
 {
 	devlink_fmsg_err_if_binary(fmsg);
-	devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_U64);
+	devlink_fmsg_put_value(fmsg, &value, sizeof(value),
+			       DEVLINK_VAR_ATTR_TYPE_U64);
 }
 
 void devlink_fmsg_string_put(struct devlink_fmsg *fmsg, const char *value)
 {
 	devlink_fmsg_err_if_binary(fmsg);
-	devlink_fmsg_put_value(fmsg, value, strlen(value) + 1, NLA_NUL_STRING);
+	devlink_fmsg_put_value(fmsg, value, strlen(value) + 1,
+			       DEVLINK_VAR_ATTR_TYPE_NUL_STRING);
 }
 EXPORT_SYMBOL_GPL(devlink_fmsg_string_put);
 
@@ -857,7 +862,8 @@ void devlink_fmsg_binary_put(struct devlink_fmsg *fmsg, const void *value,
 	if (!fmsg->err && !fmsg->putting_binary)
 		fmsg->err = -EINVAL;
 
-	devlink_fmsg_put_value(fmsg, value, value_len, NLA_BINARY);
+	devlink_fmsg_put_value(fmsg, value, value_len,
+			       DEVLINK_VAR_ATTR_TYPE_BINARY);
 }
 EXPORT_SYMBOL_GPL(devlink_fmsg_binary_put);
 
@@ -927,36 +933,6 @@ void devlink_fmsg_binary_pair_put(struct devlink_fmsg *fmsg, const char *name,
 }
 EXPORT_SYMBOL_GPL(devlink_fmsg_binary_pair_put);
 
-static int
-devlink_fmsg_item_fill_type(struct devlink_fmsg_item *msg, struct sk_buff *skb)
-{
-	enum devlink_var_attr_type var_attr_type;
-
-	switch (msg->nla_type) {
-	case NLA_FLAG:
-		var_attr_type = DEVLINK_VAR_ATTR_TYPE_FLAG;
-		break;
-	case NLA_U8:
-		var_attr_type = DEVLINK_VAR_ATTR_TYPE_U8;
-		break;
-	case NLA_U32:
-		var_attr_type = DEVLINK_VAR_ATTR_TYPE_U32;
-		break;
-	case NLA_U64:
-		var_attr_type = DEVLINK_VAR_ATTR_TYPE_U64;
-		break;
-	case NLA_NUL_STRING:
-		var_attr_type = DEVLINK_VAR_ATTR_TYPE_NUL_STRING;
-		break;
-	case NLA_BINARY:
-		var_attr_type = DEVLINK_VAR_ATTR_TYPE_BINARY;
-		break;
-	default:
-		return -EINVAL;
-	}
-	return nla_put_u8(skb, DEVLINK_ATTR_FMSG_OBJ_VALUE_TYPE, var_attr_type);
-}
-
 static int
 devlink_fmsg_item_fill_data(struct devlink_fmsg_item *msg, struct sk_buff *skb)
 {
@@ -964,20 +940,20 @@ devlink_fmsg_item_fill_data(struct devlink_fmsg_item *msg, struct sk_buff *skb)
 	u8 tmp;
 
 	switch (msg->nla_type) {
-	case NLA_FLAG:
+	case DEVLINK_VAR_ATTR_TYPE_FLAG:
 		/* Always provide flag data, regardless of its value */
 		tmp = *(bool *)msg->value;
 
 		return nla_put_u8(skb, attrtype, tmp);
-	case NLA_U8:
+	case DEVLINK_VAR_ATTR_TYPE_U8:
 		return nla_put_u8(skb, attrtype, *(u8 *)msg->value);
-	case NLA_U32:
+	case DEVLINK_VAR_ATTR_TYPE_U32:
 		return nla_put_u32(skb, attrtype, *(u32 *)msg->value);
-	case NLA_U64:
+	case DEVLINK_VAR_ATTR_TYPE_U64:
 		return devlink_nl_put_u64(skb, attrtype, *(u64 *)msg->value);
-	case NLA_NUL_STRING:
+	case DEVLINK_VAR_ATTR_TYPE_NUL_STRING:
 		return nla_put_string(skb, attrtype, (char *)&msg->value);
-	case NLA_BINARY:
+	case DEVLINK_VAR_ATTR_TYPE_BINARY:
 		return nla_put(skb, attrtype, msg->len, (void *)&msg->value);
 	default:
 		return -EINVAL;
@@ -1011,7 +987,8 @@ devlink_fmsg_prepare_skb(struct devlink_fmsg *fmsg, struct sk_buff *skb,
 			err = nla_put_flag(skb, item->attrtype);
 			break;
 		case DEVLINK_ATTR_FMSG_OBJ_VALUE_DATA:
-			err = devlink_fmsg_item_fill_type(item, skb);
+			err = nla_put_u8(skb, DEVLINK_ATTR_FMSG_OBJ_VALUE_TYPE,
+					 item->nla_type);
 			if (err)
 				break;
 			err = devlink_fmsg_item_fill_data(item, skb);
-- 
2.49.0

Re: [PATCH net-next 1/5] tools: ynl-gen: extend block_start/end by noind arg

[Jakub Kicinski]

On Fri,  2 May 2025 13:38:17 +0200 Jiri Pirko wrote:
> -    def block_start(self, line=''):
> +    def block_start(self, line='', noind=False):
>          if line:
>              line = line + ' '
>          self.p(line + '{')
> -        self._ind += 1
> +        if not noind:
> +            self._ind += 1
>  
> -    def block_end(self, line=''):
> +    def block_end(self, line='', noind=False):
>          if line and line[0] not in {';', ','}:
>              line = ' ' + line
> -        self._ind -= 1
> +        if not noind:
> +            self._ind -= 1

Should not be necessary, CodeWriter already automatically "unindents"
lines which end with : as all switch cases should.

Re: [PATCH net-next 2/5] tools: ynl-gen: allow noncontiguous enums

[Jakub Kicinski]

On Fri,  2 May 2025 13:38:18 +0200 Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> In case the enum has holes, instead of hard stop, generate a validation
> callback to check valid enum values.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> Saeed's v3->v1:
> - add validation callback generation
> ---
>  tools/net/ynl/pyynl/ynl_gen_c.py | 45 +++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> index b4889974f645..c37551473923 100755
> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
> @@ -358,11 +358,13 @@ class TypeScalar(Type):
>          if 'enum' in self.attr:
>              enum = self.family.consts[self.attr['enum']]
>              low, high = enum.value_range()
> -            if 'min' not in self.checks:
> +            if low and 'min' not in self.checks:
>                  if low != 0 or self.type[0] == 's':
>                      self.checks['min'] = low
> -            if 'max' not in self.checks:
> +            if high and 'max' not in self.checks:
>                  self.checks['max'] = high
> +            if not low and not high:
> +                self.checks['sparse'] = True

you should probably explicitly check for None, 0 is a valid low / high
  
>          if 'min' in self.checks and 'max' in self.checks:
>              if self.get_limit('min') > self.get_limit('max'):

> +def print_kernel_policy_sparse_enum_validates(family, cw):
> +    first = True
> +    for _, attr_set in family.attr_sets.items():
> +        if attr_set.subset_of:
> +            continue
> +
> +        for _, attr in attr_set.items():
> +            if not attr.request:
> +                continue
> +            if not attr.enum_name:
> +                continue
> +            if 'sparse' not in attr.checks:
> +                continue
> +
> +            if first:
> +                cw.p('/* Sparse enums validation callbacks */')
> +                first = False
> +
> +            sign = '' if attr.type[0] == 'u' else '_signed'
> +            suffix = 'ULL' if attr.type[0] == 'u' else 'LL'
> +            cw.write_func_prot('static int', f'{c_lower(attr.enum_name)}_validate',
> +                               ['const struct nlattr *attr', 'struct netlink_ext_ack *extack'])
> +            cw.block_start()
> +            cw.block_start(line=f'switch (nla_get_{attr["type"]}(attr))', noind=True)
> +            enum = family.consts[attr['enum']]
> +            for entry in enum.entries.values():
> +                cw.p(f'case {entry.c_name}: return 0;')

All the cases end in "return 0;"
remove this, and add the return 0; before the block end.
The code should look something like:

	switch (nla_get_...) {
	case VAL1:
	case VAL2:
	case VAL3:
		return 0;
	}

> +            cw.block_end(noind=True)
> +            cw.p('NL_SET_ERR_MSG_ATTR(extack, attr, "invalid enum value");')
> +            cw.p('return -EINVAL;')
> +            cw.block_end()
> +            cw.nl()
> +
> +
>  def print_kernel_op_table_fwd(family, cw, terminate):
>      exported = not kernel_can_gen_family_struct(family)
>  
> @@ -2965,6 +3003,7 @@ def main():
>              print_kernel_family_struct_hdr(parsed, cw)
>          else:
>              print_kernel_policy_ranges(parsed, cw)
> +            print_kernel_policy_sparse_enum_validates(parsed, cw)
>  
>              for _, struct in sorted(parsed.pure_nested_structs.items()):
>                  if struct.request:

Re: [PATCH net-next 3/5] devlink: define enum for attr types of dynamic attributes

[Jakub Kicinski]

On Fri,  2 May 2025 13:38:19 +0200 Jiri Pirko wrote:
> +/**
> + * enum devlink_var_attr_type - Variable attribute type.
> + */

If we want this to be a kdoc it needs to document all values.
Same as a struct kdoc needs to document all values.
Not sure if there's much to say here for each value, so a non-kdoc
comment is probably better?

I think I already mentioned this generates a build warning..

> +enum devlink_var_attr_type {
> +	/* Following values relate to the internal NLA_* values */
> +	DEVLINK_VAR_ATTR_TYPE_U8 = 1,
> +	DEVLINK_VAR_ATTR_TYPE_U16,
> +	DEVLINK_VAR_ATTR_TYPE_U32,
> +	DEVLINK_VAR_ATTR_TYPE_U64,
> +	DEVLINK_VAR_ATTR_TYPE_STRING,
> +	DEVLINK_VAR_ATTR_TYPE_FLAG,
> +	DEVLINK_VAR_ATTR_TYPE_NUL_STRING = 10,
> +	DEVLINK_VAR_ATTR_TYPE_BINARY,
> +	__DEVLINK_VAR_ATTR_TYPE_CUSTOM_BASE = 0x80,
> +	/* Any possible custom types, unrelated to NLA_* values go below */
> +};

Re: [PATCH net-next 3/5] devlink: define enum for attr types of dynamic attributes

[Jiri Pirko]

Sat, May 03, 2025 at 03:46:04AM +0200, kuba@kernel.org wrote:
>On Fri,  2 May 2025 13:38:19 +0200 Jiri Pirko wrote:
>> +/**
>> + * enum devlink_var_attr_type - Variable attribute type.
>> + */
>
>If we want this to be a kdoc it needs to document all values.
>Same as a struct kdoc needs to document all values.
>Not sure if there's much to say here for each value, so a non-kdoc
>comment is probably better?

Okay.

>
>I think I already mentioned this generates a build warning..

Did not see. Any. Will check.

>
>> +enum devlink_var_attr_type {
>> +	/* Following values relate to the internal NLA_* values */
>> +	DEVLINK_VAR_ATTR_TYPE_U8 = 1,
>> +	DEVLINK_VAR_ATTR_TYPE_U16,
>> +	DEVLINK_VAR_ATTR_TYPE_U32,
>> +	DEVLINK_VAR_ATTR_TYPE_U64,
>> +	DEVLINK_VAR_ATTR_TYPE_STRING,
>> +	DEVLINK_VAR_ATTR_TYPE_FLAG,
>> +	DEVLINK_VAR_ATTR_TYPE_NUL_STRING = 10,
>> +	DEVLINK_VAR_ATTR_TYPE_BINARY,
>> +	__DEVLINK_VAR_ATTR_TYPE_CUSTOM_BASE = 0x80,
>> +	/* Any possible custom types, unrelated to NLA_* values go below */
>> +};

Re: [PATCH net-next 2/5] tools: ynl-gen: allow noncontiguous enums

[Jiri Pirko]

Sat, May 03, 2025 at 03:43:47AM +0200, kuba@kernel.org wrote:
>On Fri,  2 May 2025 13:38:18 +0200 Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> In case the enum has holes, instead of hard stop, generate a validation
>> callback to check valid enum values.
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> Saeed's v3->v1:
>> - add validation callback generation
>> ---
>>  tools/net/ynl/pyynl/ynl_gen_c.py | 45 +++++++++++++++++++++++++++++---
>>  1 file changed, 42 insertions(+), 3 deletions(-)
>> 
>> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
>> index b4889974f645..c37551473923 100755
>> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
>> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
>> @@ -358,11 +358,13 @@ class TypeScalar(Type):
>>          if 'enum' in self.attr:
>>              enum = self.family.consts[self.attr['enum']]
>>              low, high = enum.value_range()
>> -            if 'min' not in self.checks:
>> +            if low and 'min' not in self.checks:
>>                  if low != 0 or self.type[0] == 's':
>>                      self.checks['min'] = low
>> -            if 'max' not in self.checks:
>> +            if high and 'max' not in self.checks:
>>                  self.checks['max'] = high
>> +            if not low and not high:
>> +                self.checks['sparse'] = True
>
>you should probably explicitly check for None, 0 is a valid low / high

Okay.


>  
>>          if 'min' in self.checks and 'max' in self.checks:
>>              if self.get_limit('min') > self.get_limit('max'):
>
>> +def print_kernel_policy_sparse_enum_validates(family, cw):
>> +    first = True
>> +    for _, attr_set in family.attr_sets.items():
>> +        if attr_set.subset_of:
>> +            continue
>> +
>> +        for _, attr in attr_set.items():
>> +            if not attr.request:
>> +                continue
>> +            if not attr.enum_name:
>> +                continue
>> +            if 'sparse' not in attr.checks:
>> +                continue
>> +
>> +            if first:
>> +                cw.p('/* Sparse enums validation callbacks */')
>> +                first = False
>> +
>> +            sign = '' if attr.type[0] == 'u' else '_signed'
>> +            suffix = 'ULL' if attr.type[0] == 'u' else 'LL'
>> +            cw.write_func_prot('static int', f'{c_lower(attr.enum_name)}_validate',
>> +                               ['const struct nlattr *attr', 'struct netlink_ext_ack *extack'])
>> +            cw.block_start()
>> +            cw.block_start(line=f'switch (nla_get_{attr["type"]}(attr))', noind=True)
>> +            enum = family.consts[attr['enum']]
>> +            for entry in enum.entries.values():
>> +                cw.p(f'case {entry.c_name}: return 0;')
>
>All the cases end in "return 0;"
>remove this, and add the return 0; before the block end.
>The code should look something like:
>
>	switch (nla_get_...) {
>	case VAL1:
>	case VAL2:
>	case VAL3:
>		return 0;
>	}

Sure.

Thanks!

>
>> +            cw.block_end(noind=True)
>> +            cw.p('NL_SET_ERR_MSG_ATTR(extack, attr, "invalid enum value");')
>> +            cw.p('return -EINVAL;')
>> +            cw.block_end()
>> +            cw.nl()
>> +
>> +
>>  def print_kernel_op_table_fwd(family, cw, terminate):
>>      exported = not kernel_can_gen_family_struct(family)
>>  
>> @@ -2965,6 +3003,7 @@ def main():
>>              print_kernel_family_struct_hdr(parsed, cw)
>>          else:
>>              print_kernel_policy_ranges(parsed, cw)
>> +            print_kernel_policy_sparse_enum_validates(parsed, cw)
>>  
>>              for _, struct in sorted(parsed.pure_nested_structs.items()):
>>                  if struct.request:
>

Re: [PATCH net-next 1/5] tools: ynl-gen: extend block_start/end by noind arg

[Jiri Pirko]

Sat, May 03, 2025 at 03:37:20AM +0200, kuba@kernel.org wrote:
>On Fri,  2 May 2025 13:38:17 +0200 Jiri Pirko wrote:
>> -    def block_start(self, line=''):
>> +    def block_start(self, line='', noind=False):
>>          if line:
>>              line = line + ' '
>>          self.p(line + '{')
>> -        self._ind += 1
>> +        if not noind:
>> +            self._ind += 1
>>  
>> -    def block_end(self, line=''):
>> +    def block_end(self, line='', noind=False):
>>          if line and line[0] not in {';', ','}:
>>              line = ' ' + line
>> -        self._ind -= 1
>> +        if not noind:
>> +            self._ind -= 1
>
>Should not be necessary, CodeWriter already automatically "unindents"
>lines which end with : as all switch cases should.

Got it. Will try. Thanks!