[PATCH] power: supply: ds2760_battery: fix NULL pointer dereference in w1_ds2760_remove_slave()

[PATCH] power: supply: ds2760_battery: fix NULL pointer dereference in w1_ds2760_remove_slave()

[Ivy Lopez]

w1_ds2760_add_slave()'s failure paths (di_alloc_failed, batt_failed,
workqueue_failed) are all empty labels that just return the error
code, with no explicit unwind. This works for di itself and the
registered power supply, both are devm-managed against the w1 slave
device and get cleaned up automatically. But di->monitor_wqueue and
di->pm_notifier are not devm-managed, and are only ever set up in
the success tail of the function, after the workqueue allocation and
power supply registration have both succeeded.

The w1 core's BUS_NOTIFY_ADD_DEVICE handling in w1_family_notify()
logs and returns on a failing add_slave(), but does not prevent the
w1 slave device from later being removed from the bus, which
triggers BUS_NOTIFY_DEL_DEVICE and an unconditional call to
remove_slave(). This means w1_ds2760_remove_slave() is effectively
the only failure-unwind path for a partially initialized di, and
needs to treat every field as potentially never having been set.

Currently it does not: it unconditionally calls
destroy_workqueue(di->monitor_wqueue), which crashes with a NULL
pointer dereference if add_slave() failed before or during the
workqueue allocation (e.g. on a power_supply_register() failure, as
seen when a colliding sysfs name from a misdetected slave device
causes registration to fail).

sl->family_data can also be NULL if add_slave() failed at its own
allocation, before family_data was ever set, which would crash on
the very first dereference in remove_slave().

Fix both: return early if di is NULL, and only call
destroy_workqueue() if monitor_wqueue was actually allocated.
unregister_pm_notifier() and cancel_delayed_work_sync() are safe to
call unconditionally: the former is a no-op if the notifier was
never registered, and the latter operates on the embedded
delayed_work struct, which is always validly initialized by the
time remove_slave() can run with a non-NULL di.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217832
Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
 drivers/power/supply/ds2760_battery.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/ds2760_battery.c b/drivers/power/supply/ds2760_battery.c
index 142c7492c3c2..3c2433f6b5e9 100644
--- a/drivers/power/supply/ds2760_battery.c
+++ b/drivers/power/supply/ds2760_battery.c
@@ -723,9 +723,13 @@ static void w1_ds2760_remove_slave(struct w1_slave *sl)
 {
 	struct ds2760_device_info *di = sl->family_data;
 
+	if (!di)
+		return;
+
 	unregister_pm_notifier(&di->pm_notifier);
 	cancel_delayed_work_sync(&di->monitor_work);
-	destroy_workqueue(di->monitor_wqueue);
+	if (di->monitor_wqueue)
+		destroy_workqueue(di->monitor_wqueue);
 }
 
 #ifdef CONFIG_OF
-- 
2.55.0

Re: [PATCH] power: supply: ds2760_battery: fix NULL pointer dereference in w1_ds2760_remove_slave()

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3536 bytes --]

Hi,

On Sat, Aug 08, 2026 at 03:44:58PM -0600, Ivy Lopez wrote:
> w1_ds2760_add_slave()'s failure paths (di_alloc_failed, batt_failed,
> workqueue_failed) are all empty labels that just return the error
> code, with no explicit unwind. This works for di itself and the
> registered power supply, both are devm-managed against the w1 slave
> device and get cleaned up automatically. But di->monitor_wqueue and
> di->pm_notifier are not devm-managed, and are only ever set up in
> the success tail of the function, after the workqueue allocation and
> power supply registration have both succeeded.
> 
> The w1 core's BUS_NOTIFY_ADD_DEVICE handling in w1_family_notify()
> logs and returns on a failing add_slave(), but does not prevent the
> w1 slave device from later being removed from the bus, which
> triggers BUS_NOTIFY_DEL_DEVICE and an unconditional call to
> remove_slave(). This means w1_ds2760_remove_slave() is effectively
> the only failure-unwind path for a partially initialized di, and
> needs to treat every field as potentially never having been set.
> 
> Currently it does not: it unconditionally calls
> destroy_workqueue(di->monitor_wqueue), which crashes with a NULL
> pointer dereference if add_slave() failed before or during the
> workqueue allocation (e.g. on a power_supply_register() failure, as
> seen when a colliding sysfs name from a misdetected slave device
> causes registration to fail).
> 
> sl->family_data can also be NULL if add_slave() failed at its own
> allocation, before family_data was ever set, which would crash on
> the very first dereference in remove_slave().
> 
> Fix both: return early if di is NULL, and only call
> destroy_workqueue() if monitor_wqueue was actually allocated.
> unregister_pm_notifier() and cancel_delayed_work_sync() are safe to
> call unconditionally: the former is a no-op if the notifier was
> never registered, and the latter operates on the embedded
> delayed_work struct, which is always validly initialized by the
> time remove_slave() can run with a non-NULL di.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217832
> Signed-off-by: Ivy Lopez <skunkolee@gmail.com>

