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

[Ivan Vecera <ivecera@redhat.com>]

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