[PATCH net-next 0/6] netlink: support reporting missing attributes

[PATCH net-next 0/6] netlink: support reporting missing attributes

[Jakub Kicinski]

This series adds support for reporting missing attributes
in a structured way. We can't point at things which don't
exist so we point at the containing nest or preceding
fixed header and report which attr type we expected to see.

Example of (YAML-based) user space reporting ethtool header
missing:

 Kernel error: missing attribute: .header

I was tempted to integrate the check with the policy
but it seems tricky without doing a full scan, and there
may be a ton of attrs in the policy. So leaving that
for later.

Jakub Kicinski (6):
  netlink: add support for ext_ack missing attributes
  netlink: add helper for extack attr presence checking
  genetlink: add helper for checking required attrs and use it in
    devlink
  devlink: use missing attribute ext_ack
  ethtool: strset: report missing ETHTOOL_A_STRINGSET_ID via ext_ack
  ethtool: report missing header via ext_ack in the default handler

 Documentation/userspace-api/netlink/intro.rst |  7 +++-
 include/linux/netlink.h                       | 24 +++++++++++
 include/net/genetlink.h                       | 14 +++++++
 include/uapi/linux/netlink.h                  |  4 ++
 net/core/devlink.c                            | 41 +++++++++----------
 net/ethtool/netlink.c                         |  3 ++
 net/ethtool/strset.c                          |  2 +-
 net/netlink/af_netlink.c                      | 12 ++++++
 net/netlink/genetlink.c                       |  2 +-
 9 files changed, 84 insertions(+), 25 deletions(-)

-- 
2.37.2

[PATCH net-next 1/6] netlink: add support for ext_ack missing attributes

[Jakub Kicinski]

There is currently no way to report via extack in a structured way
that an attribute is missing. This leads to families resorting to
string messages.

Add a pair of attributes - @offset and @type for machine-readable
way of reporting missing attributes. The @offset points to the
nest which should have contained the attribute, @type is the
expected nla_type. The offset may point to a struct nlattr
header or a fixed metadata header like struct genlmsghdr.

User space should be able to figure out which attribute enum
(AKA attribute space AKA attribute set) the nest pointed to by
@offset is using.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/userspace-api/netlink/intro.rst |  7 +++++--
 include/linux/netlink.h                       | 13 +++++++++++++
 include/uapi/linux/netlink.h                  |  4 ++++
 net/netlink/af_netlink.c                      | 12 ++++++++++++
 4 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/netlink/intro.rst b/Documentation/userspace-api/netlink/intro.rst
index 94337f79e077..8f1220756412 100644
--- a/Documentation/userspace-api/netlink/intro.rst
+++ b/Documentation/userspace-api/netlink/intro.rst
@@ -359,8 +359,8 @@ compatibility this feature has to be explicitly enabled by setting
 the ``NETLINK_EXT_ACK`` setsockopt() to ``1``.
 
 Types of extended ack attributes are defined in enum nlmsgerr_attrs.
-The two most commonly used attributes are ``NLMSGERR_ATTR_MSG``
-and ``NLMSGERR_ATTR_OFFS``.
+The most commonly used attributes are ``NLMSGERR_ATTR_MSG``,
+``NLMSGERR_ATTR_OFFS`` and ``NLMSGERR_ATTR_MISS_*``.
 
 ``NLMSGERR_ATTR_MSG`` carries a message in English describing
 the encountered problem. These messages are far more detailed
@@ -368,6 +368,9 @@ than what can be expressed thru standard UNIX error codes.
 
 ``NLMSGERR_ATTR_OFFS`` points to the attribute which caused the problem.
 
+``NLMSGERR_ATTR_MISS_TYPE`` and ``NLMSGERR_ATTR_MISS_NEST``
+inform about a missing attribute.
+
 Extended ACKs can be reported on errors as well as in case of success.
 The latter should be treated as a warning.
 
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index bda1c385cffb..24f9fece7599 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -71,6 +71,8 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
  *	%NL_SET_ERR_MSG
  * @bad_attr: attribute with error
  * @policy: policy for a bad attribute
+ * @miss_nest: nest which was missing an attribute
+ * @miss_type: type which was missing in @miss_nest
  * @cookie: cookie data to return to userspace (for success)
  * @cookie_len: actual cookie data length
  */