This should get a Fixes tag.

> ---
>  drivers/power/supply/ds2760_battery.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/ds2760_battery.c b/drivers/power/supply/ds2760_battery.c
> index 142c7492c3c2..3c2433f6b5e9 100644
> --- a/drivers/power/supply/ds2760_battery.c
> +++ b/drivers/power/supply/ds2760_battery.c
> @@ -723,9 +723,13 @@ static void w1_ds2760_remove_slave(struct w1_slave *sl)
>  {
>  	struct ds2760_device_info *di = sl->family_data;
>  
> +	if (!di)
> +		return;
> +
>  	unregister_pm_notifier(&di->pm_notifier);
>  	cancel_delayed_work_sync(&di->monitor_work);
> -	destroy_workqueue(di->monitor_wqueue);
> +	if (di->monitor_wqueue)
> +		destroy_workqueue(di->monitor_wqueue);

Considering the driver has mostly been converted to device managed
resources already, I think it makes sense to simply use
devm_alloc_ordered_workqueue() in the probe function and thus avoid
this problem. Also while at it switch INIT_DELAYED_WORK to
devm_delayed_work_autocancel() to get rid of
w1_ds2760_remove_slave(). Just make sure to do this *after*
allocating the workqueue.

Last but not least use devm_add_action_or_reset() for the
unregister_pm_notifier and simply drop the complete
w1_ds2760_remove_slave() function.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] power: supply: ds2760_battery: convert to devm-managed workqueue and pm_notifier

[Ivy Lopez]

Following review feedback, convert the driver to use devm-managed
resources instead of manual cleanup in w1_ds2760_remove_slave():

- devm_alloc_ordered_workqueue() for the monitor workqueue
- devm_delayed_work_autocancel() for the monitor delayed work
- devm_add_action_or_reset() to unregister the pm_notifier

This removes the need for w1_ds2760_remove_slave() entirely, along
with the NULL checks it required to safely handle a partially
initialized di on an add_slave() failure path.

Fixes: bf4973553737 ("power: supply: ds2760_battery: merge ds2760 supply driver with its w1 slave companion")
Suggested-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
 drivers/power/supply/ds2760_battery.c | 54 +++++++++++----------------
 1 file changed, 21 insertions(+), 33 deletions(-)

diff --git a/drivers/power/supply/ds2760_battery.c b/drivers/power/supply/ds2760_battery.c
index 3c2433f6b5e9..901aaaf916c4 100644
--- a/drivers/power/supply/ds2760_battery.c
+++ b/drivers/power/supply/ds2760_battery.c
@@ -23,6 +23,7 @@
 #include <linux/param.h>
 #include <linux/jiffies.h>
 #include <linux/workqueue.h>
+#include <linux/devm-helpers.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
@@ -621,6 +622,11 @@ static int ds2760_pm_notifier(struct notifier_block *notifier,
 	return NOTIFY_DONE;
 }
 
