[PATCH net] dpll: reject a reference sync pin which is not on the pin's dpll

[PATCH net] dpll: reject a reference sync pin which is not on the pin's dpll

[Jakub Kicinski]

dpll_pin_ref_sync_state_set() resolves the partner's driver private data
with dpll_pin_on_dpll_priv() and passes the result to ref_sync_get() and
ref_sync_set() without looking at it. The helper returns NULL when the
partner holds no ref on that dpll. Of the two drivers implementing the
feature only zl3073x dereferences the pointer (sync_pin->id); ice ignores
it, so ice cannot fault here.

The NULL is a teardown race, not a steady state - zl3073x registers every
input pin with every channel, so the partner is normally present on the
dpll the base pin resolves to. zl3073x_dev_stop() unregisters pins one at
a time, taking and dropping dpll_lock for each, and between the partner's
turn and the base pin's the partner is out of that dpll's pin_refs while
still registered with the channels not yet torn down, so
dpll_pin_available() keeps passing. That path is not only driver removal:
devlink reload and devlink dev flash both run zl3073x_dev_stop().

Reproduced by holding that state open with a mock dpll device, which is
where the frame name comes from:

 BUG: kernel NULL pointer dereference, address: 0000000000000000
 Oops: Oops: 0000 [#1] SMP NOPTI
 RIP: 0010:mock_ref_sync_get+0x5/0x30
 Call Trace:
  <TASK>
  dpll_pin_ref_sync_set+0x19f/0x4a0
  dpll_nl_pin_set_doit+0x17d/0x840
  genl_family_rcv_msg_doit+0xd6/0x130
  genl_rcv_msg+0x181/0x2b0
  netlink_rcv_skb+0x55/0x100
  genl_rcv+0x23/0x30
  netlink_unicast+0x24d/0x370
  netlink_sendmsg+0x1e2/0x420
  __sys_sendto+0x1db/0x1f0
  __x64_sys_sendto+0x1f/0x30
  do_syscall_64+0xe1/0x490

Commit d2e914a4a0d0 ("dpll: fix NULL pointer dereference in
dpll_msg_add_pin_ref_sync()") added the same guard to the read side, which
the kernel walks into by itself because the delete notification is emitted
from inside the unregister; the write side needs a pin-set to land in the
window and was left alone. Test the priv rather than look up pin_refs
directly, so that the two halves key off the same condition.

Fixes: 58256a26bfb3 ("dpll: add reference sync get/set")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: vadim.fedorenko@linux.dev
CC: arkadiusz.kubalewski@intel.com
CC: jiri@resnulli.us
CC: ivecera@redhat.com
CC: milena.olech@intel.com
CC: przemyslaw.kitszel@intel.com
---
 drivers/dpll/dpll_netlink.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index d4f133ac9fea..9a90733b2da3 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -1201,6 +1201,7 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin, u32 ref_sync_pin_id,
 			    const enum dpll_pin_state state,
 			    struct netlink_ext_ack *extack)
 {
+	void *pin_priv, *ref_sync_pin_priv;
 	const struct dpll_pin_ops *ops;
 	enum dpll_pin_state old_state;
 	struct dpll_pin *ref_sync_pin;
@@ -1228,9 +1229,15 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin, u32 ref_sync_pin_id,
 		return -EOPNOTSUPP;
 	}
 	dpll = ref->dpll;
-	ret = ops->ref_sync_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
-				ref_sync_pin,
-				dpll_pin_on_dpll_priv(dpll, ref_sync_pin),
+	pin_priv = dpll_pin_on_dpll_priv(dpll, pin);
+	ref_sync_pin_priv = dpll_pin_on_dpll_priv(dpll, ref_sync_pin);
+	/* Pin may have been unregistered from this dpll already */
+	if (!ref_sync_pin_priv) {
+		NL_SET_ERR_MSG(extack,
+			       "reference sync pin not registered with the dpll");
+		return -ENODEV;
+	}
+	ret = ops->ref_sync_get(pin, pin_priv, ref_sync_pin, ref_sync_pin_priv,
 				&old_state, extack);
 	if (ret) {
 		NL_SET_ERR_MSG(extack, "unable to get old reference sync state");
@@ -1239,9 +1246,7 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin, u32 ref_sync_pin_id,
 	if (state == old_state)
 		return 0;
 
-	ret = ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-				ref_sync_pin,
-				dpll_pin_on_dpll_priv(dpll, ref_sync_pin),
+	ret = ops->ref_sync_set(pin, pin_priv, ref_sync_pin, ref_sync_pin_priv,
 				state, extack);
 	if (ret) {
 		NL_SET_ERR_MSG_FMT(extack,
-- 
2.55.0

Re: [PATCH net] dpll: reject a reference sync pin which is not on the pin's dpll

[Jakub Kicinski]

On Tue, 15 Sep 2026 14:30:47 -0700 Jakub Kicinski wrote:
> CC: vadim.fedorenko@linux.dev
> CC: arkadiusz.kubalewski@intel.com
> CC: jiri@resnulli.us
> CC: ivecera@redhat.com

How many maintainers does it take to get one ack? :/

Re: [PATCH net] dpll: reject a reference sync pin which is not on the pin's dpll

[Ivan Vecera]

On 9/17/26 1:37 AM, Jakub Kicinski wrote:
> On Tue, 15 Sep 2026 14:30:47 -0700 Jakub Kicinski wrote:
>> CC: vadim.fedorenko@linux.dev
>> CC: arkadiusz.kubalewski@intel.com
>> CC: jiri@resnulli.us
>> CC: ivecera@redhat.com
> 
> How many maintainers does it take to get one ack? :/

Sorry, Jakub; I've been a bit busy these past few days since returning
from vacation.

Reviewed few minutes ago.

Thanks,
Ivan

Re: [PATCH net] dpll: reject a reference sync pin which is not on the pin's dpll

[Ivan Vecera]

On 9/15/26 11:30 PM, Jakub Kicinski wrote:
> dpll_pin_ref_sync_state_set() resolves the partner's driver private data
> with dpll_pin_on_dpll_priv() and passes the result to ref_sync_get() and
> ref_sync_set() without looking at it. The helper returns NULL when the
> partner holds no ref on that dpll. Of the two drivers implementing the
> feature only zl3073x dereferences the pointer (sync_pin->id); ice ignores
> it, so ice cannot fault here.
> 
> The NULL is a teardown race, not a steady state - zl3073x registers every
> input pin with every channel, so the partner is normally present on the
> dpll the base pin resolves to. zl3073x_dev_stop() unregisters pins one at
> a time, taking and dropping dpll_lock for each, and between the partner's
> turn and the base pin's the partner is out of that dpll's pin_refs while
> still registered with the channels not yet torn down, so
> dpll_pin_available() keeps passing. That path is not only driver removal:
> devlink reload and devlink dev flash both run zl3073x_dev_stop().
> 
> Reproduced by holding that state open with a mock dpll device, which is
> where the frame name comes from:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000000
>   Oops: Oops: 0000 [#1] SMP NOPTI
>   RIP: 0010:mock_ref_sync_get+0x5/0x30
>   Call Trace:
>    <TASK>
>    dpll_pin_ref_sync_set+0x19f/0x4a0
>    dpll_nl_pin_set_doit+0x17d/0x840
>    genl_family_rcv_msg_doit+0xd6/0x130
>    genl_rcv_msg+0x181/0x2b0
>    netlink_rcv_skb+0x55/0x100
>    genl_rcv+0x23/0x30
>    netlink_unicast+0x24d/0x370
>    netlink_sendmsg+0x1e2/0x420
>    __sys_sendto+0x1db/0x1f0
>    __x64_sys_sendto+0x1f/0x30
>    do_syscall_64+0xe1/0x490
> 
> Commit d2e914a4a0d0 ("dpll: fix NULL pointer dereference in
> dpll_msg_add_pin_ref_sync()") added the same guard to the read side, which
> the kernel walks into by itself because the delete notification is emitted
> from inside the unregister; the write side needs a pin-set to land in the
> window and was left alone. Test the priv rather than look up pin_refs
> directly, so that the two halves key off the same condition.
> 
> Fixes: 58256a26bfb3 ("dpll: add reference sync get/set")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: vadim.fedorenko@linux.dev
> CC: arkadiusz.kubalewski@intel.com
> CC: jiri@resnulli.us
> CC: ivecera@redhat.com
> CC: milena.olech@intel.com
> CC: przemyslaw.kitszel@intel.com

Reviewed-by: Ivan Vecera <ivecera@redhat.com>

Re: [PATCH net] dpll: reject a reference sync pin which is not on the pin's dpll

[Paolo Abeni]

On 9/15/26 23:30, Jakub Kicinski wrote:
> dpll_pin_ref_sync_state_set() resolves the partner's driver private data
> with dpll_pin_on_dpll_priv() and passes the result to ref_sync_get() and
> ref_sync_set() without looking at it. The helper returns NULL when the
> partner holds no ref on that dpll. Of the two drivers implementing the
> feature only zl3073x dereferences the pointer (sync_pin->id); ice ignores
> it, so ice cannot fault here.
> 
> The NULL is a teardown race, not a steady state - zl3073x registers every
> input pin with every channel, so the partner is normally present on the
> dpll the base pin resolves to. zl3073x_dev_stop() unregisters pins one at
> a time, taking and dropping dpll_lock for each, and between the partner's
> turn and the base pin's the partner is out of that dpll's pin_refs while
> still registered with the channels not yet torn down, so
> dpll_pin_available() keeps passing. That path is not only driver removal:
> devlink reload and devlink dev flash both run zl3073x_dev_stop().
> 
> Reproduced by holding that state open with a mock dpll device, which is
> where the frame name comes from:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000000
>   Oops: Oops: 0000 [#1] SMP NOPTI
>   RIP: 0010:mock_ref_sync_get+0x5/0x30
>   Call Trace:
>    <TASK>
>    dpll_pin_ref_sync_set+0x19f/0x4a0
>    dpll_nl_pin_set_doit+0x17d/0x840
>    genl_family_rcv_msg_doit+0xd6/0x130
>    genl_rcv_msg+0x181/0x2b0
>    netlink_rcv_skb+0x55/0x100
>    genl_rcv+0x23/0x30
>    netlink_unicast+0x24d/0x370
>    netlink_sendmsg+0x1e2/0x420
>    __sys_sendto+0x1db/0x1f0
>    __x64_sys_sendto+0x1f/0x30
>    do_syscall_64+0xe1/0x490
> 
> Commit d2e914a4a0d0 ("dpll: fix NULL pointer dereference in
> dpll_msg_add_pin_ref_sync()") added the same guard to the read side, which
> the kernel walks into by itself because the delete notification is emitted
> from inside the unregister; the write side needs a pin-set to land in the
> window and was left alone. Test the priv rather than look up pin_refs
> directly, so that the two halves key off the same condition.
> 
> Fixes: 58256a26bfb3 ("dpll: add reference sync get/set")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Note that sashiko suggests more follow-up:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915213047.1352286-1-kuba@kernel.org

/P

Re: [PATCH net] dpll: reject a reference sync pin which is not on the pin's dpll

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 15 Sep 2026 14:30:47 -0700 you wrote:
> dpll_pin_ref_sync_state_set() resolves the partner's driver private data
> with dpll_pin_on_dpll_priv() and passes the result to ref_sync_get() and
> ref_sync_set() without looking at it. The helper returns NULL when the
> partner holds no ref on that dpll. Of the two drivers implementing the
> feature only zl3073x dereferences the pointer (sync_pin->id); ice ignores
> it, so ice cannot fault here.
> 
> [...]

Here is the summary with links:
  - [net] dpll: reject a reference sync pin which is not on the pin's dpll
    https://git.kernel.org/netdev/net/c/d798162eb364

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html