@@ -78,6 +80,8 @@ struct netlink_ext_ack {
 	const char *_msg;
 	const struct nlattr *bad_attr;
 	const struct nla_policy *policy;
+	const void *miss_nest;
+	u16 miss_type;
 	u8 cookie[NETLINK_MAX_COOKIE_LEN];
 	u8 cookie_len;
 };
@@ -126,6 +130,15 @@ struct netlink_ext_ack {
 #define NL_SET_ERR_MSG_ATTR(extack, attr, msg)		\
 	NL_SET_ERR_MSG_ATTR_POL(extack, attr, NULL, msg)
 
+#define NL_SET_ERR_ATTR_MISS(extack, nest, type)  do {	\
+	struct netlink_ext_ack *__extack = (extack);	\
+							\
+	if (__extack) {					\
+		__extack->miss_nest = (nest);		\
+		__extack->miss_type = (type);		\
+	}						\
+} while (0)
+
 static inline void nl_set_extack_cookie_u64(struct netlink_ext_ack *extack,
 					    u64 cookie)
 {
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index e0ab261ceca2..25d6d0764737 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -140,6 +140,8 @@ struct nlmsgerr {
  *	be used - in the success case - to identify a created
  *	object or operation or similar (binary)
  * @NLMSGERR_ATTR_POLICY: policy for a rejected attribute
+ * @NLMSGERR_ATTR_MISS_TYPE: type of a missing required attribute
+ * @NLMSGERR_ATTR_MISS_NEST: start of the payload where attr was missing
  * @__NLMSGERR_ATTR_MAX: number of attributes
  * @NLMSGERR_ATTR_MAX: highest attribute number
  */
@@ -149,6 +151,8 @@ enum nlmsgerr_attrs {
 	NLMSGERR_ATTR_OFFS,
 	NLMSGERR_ATTR_COOKIE,
 	NLMSGERR_ATTR_POLICY,
+	NLMSGERR_ATTR_MISS_TYPE,
+	NLMSGERR_ATTR_MISS_NEST,
 
 	__NLMSGERR_ATTR_MAX,
 	NLMSGERR_ATTR_MAX = __NLMSGERR_ATTR_MAX - 1
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 0cd91f813a3b..b00301e29d56 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2429,6 +2429,8 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 		tlvlen += nla_total_size(extack->cookie_len);
 	if (err && nlk_has_extack && extack && extack->policy)
 		tlvlen += netlink_policy_dump_attr_size_estimate(extack->policy);
+	if (err && nlk_has_extack && extack && extack->miss_nest)
+		tlvlen += nla_total_size(sizeof(u32)) * 2;
 
 	if (tlvlen)
 		flags |= NLM_F_ACK_TLVS;
@@ -2458,6 +2460,16 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 			WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_OFFS,
 					    (u8 *)extack->bad_attr -
 					    (u8 *)nlh));
+		if (err && extack->miss_nest &&
+		    !WARN_ON((u8 *)extack->miss_nest < in_skb->data ||
+			     (u8 *)extack->miss_nest > in_skb->data +
+						       in_skb->len)) {
+			WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_MISS_TYPE,
+					    extack->miss_type));
+			WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_MISS_NEST,
+					    (u8 *)extack->miss_nest -
+					    (u8 *)nlh));
+		}
 		if (extack->cookie_len)
 			WARN_ON(nla_put(skb, NLMSGERR_ATTR_COOKIE,
 					extack->cookie_len, extack->cookie));
-- 
2.37.2

[PATCH net-next 2/6] netlink: add helper for extack attr presence checking

[Jakub Kicinski]

Being able to check attribute presence and set extack
if not on one line is handy, add a helper.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/netlink.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 24f9fece7599..d20007ad7e26 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -139,6 +139,17 @@ struct netlink_ext_ack {
 	}						\
 } while (0)
 