+static void ds2760_battery_unregister_pm_notifier(void *notifier)
+{
+	unregister_pm_notifier(notifier);
+}
+
 static int w1_ds2760_add_slave(struct w1_slave *sl)
 {
 	struct power_supply_config psy_cfg = {};
@@ -631,10 +637,8 @@ static int w1_ds2760_add_slave(struct w1_slave *sl)
 	char status;
 
 	di = devm_kzalloc(dev, sizeof(*di), GFP_KERNEL);
-	if (!di) {
-		retval = -ENOMEM;
-		goto di_alloc_failed;
-	}
+	if (!di)
+		return -ENOMEM;
 
 	snprintf(name, sizeof(name), "ds2760-battery.%d", dev->id);
 
@@ -695,41 +699,26 @@ static int w1_ds2760_add_slave(struct w1_slave *sl)
 	di->bat = devm_power_supply_register(dev, &di->bat_desc, &psy_cfg);
 	if (IS_ERR(di->bat)) {
 		dev_err(di->dev, "failed to register battery\n");
-		retval = PTR_ERR(di->bat);
-		goto batt_failed;
+		return PTR_ERR(di->bat);
 	}
 
-	INIT_DELAYED_WORK(&di->monitor_work, ds2760_battery_work);
-	di->monitor_wqueue = alloc_ordered_workqueue(name, WQ_MEM_RECLAIM);
-	if (!di->monitor_wqueue) {
-		retval = -ESRCH;
-		goto workqueue_failed;
-	}
+	di->monitor_wqueue = devm_alloc_ordered_workqueue(dev, name, WQ_MEM_RECLAIM);
+	if (!di->monitor_wqueue)
+		return -ESRCH;
+
+	retval = devm_delayed_work_autocancel(dev, &di->monitor_work,
+						 ds2760_battery_work);
+	if (retval)
+		return retval;
+
 	queue_delayed_work(di->monitor_wqueue, &di->monitor_work, HZ * 1);
 
 	di->pm_notifier.notifier_call = ds2760_pm_notifier;
 	register_pm_notifier(&di->pm_notifier);
 
-	goto success;
-
-workqueue_failed:
-batt_failed:
-di_alloc_failed:
-success:
-	return retval;
-}
-
-static void w1_ds2760_remove_slave(struct w1_slave *sl)
-{
-	struct ds2760_device_info *di = sl->family_data;
-
-	if (!di)
-		return;
-
-	unregister_pm_notifier(&di->pm_notifier);
-	cancel_delayed_work_sync(&di->monitor_work);
-	if (di->monitor_wqueue)
-		destroy_workqueue(di->monitor_wqueue);
+	return devm_add_action_or_reset(dev,
+					   ds2760_battery_unregister_pm_notifier,
+					   &di->pm_notifier);
 }
 
 #ifdef CONFIG_OF
