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:
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
---[ 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
---
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