+#define NL_REQ_ATTR_CHECK(extack, nest, tb, type) ({		\
+	struct nlattr **__tb = (tb);				\
+	u32 __attr = (type);					\
+	int __retval;						\
+								\
+	__retval = !__tb[__attr];				\
+	if (__retval)						\
+		NL_SET_ERR_ATTR_MISS((extack), (nest), __attr);	\
+	__retval;						\
+})
+
 static inline void nl_set_extack_cookie_u64(struct netlink_ext_ack *extack,
 					    u64 cookie)
 {
-- 
2.37.2

[PATCH net-next 3/6] genetlink: add helper for checking required attrs and use it in devlink

[Jakub Kicinski]

Add a genetlink wrapper for checking if a required attribute
is present in the root nest. Because we want the offset to
point at the inner-most header we need to differentiate
between families which have userhdr and those which don't.
Only ovs, tipc and drbd access userhdr, and they all have
additional userhdr so it's safe to set userhdr to NULL
for families which depend purely on attributes.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/genetlink.h | 14 ++++++++++++++
 net/netlink/genetlink.c |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 56a50e1c51b9..5da038e1b39e 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -107,6 +107,20 @@ static inline void genl_info_net_set(struct genl_info *info, struct net *net)
 
 #define GENL_SET_ERR_MSG(info, msg) NL_SET_ERR_MSG((info)->extack, msg)
 
+/* Report that a root attribute is missing */
+#define GENL_REQ_ATTR_CHECK(info, attr) ({				\
+	struct genl_info *__info = (info);				\
+	u32 __attr = (attr);						\
+	int __retval;							\
+									\
+	__retval = !__info->attrs[__attr];				\
+	if (__retval)							\
+		NL_SET_ERR_ATTR_MISS(__info->extack,			\
+				     __info->userhdr ? : __info->genlhdr, \
+				     __attr);				\
+	__retval;							\
+})
+
 enum genl_validate_flags {
 	GENL_DONT_VALIDATE_STRICT		= BIT(0),
 	GENL_DONT_VALIDATE_DUMP			= BIT(1),
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 57010927e20a..37805115e637 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -716,7 +716,7 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
 	info.snd_portid = NETLINK_CB(skb).portid;
 	info.nlhdr = nlh;
 	info.genlhdr = nlmsg_data(nlh);
-	info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
+	info.userhdr = family->hdrsize ? nlmsg_data(nlh) + GENL_HDRLEN : NULL;
 	info.attrs = attrbuf;
 	info.extack = extack;
 	genl_info_net_set(&info, net);
-- 
2.37.2

[PATCH net-next 4/6] devlink: use missing attribute ext_ack

[Jakub Kicinski]

Devlink with its global attr policy has a lot of attribute
presence check, use the new ext ack reporting when they are
missing.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/devlink.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index b50bcc18b8d9..e0073981e92d 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1710,7 +1710,7 @@ static int devlink_nl_cmd_port_split_doit(struct sk_buff *skb,
 	struct devlink *devlink = info->user_ptr[0];
 	u32 count;
 
-	if (!info->attrs[DEVLINK_ATTR_PORT_SPLIT_COUNT])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_PORT_SPLIT_COUNT))
 		return -EINVAL;
 	if (!devlink->ops->port_split)
 		return -EOPNOTSUPP;
@@ -1838,7 +1838,7 @@ static int devlink_nl_cmd_port_del_doit(struct sk_buff *skb,
 	if (!devlink->ops->port_del)
 		return -EOPNOTSUPP;
 
-	if (!info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_PORT_INDEX)) {
 		NL_SET_ERR_MSG_MOD(extack, "Port index is not specified");
 		return -EINVAL;
 	}
@@ -2690,7 +2690,7 @@ static int devlink_nl_cmd_sb_pool_set_doit(struct sk_buff *skb,
 	if (err)
 		return err;
 
-	if (!info->attrs[DEVLINK_ATTR_SB_POOL_SIZE])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_SB_POOL_SIZE))
 		return -EINVAL;
 
 	size = nla_get_u32(info->attrs[DEVLINK_ATTR_SB_POOL_SIZE]);
@@ -2900,7 +2900,7 @@ static int devlink_nl_cmd_sb_port_pool_set_doit(struct sk_buff *skb,
 	if (err)
 		return err;
 
-	if (!info->attrs[DEVLINK_ATTR_SB_THRESHOLD])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_SB_THRESHOLD))
 		return -EINVAL;
 
 	threshold = nla_get_u32(info->attrs[DEVLINK_ATTR_SB_THRESHOLD]);
