netdev - Re: [RFC net-next v2 2/3] devlink: health: add remediation type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 11 Mar 2021 08:48:33 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     f242ed68-d31b-527d-562f-c5a35123861a@...el.com
Cc:     netdev@...r.kernel.org, saeedm@...dia.com,
        andrew.gospodarek@...adcom.com, jacob.e.keller@...el.com,
        guglielmo.morandin@...adcom.com, eugenem@...com,
        eranbe@...lanox.com, Jakub Kicinski <kuba@...nel.org>
Subject: Re: [RFC net-next v2 2/3] devlink: health: add remediation type

Thu, Mar 11, 2021 at 04:26:12AM CET, kuba@...nel.org wrote:
>Currently devlink health does not give user any clear information
>of what kind of remediation ->recover callback will perform. This
>makes it difficult to understand the impact of enabling auto-
>-remediation, and the severity of the error itself.
>
>To allow users to make more informed decision add a new remediation
>type attribute.
>
>Note that we only allow one remediation type per reporter, this
>is intentional. devlink health is not built for mixing issues
>of different severity into one reporter since it only maintains
>one dump, of the first event and a single error counter.
>Nudging vendors towards categorizing issues beyond coarse
>groups is an added bonus.
>
>Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>---
> include/net/devlink.h        |  2 ++
> include/uapi/linux/devlink.h | 25 +++++++++++++++++++++++++
> net/core/devlink.c           |  7 ++++++-
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index b424328af658..72b37769761f 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -659,6 +659,7 @@ struct devlink_health_reporter;
> /**
>  * struct devlink_health_reporter_ops - Reporter operations
>  * @name: reporter name
>+ * remedy: severity of the remediation required
>  * @recover: callback to recover from reported error
>  *           if priv_ctx is NULL, run a full recover
>  * @dump: callback to dump an object
>@@ -669,6 +670,7 @@ struct devlink_health_reporter;
> 
> struct devlink_health_reporter_ops {
> 	char *name;
>+	enum devlink_health_remedy remedy;
> 	int (*recover)(struct devlink_health_reporter *reporter,
> 		       void *priv_ctx, struct netlink_ext_ack *extack);
> 	int (*dump)(struct devlink_health_reporter *reporter,
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 41a6ea3b2256..8cd1508b525b 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -534,6 +534,9 @@ enum devlink_attr {
> 	DEVLINK_ATTR_RELOAD_ACTION_STATS,       /* nested */
> 
> 	DEVLINK_ATTR_PORT_PCI_SF_NUMBER,	/* u32 */
>+
>+	DEVLINK_ATTR_HEALTH_REPORTER_REMEDY,	/* u32 */
>+
> 	/* add new attributes above here, update the policy in devlink.c */
> 
> 	__DEVLINK_ATTR_MAX,
>@@ -620,4 +623,26 @@ enum devlink_health_state {
> 	DL_HEALTH_STATE_ERROR,
> };
> 
>+/**
>+ * enum devlink_health_reporter_remedy - severity of remediation procedure
>+ * @DL_HEALTH_REMEDY_NONE: transient error, no remediation required
>+ * @DL_HEALTH_REMEDY_KICK: device stalled, processing will be re-triggered
>+ * @DL_HEALTH_REMEDY_COMP_RESET: associated device component (e.g. device queue)
>+ *			will be reset
>+ * @DL_HEALTH_REMEDY_RESET: full device reset, will result in temporary
>+ *			unavailability of the device, device configuration
>+ *			should not be lost
>+ * @DL_HEALTH_REMEDY_REINIT: device will be reinitialized and configuration lost
>+ *
>+ * Used in %DEVLINK_ATTR_HEALTH_REPORTER_REMEDY, categorizes the health reporter
>+ * by the severity of the remediation.
>+ */
>+enum devlink_health_remedy {
>+	DL_HEALTH_REMEDY_NONE = 1,
>+	DL_HEALTH_REMEDY_KICK,
>+	DL_HEALTH_REMEDY_COMP_RESET,
>+	DL_HEALTH_REMEDY_RESET,
>+	DL_HEALTH_REMEDY_REINIT,

It is nice if enum name and values are consistent:
enum something {
	SOMETHING_*


>+};
>+
> #endif /* _UAPI_LINUX_DEVLINK_H_ */
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 8e4e4bd7bb36..09d77d43ff63 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -6095,7 +6095,8 @@ __devlink_health_reporter_create(struct devlink *devlink,
> {
> 	struct devlink_health_reporter *reporter;
> 
>-	if (WARN_ON(graceful_period && !ops->recover))
>+	if (WARN_ON(graceful_period && !ops->recover) ||
>+	    WARN_ON(ops->recover && !ops->remedy))
> 		return ERR_PTR(-EINVAL);
> 
> 	reporter = kzalloc(sizeof(*reporter), GFP_KERNEL);
>@@ -6265,6 +6266,10 @@ devlink_nl_health_reporter_fill(struct sk_buff *msg,
> 	if (nla_put_string(msg, DEVLINK_ATTR_HEALTH_REPORTER_NAME,
> 			   reporter->ops->name))
> 		goto reporter_nest_cancel;
>+	if (reporter->ops->remedy &&
>+	    nla_put_u32(msg, DEVLINK_ATTR_HEALTH_REPORTER_REMEDY,
>+			reporter->ops->remedy))
>+		goto reporter_nest_cancel;
> 	if (nla_put_u8(msg, DEVLINK_ATTR_HEALTH_REPORTER_STATE,
> 		       reporter->health_state))
> 		goto reporter_nest_cancel;
>-- 
>2.29.2
>