All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net v2] dpll: fix NULL deref in dpll_device_ops() during teardown race
@ 2026-08-13 14:08 Petr Oros
  2026-08-13 15:36 ` Ivan Vecera
  2026-08-14 22:35 ` Vadim Fedorenko
  0 siblings, 2 replies; 3+ messages in thread
From: Petr Oros @ 2026-08-13 14:08 UTC (permalink / raw)
  To: netdev
  Cc: Petr Oros, Vadim Fedorenko, Arkadiusz Kubalewski, Jiri Pirko,
	Michal Michalik, Milena Olech, linux-kernel, ivecera

When the last owner of a dpll device unregisters while a foreign driver
still holds a pin on it via dpll_pin_on_pin_register(), the dpll object
stays alive with an empty registration list. A pin notification queued
before the unregister (e.g. ice reacting to zl3073x_i2c removal) then
walks pin->dpll_refs into dpll_device_ops(), which trips the WARN_ON and
dereferences the missing registration. dpll_lock cannot help because the
notification work was queued before the unregistering driver took the
lock.

Treat the empty registration list as a legitimate transient state. Make
dpll_priv() and dpll_device_ops() return NULL in that case and make
every pin netlink path that resolves a device from a pin skip such
dplls. dpll_cmd_pin_get_one() picks a ref with a live registration and
returns -ENODEV when there is none, the pin dumpit skips such a pin
instead of aborting the dump, dpll_msg_add_pin_dplls() and the
frequency, esync, reference sync and phase adjust set paths skip dead
refs, and dpll_pin_parent_device_set() validates the parent with
dpll_device_get_by_id(). dpll_pin_register() is the last caller that
dereferenced the device ops without a check, so move its frequency
monitor validation under dpll_lock and tolerate a missing registration
there as well.

The empty registration list is equivalent to a cleared DPLL_REGISTERED
mark, both transitions happen under dpll_lock in dpll_device_register()
and dpll_device_unregister(). A pin notification for a pin whose dplls
are all gone is now dropped with -ENODEV instead of crashing, all
callers in the core ignore that return value.

 WARNING: drivers/dpll/dpll_core.c:1092 at dpll_device_ops+0x24/0x40,
 CPU#83: kworker/u576:3/23471
 Modules linked in: ... ice ... zl3073x_i2c(-) ... zl3073x ...
 Workqueue: ice_dpll_wq ice_dpll_pin_notify_work [ice]
 RIP: 0010:dpll_device_ops+0x24/0x40
 Call Trace:
  <TASK>
  dpll_cmd_pin_get_one+0x336/0x520
  dpll_pin_event_send+0x82/0x140
  dpll_pin_on_pin_unregister+0xbb/0x160
  ice_dpll_pin_notify_work+0x1bc/0x1f0 [ice]
  process_one_work+0x19e/0x370
  worker_thread+0x1a6/0x310
  kthread+0xe4/0x120
  ret_from_fork+0x1a1/0x270
  ret_from_fork_asm+0x1a/0x30
  </TASK>
 ---[ end trace 0000000000000000 ]---
 BUG: kernel NULL pointer dereference, address: 0000000000000010
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page

Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
Signed-off-by: Petr Oros <poros@redhat.com>
---
v2:
- guard every path that resolves a device from a pin, not only the
  first ref in dpll_cmd_pin_get_one(); skip half-dead refs in
  dpll_msg_add_pin_dplls() and the set paths, select a live
  representative ref and turn the pin dumpit -ENODEV into a per pin
  skip (Jakub)
- validate the parent device in dpll_pin_parent_device_set() via
  dpll_device_get_by_id()
- guard the frequency monitor validation in dpll_pin_register() and
  perform it under dpll_lock, it was the only remaining unchecked
  dereference of the device ops