@@ -3156,7 +3156,7 @@ static int devlink_nl_cmd_sb_tc_pool_bind_set_doit(struct sk_buff *skb,
 	if (err)
 		return err;
 
-	if (!info->attrs[DEVLINK_ATTR_SB_THRESHOLD])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_SB_THRESHOLD))
 		return -EINVAL;
 
 	threshold = nla_get_u32(info->attrs[DEVLINK_ATTR_SB_THRESHOLD]);
@@ -3845,7 +3845,7 @@ static int devlink_nl_cmd_dpipe_entries_get(struct sk_buff *skb,
 	struct devlink_dpipe_table *table;
 	const char *table_name;
 
-	if (!info->attrs[DEVLINK_ATTR_DPIPE_TABLE_NAME])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_DPIPE_TABLE_NAME))
 		return -EINVAL;
 
 	table_name = nla_data(info->attrs[DEVLINK_ATTR_DPIPE_TABLE_NAME]);
@@ -4029,8 +4029,9 @@ static int devlink_nl_cmd_dpipe_table_counters_set(struct sk_buff *skb,
 	const char *table_name;
 	bool counters_enable;
 
-	if (!info->attrs[DEVLINK_ATTR_DPIPE_TABLE_NAME] ||
-	    !info->attrs[DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_DPIPE_TABLE_NAME) ||
+	    GENL_REQ_ATTR_CHECK(info,
+				DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED))
 		return -EINVAL;
 
 	table_name = nla_data(info->attrs[DEVLINK_ATTR_DPIPE_TABLE_NAME]);
@@ -4119,8 +4120,8 @@ static int devlink_nl_cmd_resource_set(struct sk_buff *skb,
 	u64 size;
 	int err;
 
-	if (!info->attrs[DEVLINK_ATTR_RESOURCE_ID] ||
-	    !info->attrs[DEVLINK_ATTR_RESOURCE_SIZE])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_RESOURCE_ID) ||
+	    GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_RESOURCE_SIZE))
 		return -EINVAL;
 	resource_id = nla_get_u64(info->attrs[DEVLINK_ATTR_RESOURCE_ID]);
 
@@ -4755,7 +4756,7 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 	if (!devlink->ops->flash_update)
 		return -EOPNOTSUPP;
 
-	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME))
 		return -EINVAL;
 
 	supported_params = devlink->ops->supported_flash_update_params;
@@ -4936,10 +4937,8 @@ static int devlink_nl_cmd_selftests_run(struct sk_buff *skb,
 	if (!devlink->ops->selftest_run || !devlink->ops->selftest_check)
 		return -EOPNOTSUPP;
 
-	if (!info->attrs[DEVLINK_ATTR_SELFTESTS]) {
-		NL_SET_ERR_MSG_MOD(info->extack, "selftest required");
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_SELFTESTS))
 		return -EINVAL;
-	}
 
 	attrs = info->attrs[DEVLINK_ATTR_SELFTESTS];
 
@@ -5393,7 +5392,7 @@ static int
 devlink_param_type_get_from_info(struct genl_info *info,
 				 enum devlink_param_type *param_type)
 {
-	if (!info->attrs[DEVLINK_ATTR_PARAM_TYPE])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_PARAM_TYPE))
 		return -EINVAL;
 
 	switch (nla_get_u8(info->attrs[DEVLINK_ATTR_PARAM_TYPE])) {
@@ -5470,7 +5469,7 @@ devlink_param_get_from_info(struct list_head *param_list,
 {
 	char *param_name;
 
-	if (!info->attrs[DEVLINK_ATTR_PARAM_NAME])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_PARAM_NAME))
 		return NULL;
 
 	param_name = nla_data(info->attrs[DEVLINK_ATTR_PARAM_NAME]);
@@ -5536,7 +5535,7 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
 			return err;
 	}
 
-	if (!info->attrs[DEVLINK_ATTR_PARAM_VALUE_CMODE])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_PARAM_VALUE_CMODE))
 		return -EINVAL;
 	cmode = nla_get_u8(info->attrs[DEVLINK_ATTR_PARAM_VALUE_CMODE]);
 	if (!devlink_param_cmode_is_supported(param, cmode))
