| 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
| ||
|
Date: Wed, 25 Mar 2020 21:38:35 +0200
From: Eran Ben Elisha <eranbe@...lanox.com>
To: Jiri Pirko <jiri@...nulli.us>, Jakub Kicinski <kuba@...nel.org>
Cc: 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 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.
>
>
>> 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.
>
>>
>> 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}.
>>
>>> nla_nest_end(msg, reporter_attr);
>>> genlmsg_end(msg, hdr);
>>> @@ -5129,10 +5135,12 @@ int devlink_health_report(struct devlink_health_reporter *reporter,
>>>
>>> reporter->health_state = DEVLINK_HEALTH_REPORTER_STATE_ERROR;
>>>
>>> - mutex_lock(&reporter->dump_lock);
>>> - /* store current dump of current error, for later analysis */
>>> - devlink_health_do_dump(reporter, priv_ctx, NULL);
>>> - mutex_unlock(&reporter->dump_lock);
>>> + if (reporter->auto_dump) {
>>> + mutex_lock(&reporter->dump_lock);
>>> + /* store current dump of current error, for later analysis */
>>> + devlink_health_do_dump(reporter, priv_ctx, NULL);
>>> + mutex_unlock(&reporter->dump_lock);
>>> + }
>>>
>>> if (reporter->auto_recover)
>>> return devlink_health_reporter_recover(reporter,
>>> @@ -5306,6 +5314,11 @@ devlink_nl_cmd_health_reporter_set_doit(struct sk_buff *skb,
>>> err = -EOPNOTSUPP;
>>> goto out;
>>> }
>>> + if (!reporter->ops->dump &&
>>> + info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP]) {
>>
>> ... and then this behavior may have to change, I think?
>>
>>> + err = -EOPNOTSUPP;
>>> + goto out;
>>> + }
>>>
>>> if (info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD])
>>> reporter->graceful_period =
>>> @@ -5315,6 +5328,10 @@ devlink_nl_cmd_health_reporter_set_doit(struct sk_buff *skb,
>>> reporter->auto_recover =
>>> nla_get_u8(info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER]);
>>>
>>> + if (info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP])
>>> + reporter->auto_dump =
>>> + nla_get_u8(info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP]);
>>> +
>>> devlink_health_reporter_put(reporter);
>>> return 0;
>>> out:
>>> @@ -6053,6 +6070,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
>>> [DEVLINK_ATTR_HEALTH_REPORTER_NAME] = { .type = NLA_NUL_STRING },
>>> [DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD] = { .type = NLA_U64 },
>>> [DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER] = { .type = NLA_U8 },
>>> + [DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP] = { .type = NLA_U8 },
>>
>> I'd suggest we keep the attrs in order of definition, because we should
>> set .strict_start_type, and then it matters which are before and which
>> are after.
>>
>> Also please set max value of 1.
>>
>>> [DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME] = { .type = NLA_NUL_STRING },
>>> [DEVLINK_ATTR_FLASH_UPDATE_COMPONENT] = { .type = NLA_NUL_STRING },
>>> [DEVLINK_ATTR_TRAP_NAME] = { .type = NLA_NUL_STRING },
>>
Powered by blists - more mailing lists