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: <20200326103913.150b8108@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 26 Mar 2020 10:39:13 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Eran Ben Elisha <eranbe@...lanox.com>, 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 Thu, 26 Mar 2020 11:22:44 +0100 Jiri Pirko wrote:
> >> >>> @@ -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?  
> 
> Well, not-0 means true. Do you think it is wise to limit to 0/1?

Just future proofing, in general seems wise to always constrain the
input as much as possible. But in this case we already have similar
attrs in the dump which don't have the constraint, and we will probably
want consistency, so maybe we're unlikely to use other values.

In particular I was wondering if auto-dump value can be extended to
mean the number of dumps we want to collect, the current behavior I
think matches collecting just one. But obviously this can be solved
with a new attr when needed..