@@ -741,7 +730,6 @@ static const struct of_device_id w1_ds2760_of_ids[] = {
 
 static const struct w1_family_ops w1_ds2760_fops = {
 	.add_slave	= w1_ds2760_add_slave,
-	.remove_slave	= w1_ds2760_remove_slave,
 	.groups		= w1_ds2760_groups,
 };
 
-- 
2.55.0

[PATCH] w1: fix spelling mistakes in comments across the subsystem

[Ivy Lopez]

Following up on a prior patch that only fixed w1_netlink.c, sweep
the rest of the w1 subsystem for the same class of comment typos:
"loosing" -> "losing", "deatch" -> "detach", "messagse" -> "messages",
"continusly" -> "continuously", "attribut" -> "attribute",
"deactive" -> "deactivate", "determing" -> "determining",
"discrepency" -> "discrepancy", "rerurn" -> "return".

No functional change.

Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
 drivers/w1/masters/ds2482.c    | 2 +-
 drivers/w1/masters/ds2490.c    | 4 ++--
 drivers/w1/masters/omap_hdq.c  | 2 +-
 drivers/w1/slaves/w1_ds28e17.c | 2 +-
 drivers/w1/slaves/w1_therm.c   | 2 +-
 drivers/w1/w1.c                | 2 +-
 drivers/w1/w1_family.c         | 2 +-
 drivers/w1/w1_netlink.h        | 2 +-
 8 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/w1/masters/ds2482.c b/drivers/w1/masters/ds2482.c
index 0069e6f854d7..040d66415a9b 100644
--- a/drivers/w1/masters/ds2482.c
+++ b/drivers/w1/masters/ds2482.c
@@ -421,7 +421,7 @@ static u8 ds2482_w1_set_pullup(void *data, int delay)
 
 	/* if delay is non-zero activate the pullup,
 	 * the strong pullup will be automatically deactivated
-	 * by the master, so do not explicitly deactive it
+	 * by the master, so do not explicitly deactivate it
 	 */
 	if (delay) {
 		/* both waits are crucial, otherwise devices might not be
diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index aa1f57f74397..3a0740d91a53 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -562,7 +562,7 @@ static int ds_write_bit(struct ds_device *dev, u8 bit)
 
 	/* Set COMM_ICP to write without a readback.  Note, this will
 	 * produce one time slot, a down followed by an up with COMM_D
-	 * only determing the timing.
+	 * only determining the timing.
 	 */
 	err = ds_send_control(dev, COMM_BIT_IO | COMM_IM | COMM_ICP |
 		(bit ? COMM_D : 0), 0);
@@ -691,7 +691,7 @@ static void ds9490r_search(void *data, struct w1_master *master,
 	 * If the number of devices found is less than or equal to the
 	 * search_limit, that number of IDs will be returned.  If there are
 	 * more, search_limit IDs will be returned followed by a non-zero
-	 * discrepency value.
+	 * discrepancy value.
 	 */
 	struct ds_device *dev = data;
 	int err;
diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
index d13db3396570..0982efa2b25b 100644
--- a/drivers/w1/masters/omap_hdq.c
+++ b/drivers/w1/masters/omap_hdq.c
@@ -282,7 +282,7 @@ static int omap_hdq_break(struct hdq_data *hdq_data)
 	}
 
 	/*
-	 * wait for both INIT and GO bits rerurn to zero.
+	 * wait for both INIT and GO bits return to zero.
 	 * zero wait time expected for interrupt mode.
 	 */
 	ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_CTRL_STATUS,
diff --git a/drivers/w1/slaves/w1_ds28e17.c b/drivers/w1/slaves/w1_ds28e17.c
index e53bc41bde3c..d656233afe54 100644
--- a/drivers/w1/slaves/w1_ds28e17.c
+++ b/drivers/w1/slaves/w1_ds28e17.c
@@ -101,7 +101,7 @@ static int w1_f19_i2c_busy_wait(struct w1_slave *sl, size_t count)
 		timebases[data->speed] * (data->stretch) * count
 		+ W1_F19_BUSY_GRATUITY);
 
-	/* Now continusly check the busy flag sent by the DS28E17. */
+	/* Now continuously check the busy flag sent by the DS28E17. */
 	checks = W1_F19_BUSY_CHECKS;
 	while ((checks--) > 0) {
 		/* Return success if the busy flag is cleared. */
diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index d96b224e2215..02acf84ffec5 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -355,7 +355,7 @@ static DEVICE_ATTR_RW(alarms);
 static DEVICE_ATTR_RW(conv_time);
 static DEVICE_ATTR_RW(features);
 
-static DEVICE_ATTR_RW(therm_bulk_read); /* attribut at master level */
+static DEVICE_ATTR_RW(therm_bulk_read); /* attribute at master level */
 
 /* Interface Functions declaration */
 
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 486f321eadc8..67ea14a06319 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -1177,7 +1177,7 @@ int w1_process(void *data)
 
 		__set_current_state(TASK_INTERRUPTIBLE);
 
-		/* hold list_mutex until after interruptible to prevent loosing
+		/* hold list_mutex until after interruptible to prevent losing
 		 * the wakeup signal when async_cmd is added.
 		 */
 		mutex_unlock(&dev->list_mutex);
diff --git a/drivers/w1/w1_family.c b/drivers/w1/w1_family.c
index 97da4f156e9a..8152a43c8c76 100644
--- a/drivers/w1/w1_family.c
+++ b/drivers/w1/w1_family.c
@@ -67,7 +67,7 @@ void w1_unregister_family(struct w1_family *fent)
 	}
 	spin_unlock(&w1_flock);
 
-	/* deatch devices using this family code */
+	/* detach devices using this family code */
 	w1_reconnect_slaves(fent, 0);
 
 	while (atomic_read(&fent->refcnt)) {
diff --git a/drivers/w1/w1_netlink.h b/drivers/w1/w1_netlink.h
index 449680a61569..1818a06740eb 100644
--- a/drivers/w1/w1_netlink.h
+++ b/drivers/w1/w1_netlink.h
@@ -14,7 +14,7 @@
 /**
  * enum w1_cn_msg_flags - bitfield flags for struct cn_msg.flags
  *
- * @W1_CN_BUNDLE: Request bundling replies into fewer messagse.  Be prepared
+ * @W1_CN_BUNDLE: Request bundling replies into fewer messages.  Be prepared
  * to handle multiple struct cn_msg, struct w1_netlink_msg, and
  * struct w1_netlink_cmd in one packet.
  */
-- 
2.55.0

Re: [PATCH] w1: fix spelling mistakes in comments across the subsystem

[Ivy Lopez]

Apologies! This was sent to you by mistake due to a stale patch
file left in my outgoing queue. It's unrelated to the ds2760 series
and already correctly merged via a separate thread with Krzysztof
Kozlowski. Please disregard!

ivy