netdev - Re: [PATCH net-next 2/2] devlink: Add auto dump flag to health reporter

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]

Message-ID: <20200325170135.28587e4a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 25 Mar 2020 17:01:35 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Eran Ben Elisha <eranbe@...lanox.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
        Jiri Pirko <jiri@...lanox.com>,
        Michael Chan <michael.chan@...adcom.com>,
        "David S. Miller" <davem@...emloft.net>,
        Saeed Mahameed <saeedm@...lanox.com>
Subject: Re: [PATCH net-next 2/2] devlink: Add auto dump flag to health
 reporter

On Wed, 25 Mar 2020 21:38:35 +0200 Eran Ben Elisha wrote:
> On 3/25/2020 9:08 PM, Jiri Pirko wrote:
> > Wed, Mar 25, 2020 at 07:45:29PM CET, kuba@...nel.org wrote:  
> >> On Wed, 25 Mar 2020 15:26:24 +0200 Eran Ben Elisha wrote:  
> >>> On low memory system, run time dumps can consume too much memory. Add
> >>> administrator ability to disable auto dumps per reporter as part of the
> >>> error flow handle routine.
> >>>
> >>> This attribute is not relevant while executing
> >>> DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET.
> >>>
> >>> By default, auto dump is activated for any reporter that has a dump method,
> >>> as part of the reporter registration to devlink.
> >>>
> >>> Signed-off-by: Eran Ben Elisha <eranbe@...lanox.com>
> >>> Reviewed-by: Jiri Pirko <jiri@...lanox.com>
> >>> ---
> >>>   include/uapi/linux/devlink.h |  2 ++
> >>>   net/core/devlink.c           | 26 ++++++++++++++++++++++----
> >>>   2 files changed, 24 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> >>> index dfdffc42e87d..e7891d1d2ebd 100644
> >>> --- a/include/uapi/linux/devlink.h
> >>> +++ b/include/uapi/linux/devlink.h
> >>> @@ -429,6 +429,8 @@ enum devlink_attr {
> >>>   	DEVLINK_ATTR_NETNS_FD,			/* u32 */
> >>>   	DEVLINK_ATTR_NETNS_PID,			/* u32 */
> >>>   	DEVLINK_ATTR_NETNS_ID,			/* u32 */
> >>> +
> >>> +	DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP,	/* u8 */
> >>>   	/* add new attributes above here, update the policy in devlink.c */
> >>>   
> >>>   	__DEVLINK_ATTR_MAX,
> >>> diff --git a/net/core/devlink.c b/net/core/devlink.c
> >>> index ad69379747ef..e14bf3052289 100644
> >>> --- a/net/core/devlink.c
> >>> +++ b/net/core/devlink.c
> >>> @@ -4837,6 +4837,7 @@ struct devlink_health_reporter {
> >>>   	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
> >>>   	u64 graceful_period;
> >>>   	bool auto_recover;
> >>> +	bool auto_dump;
> >>>   	u8 health_state;
> >>>   	u64 dump_ts;
> >>>   	u64 dump_real_ts;
> >>> @@ -4903,6 +4904,7 @@ devlink_health_reporter_create(struct devlink *devlink,
> >>>   	reporter->devlink = devlink;
> >>>   	reporter->graceful_period = graceful_period;
> >>>   	reporter->auto_recover = !!ops->recover;
> >>> +	reporter->auto_dump = !!ops->dump;
> >>>   	mutex_init(&reporter->dump_lock);
> >>>   	refcount_set(&reporter->refcount, 1);
> >>>   	list_add_tail(&reporter->list, &devlink->reporter_list);
> >>> @@ -4983,6 +4985,10 @@ devlink_nl_health_reporter_fill(struct sk_buff *msg,
> >>>   	    nla_put_u64_64bit(msg, DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS_NS,
> >>>   			      reporter->dump_real_ts, DEVLINK_ATTR_PAD))
> >>>   		goto reporter_nest_cancel;
> >>> +	if (reporter->ops->dump &&
> >>> +	    nla_put_u8(msg, DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP,
> >>> +		       reporter->auto_dump))
> >>> +		goto reporter_nest_cancel;  
> >>
> >> Since you're making it a u8 - does it make sense to indicate to user  
> > 
> > Please don't be mistaken. u8 carries a bool here.

Are you okay with limiting the value in the policy?

I guess the auto-recover doesn't have it so we'd create a little
inconsistency.

> >> space whether the dump is disabled or not supported?  
> > 
> > If you want to expose "not supported", I suggest to do it in another
> > attr. Because this attr is here to do the config from userspace. Would
> > be off if the same enum would carry "not supported".
> > 
> > But anyway, since you opened this can, the supported/capabilities
> > should be probably passed by a separate bitfield for all features.
> >   
> 
> Actually we supports trinary state per x attribute.
> (x can be auto-dump or auto-recover which is supported since day 1)
> 
> devlink_nl_health_reporter_fill can set
> DEVLINK_ATTR_HEALTH_REPORTER_AUTO_X to {0 , 1 , no set}
> And user space devlink propagates it accordingly.
> 
> If auto_x is supported, user will see "auto_x true"
> If auto_x is not supported, user will see "auto_x false"
> If x is not supported at all, user will not the auto_x at all for this 
> reporter.

Yeah, makes perfect the only glitch is that for auto-dump we have the
old kernel case. auto-recover was there from day 1.

> >> Right now no attribute means either old kernel or dump not possible.  
> when you run on old kernel, you will not see auto-dump attribute at all. 
> In any case you won't be able to distinguish in user space between 
> {auto-dump, no-auto-dump, no dump support}.

Right.

Anyway, I don't think this will matter in this particular case in
practice, so if you're okay with the code as is:

Reviewed-by: Jakub Kicinski <kuba@...nel.org>