Re: [PATCH net 2/2] dpll: report dumps interrupted by topology changes

[Petr Oros <poros@redhat.com>]

On 11/5/26 2:11 PM, Ivan Vecera wrote:
> Pin and device dumps release dpll_lock between parts and resume from the
> id saved by the previous part. An object registered in between is missed
> if its id is below that one, as is the case for an object allocated
> before the dump started. User space gets an incomplete dump and has no
> way to tell.
> 
> Count registrations and unregistrations in a generation counter and pass
> it to genl_dump_check_consistent() to set NLM_F_DUMP_INTR. Check it only
> after a message is built, a cancelled message would consume the change.
> 
> Found by code review. Reproduced with a test module registering a device
> or a pin with a low id between parts of a raw netlink dump: the object was
> missing and no message had the flag set. With this patch the flag is set
> when an object is registered or unregistered during a dump, and is not
> set on dumps without such a change. Tested in QEMU and on a ZL30732A
> board, where unbinding the driver during a dump sets the flag as well.
> 
> Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/dpll/dpll_core.c    | 19 +++++++++++++++++++
>   drivers/dpll/dpll_core.h    |  1 +
>   drivers/dpll/dpll_netlink.c |  4 ++++
>   3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
> index 277692d12d3d1e..a68e8f5b609db8 100644
> --- a/drivers/dpll/dpll_core.c
> +++ b/drivers/dpll/dpll_core.c
> @@ -22,6 +22,12 @@
>   /* Mutex lock to protect DPLL subsystem devices and pins */
>   DEFINE_MUTEX(dpll_lock);
>   
> +/* Changes with the topology of devices and pins, so that netlink dumps can
> + * detect it. Never 0, as netlink takes that for no sequence. Protected by
> + * dpll_lock.
> + */
> +unsigned int dpll_generation = 1;
> +
>   DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC);
>   DEFINE_XARRAY_FLAGS(dpll_pin_xa, XA_FLAGS_ALLOC);
>   
> @@ -53,6 +59,13 @@ struct dpll_pin_registration {
>   	dpll_tracker tracker;
>   };
>   
> +static void dpll_generation_inc(void)
> +{
> +	lockdep_assert_held(&dpll_lock);
> +	while (++dpll_generation == 0)
> +		;
> +}
> +
>   static int call_dpll_notifiers(unsigned long action, void *info)
>   {
>   	lockdep_assert_held(&dpll_lock);
> @@ -510,6 +523,7 @@ int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>   	}
>   
>   	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
> +	dpll_generation_inc();
>   	dpll_device_create_ntf(dpll);
>   	mutex_unlock(&dpll_lock);
>   
> @@ -549,6 +563,7 @@ void dpll_device_unregister(struct dpll_device *dpll,
>   		return;
>   	}
>   	xa_clear_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
> +	dpll_generation_inc();
>   	mutex_unlock(&dpll_lock);
>   }
>   EXPORT_SYMBOL_GPL(dpll_device_unregister);
> @@ -851,6 +866,7 @@ __dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
>   	if (ret)
>   		goto ref_pin_del;
>   	xa_set_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED);
> +	dpll_generation_inc();
>   	dpll_pin_create_ntf(pin, dpll->clock_id);
>   
>   	return ret;
> @@ -941,6 +957,7 @@ __dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin,
>   	dpll_pin_delete_ntf(pin, dpll->clock_id);
>   	dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv, cookie);
>   	dpll_xa_ref_dpll_del(&pin->dpll_refs, dpll, ops, priv, cookie);
> +	dpll_generation_inc();
>   	if (xa_empty(&pin->dpll_refs)) {
>   		dpll_pin_ref_sync_pair_del(pin->id);
>   		xa_clear_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED);
> @@ -1005,6 +1022,7 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
>   	ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv, pin);
>   	if (ret)
>   		goto unlock;
> +	dpll_generation_inc();
>   	xa_for_each(&parent->dpll_refs, i, ref) {
>   		ret = __dpll_pin_register(ref->dpll, pin, ops, priv, parent);
>   		if (ret) {
> @@ -1057,6 +1075,7 @@ void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin *pin,
>   		__dpll_pin_unregister(ref->dpll, pin, ops, priv, parent);
>   	}
>   	dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv, pin);
> +	dpll_generation_inc();
>   	mutex_unlock(&dpll_lock);
>   }
>   EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister);
> diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
> index da8a369556ed11..c2ed0926de1b95 100644
> --- a/drivers/dpll/dpll_core.h
> +++ b/drivers/dpll/dpll_core.h
> @@ -99,6 +99,7 @@ struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs);
>   extern struct xarray dpll_device_xa;
>   extern struct xarray dpll_pin_xa;
>   extern struct mutex dpll_lock;
> +extern unsigned int dpll_generation;
>   
>   void dpll_device_notify(struct dpll_device *dpll, unsigned long action);
>   void dpll_pin_notify(struct dpll_pin *pin, u64 src_clock_id,
> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> index e5380d95f238b3..87c8caef0f574b 100644
> --- a/drivers/dpll/dpll_netlink.c
> +++ b/drivers/dpll/dpll_netlink.c
> @@ -1768,6 +1768,7 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>   	int ret = 0;
>   
>   	mutex_lock(&dpll_lock);
> +	cb->seq = dpll_generation;
>   	xa_for_each_marked_start(&dpll_pin_xa, i, pin, DPLL_REGISTERED,
>   				 ctx->idx) {
>   		if (!dpll_pin_available(pin))
> @@ -1789,6 +1790,7 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>   			}
>   			break;
>   		}
> +		genl_dump_check_consistent(cb, hdr);
>   		genlmsg_end(skb, hdr);
>   	}
>   	mutex_unlock(&dpll_lock);
> @@ -1990,6 +1992,7 @@ int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>   	int ret = 0;
>   
>   	mutex_lock(&dpll_lock);
> +	cb->seq = dpll_generation;
>   	xa_for_each_marked_start(&dpll_device_xa, i, dpll, DPLL_REGISTERED,
>   				 ctx->idx) {
>   		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> @@ -2004,6 +2007,7 @@ int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>   			genlmsg_cancel(skb, hdr);
>   			break;
>   		}
> +		genl_dump_check_consistent(cb, hdr);
>   		genlmsg_end(skb, hdr);
>   	}
>   	mutex_unlock(&dpll_lock);
Reviewed-by: Petr Oros <poros@redhat.com>