| 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
| ||
|
Message-ID: <2025071645-panoramic-pyromania-2f8c@gregkh>
Date: Wed, 16 Jul 2025 14:43:47 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Oleksij Rempel <o.rempel@...gutronix.de>,
Sebastian Reichel <sre@...nel.org>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Benson Leung <bleung@...omium.org>,
Tzung-Bi Shih <tzungbi@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Oleksij Rempel <o.rempel@...gutronix.de>, kernel@...gutronix.de,
linux-kernel@...r.kernel.org, Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Zhang Rui <rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
linux-pm@...r.kernel.org,
Søren Andersen <san@...v.dk>,
Guenter Roeck <groeck@...omium.org>,
Matti Vaittinen <mazziesaccount@...il.com>,
Ahmad Fatoum <a.fatoum@...gutronix.de>,
Andrew Morton <akpm@...ux-foundation.org>,
chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v11 3/7] power: reset: Introduce PSCR Recording Framework
for Non-Volatile Storage
Overall, no real issues, just some very minor ones:
On Wed, Jun 18, 2025 at 02:02:51PM +0200, Oleksij Rempel wrote:
> + * Sysfs Interface:
> + * ----------------
> + * /sys/kernel/pscrr/reason - Read/write current power state change
> + * reason
> + * /sys/kernel/pscrr/reason_boot - Read-only last recorded reason from
> + * previous boot
The sysfs documentation is in the ABI file, so it's not needed here.
> +static int pscrr_reboot_notifier(struct notifier_block *nb,
> + unsigned long action, void *unused)
> +{
> + struct pscrr_backend *backend;
> + int ret;
> +
> + guard(g_pscrr)(&g_pscrr);
> +
> + backend = g_pscrr.backend;
> +
> + if (!backend || !backend->ops || !backend->ops->write_reason)
> + return NOTIFY_DONE;
> +
> + ret = backend->ops->write_reason(get_psc_reason());
> + if (ret) {
> + pr_err("PSCRR: Failed to store reason %d (%s) at reboot, err=%pe\n",
> + get_psc_reason(), psc_reason_to_str(get_psc_reason()),
> + ERR_PTR(ret));
> + } else {
> + pr_info("PSCRR: Stored reason %d (%s) at reboot.\n",
> + get_psc_reason(), psc_reason_to_str(get_psc_reason()));
Why print anything? If this works properly, it should be quiet, right?
> +static ssize_t reason_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + struct pscrr_backend *backend;
> + enum psc_reason r;
> +
> + guard(g_pscrr)(&g_pscrr);
> +
> + backend = g_pscrr.backend;
> +
> + if (!backend || !backend->ops)
> + return scnprintf(buf, PAGE_SIZE, "No backend registered\n");
So a string, or an int will be returned? That's crazy, just return an
error here, -ENODEV?
> +
> + /* If the backend can read from hardware, do so. Otherwise, use our cached value. */
> + if (backend->ops->read_reason) {
> + if (backend->ops->read_reason(&r) == 0) {
> + /* Also update our cached value for consistency */
> + set_psc_reason(r);
> + } else {
> + /* If read fails, fallback to cached. */
> + r = get_psc_reason();
> + }
> + } else {
> + r = get_psc_reason();
> + }
> +
> + return scnprintf(buf, PAGE_SIZE, "%d\n", r);
sysfs files should use sysfs_emit() so you don't get people sending you
patches later on to convert it :)
> +static ssize_t reason_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct pscrr_backend *backend;
> + long val;
> + int ret;
> +
> + guard(g_pscrr)(&g_pscrr);
> +
> + backend = g_pscrr.backend;
> +
> + if (!backend || !backend->ops || !backend->ops->write_reason)
> + return -ENODEV;
> +
> + ret = kstrtol(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > U32_MAX)
> + return -ERANGE;
> +
> + if (val < PSCR_UNKNOWN || val > PSCR_MAX_REASON)
> + /*
> + * Log a warning, but still attempt to write the value. In
> + * case the backend can handle it, we don't want to block it.
> + */
> + pr_warn("PSCRR: writing unknown reason %ld (out of range)\n",
> + val);
Do not let userspace cause a DoS of kernel log messages just because it
sent you an invalid data range.
> +static struct kobj_attribute reason_attr = __ATTR(reason, 0644, reason_show,
> + reason_store);
__ATTR_RW(), right? If not, why not?
> +static struct kobj_attribute reason_boot_attr =
> + __ATTR(reason_boot, 0444, reason_boot_show, NULL); /* Read-only */
__ATTR_RO(), right? Then no comment is needed :)
> +int pscrr_core_init(const struct pscrr_backend_ops *ops)
> +{
> + enum psc_reason stored_val = PSCR_UNKNOWN;
> + struct pscrr_backend *backend;
> + int ret;
> +
> + guard(g_pscrr)(&g_pscrr);
> +
> + backend = g_pscrr.backend;
> +
> + if (backend) {
> + pr_err("PSCRR: Core is already initialized!\n");
All of the "PSCRR:" stuff should just be set with pr_fmt being defined
at the top of this file, don't put it everywhere manually.
> + return -EBUSY;
> + }
> +
> + if (!ops->read_reason) {
> + pr_err("PSCRR: Backend must provide read callbacks\n");
> + return -EINVAL;
> + }
> +
> + backend = kzalloc(sizeof(*backend), GFP_KERNEL);
> + if (!backend)
> + return -ENOMEM;
> +
> + backend->ops = ops;
> + backend->last_boot_reason = PSCR_UNKNOWN;
> + g_pscrr.backend = backend;
> +
> + ret = ops->read_reason(&stored_val);
> + if (!ret) {
> + backend->last_boot_reason = stored_val;
> + pr_info("PSCRR: Initial read_reason: %d (%s)\n",
> + stored_val, psc_reason_to_str(stored_val));
When code works properly, it should be quiet. Don't spam the boot log
please.
> + ret = sysfs_create_group(g_pscrr.kobj, &pscrr_attr_group);
> + if (ret) {
> + pr_err("PSCRR: Failed to create sysfs group, err=%pe\n",
> + ERR_PTR(ret));
> + goto err_kobj_put;
> + }
> +
> + pr_info("PSCRR: initialized successfully.\n");
Same here, be quiet.
> +void pscrr_core_exit(void)
> +{
> + guard(g_pscrr)(&g_pscrr);
> +
> + if (!g_pscrr.backend)
> + return;
> +
> + if (g_pscrr.kobj) {
> + sysfs_remove_group(g_pscrr.kobj, &pscrr_attr_group);
> + kobject_put(g_pscrr.kobj);
> + }
> +
> + unregister_reboot_notifier(&g_pscrr.reboot_nb);
> +
> + kfree(g_pscrr.backend);
> + g_pscrr.backend = NULL;
> +
> + pr_info("PSCRR: exited.\n");
Same here, please be quiet.
thanks,
greg k-h
Powered by blists - more mailing lists