Re: [PATCH net-next 1/6] dpll: zl3073x: notify sibling pin when shared output config changes

[Ivan Vecera]

On 9/30/26 9:49 AM, Ivan Vecera wrote:
> > [Severity: High]
> > Can sibling->dpll_pin be NULL, or the sibling already freed, by the time
> > it is dereferenced here?
> >
> > zl3073x_dpll_output_pin_sibling_get() returns any output pin on
> > zldpll->pins with the matching id. It does not check that
> > sibling->dpll_pin is still set. The lookup runs under zldpll->lock, but
> > the dereference happens after mutex_unlock().
> >
> > The teardown path does not take zldpll->lock:
> >
> > drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_pins_unregister() {
> >     list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
> >         zl3073x_dpll_pin_unregister(pin);
> >         list_del(&pin->list);
> >         zl3073x_dpll_pin_free(pin);
> >     }
> > }
> >
> > zl3073x_dpll_pin_unregister() ends with:
> >
> >     dpll_pin_put(pin->dpll_pin, &pin->tracker);
> >     pin->dpll_pin = NULL;
> >
> > and dpll_pin_put() has already dropped dpll_lock at that point.
> >
> > zl3073x_dpll_pins_register() uses list_add(), so the N pin (2k+1) sits
> > before its P pin (2k) on the list. That means N is torn down while P is
> > still registered and reachable over netlink.
> >
> > One possible interleaving:
> >
> > CPU1 (devlink reload, flash_prepare or unbind)
> > zl3073x_dev_stop()
> >    zl3073x_dpll_unregister()
> >      zl3073x_dpll_pins_unregister()
> >        dpll_pin_put(N)          /* dpll_lock released */
> >
> > CPU0 (DPLL_CMD_PIN_SET on P, dpll_lock held)
> > dpll_pin_freq_set()
> >    zl3073x_dpll_output_pin_frequency_set(P)
> >      mutex_lock(&zldpll->lock)
> >      sibling = zl3073x_dpll_output_pin_sibling_get(P)  /* finds N */
> >      mutex_unlock(&zldpll->lock)
> >
> > CPU1
> >        N->dpll_pin = NULL
> >        list_del(&N->list)
> >        zl3073x_dpll_pin_free(N)
> >
> > CPU0
> >      __dpll_pin_change_ntf(sibling->dpll_pin)
> >
> > In that case, would __dpll_pin_change_ntf() get NULL and oops on
> > pin->id and pin->clock_id in dpll_pin_notify()? Or would CPU0 read the
> > freed zl3073x_dpll_pin?
> >
> > The list walk in zl3073x_dpll_output_pin_sibling_get() can also run
> > while list_del() is in progress, because the writer never takes
> > zldpll->lock.
> >
> > zl3073x_dpll_output_pin_frequency_set() and
> > zl3073x_dpll_output_pin_phase_adjust_set() have the same pattern.
> >
> > The later patch "dpll: zl3073x: add PTP periodic output support" also
> > calls zl3073x_dpll_output_pin_sibling_get() from
> > zl3073x_dpll_ptp_enable() without holding zldpll->lock at all.
>
> This cannot happen; the notification is already serialized against pin
> teardown.
>
> The output setters (esync_set / frequency_set / phase_adjust_set) run
> with dpll_lock held: the dpll core invokes the pin ops under dpll_lock,
> and they use __dpll_pin_change_ntf(), whose contract is exactly "caller
> must hold dpll_lock" (dpll_netlink.c: lockdep_assert_held(&dpll_lock),
> "suitable for use inside pin callbacks which are already invoked under
> dpll_lock"). So sibling_get() and the __dpll_pin_change_ntf() call - even
> though it runs after mutex_unlock(&zldpll->lock) - are all covered by
> dpll_lock.
>
> Pin teardown frees the sibling via zl3073x_dpll_pin_unregister() ->
> dpll_pin_unregister(), which takes dpll_lock. While a setter holds
> dpll_lock for the whole callback, teardown cannot even reach
> dpll_pin_unregister(N), let alone the following list_del()/kfree(). The
> proposed interleaving (CPU1 freeing N while CPU0 notifies) is therefore
> impossible: CPU0 holds dpll_lock throughout.
>
> The PTP enable() path (zl3073x_dpll_ptp_enable(), added in patch 6) does
> call sibling_get() outside dpll_lock, but it is serialized differently:
> zl3073x_dpll_unregister() unregisters the PTP clock *before* the pins
> (zl3073x_dpll_ptp_unregister() then zl3073x_dpll_pins_unregister()), and
> ptp_clock_unregister() quiesces in-flight enable() callbacks. No
> enable() can run while the pins are being freed.

More thinking about it... and yes this can happen :-(
The pin removal from the list must be performed prior its unregistration
and also the list management has to be protected by zldpll->lock.

Will fix and send as bugfix to net branch with proper Fixes: tags.

Thanks,
Ivan