@@ -6056,7 +6055,7 @@ static int devlink_nl_cmd_region_get_doit(struct sk_buff *skb,
 	unsigned int index;
 	int err;
 
-	if (!info->attrs[DEVLINK_ATTR_REGION_NAME])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_REGION_NAME))
 		return -EINVAL;
 
 	if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
@@ -6189,8 +6188,8 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
 	unsigned int index;
 	u32 snapshot_id;
 
-	if (!info->attrs[DEVLINK_ATTR_REGION_NAME] ||
-	    !info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID])
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_REGION_NAME) ||
+	    GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_REGION_SNAPSHOT_ID))
 		return -EINVAL;
 
 	region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
@@ -6238,7 +6237,7 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 	u8 *data;
 	int err;
 
-	if (!info->attrs[DEVLINK_ATTR_REGION_NAME]) {
+	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_REGION_NAME)) {
 		NL_SET_ERR_MSG_MOD(info->extack, "No region name provided");
 		return -EINVAL;
 	}
-- 
2.37.2

[PATCH net-next 5/6] ethtool: strset: report missing ETHTOOL_A_STRINGSET_ID via ext_ack

[Jakub Kicinski]

Strset needs ETHTOOL_A_STRINGSET_ID, use it as an example of
reporting attrs missing in nests.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/strset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 2d51b7ab4dc5..3f7de54d85fb 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -167,7 +167,7 @@ static int strset_get_id(const struct nlattr *nest, u32 *val,
 			       get_stringset_policy, extack);
 	if (ret < 0)
 		return ret;
-	if (!tb[ETHTOOL_A_STRINGSET_ID])
+	if (NL_REQ_ATTR_CHECK(extack, nest, tb, ETHTOOL_A_STRINGSET_ID))
 		return -EINVAL;
 
 	*val = nla_get_u32(tb[ETHTOOL_A_STRINGSET_ID]);
-- 
2.37.2

[PATCH net-next 6/6] ethtool: report missing header via ext_ack in the default handler

[Jakub Kicinski]

The actual presence check for the header is in
ethnl_parse_header_dev_get() but it's a few layers in,
and already has a ton of arguments so let's just pick
the low handing fruit and check for missing header in
the default request handler.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/netlink.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index e26079e11835..0ccb177aaf04 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -361,6 +361,9 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
 	ops = ethnl_default_requests[cmd];
 	if (WARN_ONCE(!ops, "cmd %u has no ethnl_request_ops\n", cmd))
 		return -EOPNOTSUPP;
+	if (GENL_REQ_ATTR_CHECK(info, ops->hdr_attr))
+		return -EINVAL;
+
 	req_info = kzalloc(ops->req_info_size, GFP_KERNEL);
 	if (!req_info)
 		return -ENOMEM;
-- 
2.37.2

Re: [PATCH net-next 1/6] netlink: add support for ext_ack missing attributes

[Johannes Berg]

On Tue, 2022-08-23 at 21:50 -0700, Jakub Kicinski wrote:
> The @offset points to the
> nest which should have contained the attribute

I find this a bit tedious, tbh. You already kernel-side have patch 2 and
patch 3 that pass different things here.

Maybe it would be better to have this point to the _end_ of the {nest,
message} header, which - if there are any - would be equivalent to the
first sibling attribute?

Though I guess one way or the other userspace has to have an if that
asks whether or not it's in a nest or the top-level namespace.

Hmm.

How about we just _remove_ the NLMSGERR_ATTR_MISS_NEST attribute if it's
not missing in a nested attribute? That would make sense from the naming
too:
 * NLMSGERR_ATTR_MISS_TYPE - which attribute type you missed
 * NLMSGERR_ATTR_MISS_NEST - which nesting you missed it in, _if any_


And that way the if simplifies down to something like

	if (tb[NLMSGERR_ATTR_MISS_NEST])

in the consumer too, and you don't need GENL_REQ_ATTR_CHECK() at all,
you just pass NULL to the second argument of NL_REQ_ATTR_CHECK().


Sounds better to me, but YMMV.

johannes

Re: [PATCH net-next 1/6] netlink: add support for ext_ack missing attributes

[Johannes Berg]

On Wed, 2022-08-24 at 10:09 +0200, Johannes Berg wrote:
>  and you don't need GENL_REQ_ATTR_CHECK() at all,
> you just pass NULL to the second argument of NL_REQ_ATTR_CHECK().
> 

