The /sys/class/devcoredump/disabled provides only one-way disable
functionality. Also, disabling devcoredump using it disables the
devcoredump functionality for everyone who is using it.
Provide a way to selectively enable/disable devcoredump for the device
which is bound to a driver that implements the '.coredump()' callback.
This adds the 'coredump_disabled' driver attribute. When the driver
implements the '.coredump()' callback, 'coredump_disabled' file is added
along with the 'coredump' file in the sysfs folder of the device upon
driver binding. The file is removed when the driver is unbound.
Drivers can use this attribute to enable/disable devcoredump and the
userspace can write 0 or 1 to /sys/devices/.../coredump_disabled sysfs
entry to control enabling/disabling of devcoredump for that device.
Signed-off-by: Manish Mandlik <[email protected]>
Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
---
Changes in v4:
- New patch in the series
drivers/base/dd.c | 43 +++++++++++++++++++++++++++++++++++---
drivers/base/devcoredump.c | 2 +-
include/linux/device.h | 4 ++++
3 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 11b0fb6414d3..c76d1145c6d9 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -426,6 +426,31 @@ static ssize_t coredump_store(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR_WO(coredump);
+static ssize_t coredump_disabled_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return scnprintf(buf, 3, "%d\n", dev->coredump_disabled);
+}
+
+static ssize_t coredump_disabled_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ long coredump_disabled;
+
+ if (!kstrtol(buf, 10, &coredump_disabled)) {
+ /* Consider any non-zero value as true */
+ if (coredump_disabled)
+ dev->coredump_disabled = true;
+ else
+ dev->coredump_disabled = false;
+ }
+
+ return count;
+}
+static DEVICE_ATTR_RW(coredump_disabled);
+
static int driver_sysfs_add(struct device *dev)
{
int ret;
@@ -448,9 +473,19 @@ static int driver_sysfs_add(struct device *dev)
return 0;
ret = device_create_file(dev, &dev_attr_coredump);
- if (!ret)
- return 0;
+ if (ret)
+ goto rm_driver;
+
+ ret = device_create_file(dev, &dev_attr_coredump_disabled);
+ if (ret)
+ goto rm_coredump;
+ return 0;
+
+rm_coredump:
+ device_remove_file(dev, &dev_attr_coredump);
+
+rm_driver:
sysfs_remove_link(&dev->kobj, "driver");
rm_dev:
@@ -466,8 +501,10 @@ static void driver_sysfs_remove(struct device *dev)
struct device_driver *drv = dev->driver;
if (drv) {
- if (drv->coredump)
+ if (drv->coredump) {
+ device_remove_file(dev, &dev_attr_coredump_disabled);
device_remove_file(dev, &dev_attr_coredump);
+ }
sysfs_remove_link(&drv->p->kobj, kobject_name(&dev->kobj));
sysfs_remove_link(&dev->kobj, "driver");
}
diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
index f4d794d6bb85..c5e9af9f3181 100644
--- a/drivers/base/devcoredump.c
+++ b/drivers/base/devcoredump.c
@@ -255,7 +255,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
struct devcd_entry *devcd;
struct device *existing;
- if (devcd_disabled)
+ if (devcd_disabled || dev->coredump_disabled)
goto free;
existing = class_find_device(&devcd_class, NULL, dev,
diff --git a/include/linux/device.h b/include/linux/device.h
index dc941997795c..120dd656f99d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -469,6 +469,8 @@ struct device_physical_location {
* This identifies the device type and carries type-specific
* information.
* @mutex: Mutex to synchronize calls to its driver.
+ * @coredump_disabled: Can be used by drivers to selectively enable/disable the
+ * coredump for a particular device via sysfs entry.
* @bus: Type of bus device is on.
* @driver: Which driver has allocated this
* @platform_data: Platform data specific to the device.
@@ -561,6 +563,8 @@ struct device {
const char *init_name; /* initial name of the device */
const struct device_type *type;
+ bool coredump_disabled;
+
struct bus_type *bus; /* type of bus device is on */
struct device_driver *driver; /* which driver has allocated this
device */
--
2.37.1.559.g78731f0fdb-goog
On Tue, Aug 09, 2022 at 08:35:24AM -0700, Manish Mandlik wrote:
> The /sys/class/devcoredump/disabled provides only one-way disable
> functionality. Also, disabling devcoredump using it disables the
> devcoredump functionality for everyone who is using it.
>
> Provide a way to selectively enable/disable devcoredump for the device
> which is bound to a driver that implements the '.coredump()' callback.
>
> This adds the 'coredump_disabled' driver attribute. When the driver
> implements the '.coredump()' callback, 'coredump_disabled' file is added
> along with the 'coredump' file in the sysfs folder of the device upon
> driver binding. The file is removed when the driver is unbound.
>
> Drivers can use this attribute to enable/disable devcoredump and the
> userspace can write 0 or 1 to /sys/devices/.../coredump_disabled sysfs
> entry to control enabling/disabling of devcoredump for that device.
>
> Signed-off-by: Manish Mandlik <[email protected]>
> Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
> ---
>
> Changes in v4:
> - New patch in the series
>
> drivers/base/dd.c | 43 +++++++++++++++++++++++++++++++++++---
> drivers/base/devcoredump.c | 2 +-
> include/linux/device.h | 4 ++++
> 3 files changed, 45 insertions(+), 4 deletions(-)
You can't add a new sysfs file without also a Documentation/ABI update
at the same time :(
thanks,
greg k-h
On Tue, Aug 09, 2022 at 08:35:24AM -0700, Manish Mandlik wrote:
> The /sys/class/devcoredump/disabled provides only one-way disable
> functionality. Also, disabling devcoredump using it disables the
> devcoredump functionality for everyone who is using it.
>
> Provide a way to selectively enable/disable devcoredump for the device
> which is bound to a driver that implements the '.coredump()' callback.
>
> This adds the 'coredump_disabled' driver attribute. When the driver
> implements the '.coredump()' callback, 'coredump_disabled' file is added
> along with the 'coredump' file in the sysfs folder of the device upon
> driver binding. The file is removed when the driver is unbound.
>
> Drivers can use this attribute to enable/disable devcoredump and the
> userspace can write 0 or 1 to /sys/devices/.../coredump_disabled sysfs
> entry to control enabling/disabling of devcoredump for that device.
>
> Signed-off-by: Manish Mandlik <[email protected]>
> Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
> ---
>
> Changes in v4:
> - New patch in the series
>
> drivers/base/dd.c | 43 +++++++++++++++++++++++++++++++++++---
> drivers/base/devcoredump.c | 2 +-
> include/linux/device.h | 4 ++++
> 3 files changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 11b0fb6414d3..c76d1145c6d9 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -426,6 +426,31 @@ static ssize_t coredump_store(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_WO(coredump);
>
> +static ssize_t coredump_disabled_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return scnprintf(buf, 3, "%d\n", dev->coredump_disabled);
Also, please use sysfs_emit() for any sysfs file output. It's in all
active kernels for a very long time now.
> +}
> +
> +static ssize_t coredump_disabled_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + long coredump_disabled;
> +
> + if (!kstrtol(buf, 10, &coredump_disabled)) {
> + /* Consider any non-zero value as true */
We have a "Y/N/0/1/y/n" check function for sysfs store callbacks that
you should use instead.
> + if (coredump_disabled)
> + dev->coredump_disabled = true;
> + else
> + dev->coredump_disabled = false;
> + }
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(coredump_disabled);
> +
> static int driver_sysfs_add(struct device *dev)
> {
> int ret;
> @@ -448,9 +473,19 @@ static int driver_sysfs_add(struct device *dev)
> return 0;
>
> ret = device_create_file(dev, &dev_attr_coredump);
> - if (!ret)
> - return 0;
> + if (ret)
> + goto rm_driver;
> +
> + ret = device_create_file(dev, &dev_attr_coredump_disabled);
Please use an attribute group now that you have multiple files.
> + if (ret)
> + goto rm_coredump;
>
> + return 0;
> +
> +rm_coredump:
> + device_remove_file(dev, &dev_attr_coredump);
> +
> +rm_driver:
> sysfs_remove_link(&dev->kobj, "driver");
>
> rm_dev:
> @@ -466,8 +501,10 @@ static void driver_sysfs_remove(struct device *dev)
> struct device_driver *drv = dev->driver;
>
> if (drv) {
> - if (drv->coredump)
> + if (drv->coredump) {
> + device_remove_file(dev, &dev_attr_coredump_disabled);
> device_remove_file(dev, &dev_attr_coredump);
> + }
> sysfs_remove_link(&drv->p->kobj, kobject_name(&dev->kobj));
> sysfs_remove_link(&dev->kobj, "driver");
> }
> diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> index f4d794d6bb85..c5e9af9f3181 100644
> --- a/drivers/base/devcoredump.c
> +++ b/drivers/base/devcoredump.c
> @@ -255,7 +255,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
> struct devcd_entry *devcd;
> struct device *existing;
>
> - if (devcd_disabled)
> + if (devcd_disabled || dev->coredump_disabled)
> goto free;
>
> existing = class_find_device(&devcd_class, NULL, dev,
> diff --git a/include/linux/device.h b/include/linux/device.h
> index dc941997795c..120dd656f99d 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -469,6 +469,8 @@ struct device_physical_location {
> * This identifies the device type and carries type-specific
> * information.
> * @mutex: Mutex to synchronize calls to its driver.
> + * @coredump_disabled: Can be used by drivers to selectively enable/disable the
> + * coredump for a particular device via sysfs entry.
> * @bus: Type of bus device is on.
> * @driver: Which driver has allocated this
> * @platform_data: Platform data specific to the device.
> @@ -561,6 +563,8 @@ struct device {
> const char *init_name; /* initial name of the device */
> const struct device_type *type;
>
> + bool coredump_disabled;
> +
That just hosed the alignment in this structure :(
Please be aware of how memory layouts for common kernel structures are
managed, and try not to add holes when you do not need to.
thanks,
greg k-h