- drop patch 2/2, superseded by commit 32239d600236 ("dpll: fix stale
  iteration in dpll_pin_on_pin_unregister()")

v1: https://lore.kernel.org/all/20260516191317.1005612-2-poros@redhat.com/
---
 drivers/dpll/dpll_core.c    | 24 +++++++++------
 drivers/dpll/dpll_netlink.c | 59 ++++++++++++++++++++++++++++++++-----
 2 files changed, 67 insertions(+), 16 deletions(-)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 43d51d942eadd3..a320eeb829ad28 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -876,19 +876,25 @@ int
 dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
 		  const struct dpll_pin_ops *ops, void *priv)
 {
+	const struct dpll_device_ops *dev_ops;
 	int ret;
 
 	if (WARN_ON(!ops) ||
 	    WARN_ON(!ops->state_on_dpll_get) ||
 	    WARN_ON(!ops->direction_get) ||
-	    WARN_ON(ops->measured_freq_get &&
-		    (!dpll_device_ops(dpll)->freq_monitor_get ||
-		     !dpll_device_ops(dpll)->freq_monitor_set)) ||
 	    WARN_ON(ops->supported_ffo && !ops->ffo_get))
 		return -EINVAL;
 
 	mutex_lock(&dpll_lock);
 
+	dev_ops = dpll_device_ops(dpll);
+	if (WARN_ON(ops->measured_freq_get &&
+		    (!dev_ops || !dev_ops->freq_monitor_get ||
+		     !dev_ops->freq_monitor_set))) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	/*
 	 * For pins identified via firmware (pin->fwnode), allow registration
 	 * even if the pin's (module, clock_id) differs from the target DPLL.
@@ -1081,12 +1087,8 @@ EXPORT_SYMBOL_GPL(dpll_pin_ref_sync_pair_add);
 static struct dpll_device_registration *
 dpll_device_registration_first(struct dpll_device *dpll)
 {
-	struct dpll_device_registration *reg;
-
-	reg = list_first_entry_or_null((struct list_head *)&dpll->registration_list,
-				       struct dpll_device_registration, list);
-	WARN_ON(!reg);
-	return reg;
+	return list_first_entry_or_null((struct list_head *)&dpll->registration_list,
+					struct dpll_device_registration, list);
 }
 
 void *dpll_priv(struct dpll_device *dpll)
@@ -1094,6 +1096,8 @@ void *dpll_priv(struct dpll_device *dpll)
 	struct dpll_device_registration *reg;
 
 	reg = dpll_device_registration_first(dpll);
+	if (!reg)
+		return NULL;
 	return reg->priv;
 }
 
@@ -1102,6 +1106,8 @@ const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll)
 	struct dpll_device_registration *reg;
 
 	reg = dpll_device_registration_first(dpll);
+	if (!reg)
+		return NULL;
 	return reg->ops;
 }
 
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index afb31c0040382c..9e55745e33e4fa 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -66,6 +66,22 @@ static bool dpll_pin_available(struct dpll_pin *pin)
 	return false;
 }
 
+static bool dpll_device_registered(struct dpll_device *dpll)
+{
+	return dpll_device_ops(dpll);
+}
+
+static struct dpll_pin_ref *dpll_pin_first_registered_ref(struct dpll_pin *pin)
+{
+	struct dpll_pin_ref *ref;
+	unsigned long i;
+
+	xa_for_each(&pin->dpll_refs, i, ref)
+		if (dpll_device_registered(ref->dpll))
+			return ref;
+	return NULL;
+}
+
 /**
  * dpll_msg_add_pin_handle - attach pin handle attribute to a given message
  * @msg: pointer to sk_buff message to attach a pin handle
@@ -656,6 +672,8 @@ dpll_msg_add_pin_dplls(struct sk_buff *msg, struct dpll_pin *pin,
 	int ret;
 
 	xa_for_each(&pin->dpll_refs, index, ref) {
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		attr = nla_nest_start(msg, DPLL_A_PIN_PARENT_DEVICE);
 		if (!attr)
 			return -EMSGSIZE;
@@ -700,9 +718,10 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
 	int ret;
 
 	ref = dpll_pin_own_dpll_ref_first(pin);
+	if (!ref || !dpll_device_registered(ref->dpll))
+		ref = dpll_pin_first_registered_ref(pin);
 	if (!ref)
-		ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
-	ASSERT_NOT_NULL(ref);
+		return -ENODEV;
 
 	ret = dpll_msg_add_pin_handle(msg, pin);
 	if (ret)
@@ -1091,6 +1110,8 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 	}
 
 	xa_for_each(&pin->dpll_refs, i, ref) {
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if ((!ops->frequency_set || !ops->frequency_get) &&
 		    ref->dpll->module == pin->module &&
@@ -1101,7 +1122,7 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 		}
 	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
-	if (!ref) {
+	if (!ref || !dpll_device_registered(ref->dpll)) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
@@ -1117,6 +1138,8 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 		return 0;
 
 	xa_for_each(&pin->dpll_refs, i, ref) {
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if (!ops->frequency_set)
 			continue;
@@ -1138,6 +1161,8 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 	xa_for_each(&pin->dpll_refs, i, ref) {
 		if (ref == failed)
 			break;
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if (!ops->frequency_set)
 			continue;
@@ -1163,6 +1188,8 @@ dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a,
 	int ret;
 
 	xa_for_each(&pin->dpll_refs, i, ref) {
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if ((!ops->esync_set || !ops->esync_get) &&
 		    ref->dpll->module == pin->module &&
@@ -1173,7 +1200,7 @@ dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a,
 		}
 	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
-	if (!ref) {
+	if (!ref || !dpll_device_registered(ref->dpll)) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
@@ -1199,6 +1226,8 @@ dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a,
 	xa_for_each(&pin->dpll_refs, i, ref) {
 		void *pin_dpll_priv;
 
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if (!ops->esync_set)
 			continue;
@@ -1224,6 +1253,8 @@ dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a,
 
 		if (ref == failed)
 			break;
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if (!ops->esync_set)
 			continue;
@@ -1262,7 +1293,7 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin,
 		return -EINVAL;
 	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
-	if (!ref) {
+	if (!ref || !dpll_device_registered(ref->dpll)) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
@@ -1283,6 +1314,8 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin,
 	if (state == old_state)
 		return 0;
 	xa_for_each(&pin->dpll_refs, i, ref) {
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if (!ops->ref_sync_set)
 			continue;
@@ -1307,6 +1340,8 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin,
 	xa_for_each(&pin->dpll_refs, i, ref) {
 		if (ref == failed)
 			break;
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if (!ops->ref_sync_set)
 			continue;
@@ -1500,6 +1535,8 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 	}
 
 	xa_for_each(&pin->dpll_refs, i, ref) {
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if ((!ops->phase_adjust_set || !ops->phase_adjust_get) &&
 		    ref->dpll->module == pin->module &&
@@ -1509,7 +1546,7 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 		}
 	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
-	if (!ref) {
+	if (!ref || !dpll_device_registered(ref->dpll)) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
@@ -1526,6 +1563,8 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 		return 0;
 
 	xa_for_each(&pin->dpll_refs, i, ref) {
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if (!ops->phase_adjust_set)
 			continue;
@@ -1550,6 +1589,8 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 	xa_for_each(&pin->dpll_refs, i, ref) {
 		if (ref == failed)
 			break;
+		if (!dpll_device_registered(ref->dpll))
+			continue;
 		ops = dpll_pin_ops(ref);
 		if (!ops->phase_adjust_set)
 			continue;
@@ -1581,7 +1622,7 @@ dpll_pin_parent_device_set(struct dpll_pin *pin, struct nlattr *parent_nest,
 		return -EINVAL;
 	}
 	pdpll_idx = nla_get_u32(tb[DPLL_A_PIN_PARENT_ID]);
-	dpll = xa_load(&dpll_device_xa, pdpll_idx);
+	dpll = dpll_device_get_by_id(pdpll_idx);
 	if (!dpll) {
 		NL_SET_ERR_MSG(extack, "parent device not found");
 		return -EINVAL;
@@ -1873,6 +1914,10 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 		ret = dpll_cmd_pin_get_one(skb, pin, cb->extack);
 		if (ret) {
 			genlmsg_cancel(skb, hdr);
+			if (ret == -ENODEV) {
+				ret = 0;
+				continue;
+			}
 			break;
 		}
 		genlmsg_end(skb, hdr);
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-08-14 22:36 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-13 14:08 [PATCH net v2] dpll: fix NULL deref in dpll_device_ops() during teardown race Petr Oros
2026-08-13 15:36 ` Ivan Vecera
2026-08-14 22:35 ` Vadim Fedorenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.