However, given that it takes fewer arguments since we can use the info
pointer, it might still be worthwhile having it, but at least it'd
collapse down to a direct call to NL_REQ_ATTR_CHECK().

johannes

Re: [PATCH net-next 1/6] netlink: add support for ext_ack missing attributes

[Jakub Kicinski]

On Wed, 24 Aug 2022 10:09:55 +0200 Johannes Berg wrote:
> On Tue, 2022-08-23 at 21:50 -0700, Jakub Kicinski wrote:
> > The @offset points to the
> > nest which should have contained the attribute  
> 
> I find this a bit tedious, tbh. You already kernel-side have patch 2 and
> patch 3 that pass different things here.
> 
> Maybe it would be better to have this point to the _end_ of the {nest,
> message} header, which - if there are any - would be equivalent to the
> first sibling attribute?

Pointing at the start of a nest is easier because I can reuse the same
"attr walking" logic in user space as for finding invalid attributes 
to find the nest.

> Though I guess one way or the other userspace has to have an if that
> asks whether or not it's in a nest or the top-level namespace.
> 
> Hmm.
> 
> How about we just _remove_ the NLMSGERR_ATTR_MISS_NEST attribute if it's
> not missing in a nested attribute? That would make sense from the naming
> too:
>  * NLMSGERR_ATTR_MISS_TYPE - which attribute type you missed
>  * NLMSGERR_ATTR_MISS_NEST - which nesting you missed it in, _if any_
> 
> 
> And that way the if simplifies down to something like
> 
> 	if (tb[NLMSGERR_ATTR_MISS_NEST])
> 
> in the consumer too, and you don't need GENL_REQ_ATTR_CHECK() at all,
> you just pass NULL to the second argument of NL_REQ_ATTR_CHECK().

Sounds good!

Re: [PATCH net-next 3/6] genetlink: add helper for checking required attrs and use it in devlink

[Johannes Berg]

On Tue, 2022-08-23 at 21:50 -0700, Jakub Kicinski wrote:
> 
> +/* Report that a root attribute is missing */
> +#define GENL_REQ_ATTR_CHECK(info, attr) ({				\
> +	struct genl_info *__info = (info);				\
> +	u32 __attr = (attr);						\
> +	int __retval;							\
> +									\
> +	__retval = !__info->attrs[__attr];				\
> +	if (__retval)							\
> +		NL_SET_ERR_ATTR_MISS(__info->extack,			\
> +				     __info->userhdr ? : __info->genlhdr, \
> +				     __attr);				\
> +	__retval;							\
> +})

Not sure this needs to be a macro btw, could be an inline returning a
bool? You're not really expanding anything here, nor doing something
with strings (unlike GENL_SET_ERR_MSG for example.)

johannes

Re: [PATCH net-next 3/6] genetlink: add helper for checking required attrs and use it in devlink

[Jakub Kicinski]

On Wed, 24 Aug 2022 21:44:22 +0200 Johannes Berg wrote:
> On Tue, 2022-08-23 at 21:50 -0700, Jakub Kicinski wrote:
> > 
> > +/* Report that a root attribute is missing */
> > +#define GENL_REQ_ATTR_CHECK(info, attr) ({				\
> > +	struct genl_info *__info = (info);				\
> > +	u32 __attr = (attr);						\
> > +	int __retval;							\
> > +									\
> > +	__retval = !__info->attrs[__attr];				\
> > +	if (__retval)							\
> > +		NL_SET_ERR_ATTR_MISS(__info->extack,			\
> > +				     __info->userhdr ? : __info->genlhdr, \
> > +				     __attr);				\
> > +	__retval;							\
> > +})  
> 
> Not sure this needs to be a macro btw, could be an inline returning a
> bool? You're not really expanding anything here, nor doing something
> with strings (unlike GENL_SET_ERR_MSG for example.)

Initially I typed up both flavors with and without the message 
but I dropped the _MSG() one since I didn't find a strong enough 
reason to use it.

If we do get the _MSG() version at some point (perhaps to preserve 
an existing message during conversion?) having different case
would seem off.

I have no opinion which way is better, LMK if you prefer lower case
(ignoring existing non-MSG helpers being upper case) and I'll sed thru
the patches, no problem at all.