Re: [PATCH 1/8] cyclic: Prevent corruption of cyclic list on reassignment

[Rasmus Villemoes <ravi@prevas.dk>]

On Sat, Jan 18 2025, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:

> Make cyclic_register() return error code, 0 in case of success,
> -EALREADY in case the called attempts to re-register already
> registered struct cyclic_info. The re-registration would lead
> to corruption of gd->cyclic_list because the re-registration
> would memset() one of its nodes, prevent that. Unregister only
> initialized struct cyclic_info.

I had considered something like this, but I don't like it, because it
relies on the cyclic structure (or more likely whatever structure it is
embedded in) being initially zero-initialized. And if the caller doesn't
know whether the cyclic_info is already registered or not, he can't do a
memset() of it.

So my preference would be that we instead simply iterate the current
list to see if the struct cyclic_info is already registered that
way. Also, I think I'd prefer if double cyclic_register() is allowed and
always succeeds; this could be used to change the period of an already
registered instance, for example. Also, that avoids making the
interfaces fallible.

And cyclic_unregister() could similarly just check
whether the passed pointer is already on the list, and be a no-op in
case it's not. Those extra list traversals are not expensive (we're
traversing them thousands of times per second anyway in cyclic_run), and
I doubt one would ever has more than about 10 items on the list.

IOW, I'd suggest adding an internal

bool cyclic_is_registered(struct cyclic_info *info)
{
  struct cyclic_info *c;
  hlist_for_each(...) if (c == info) return true;
  return false;
}

add

  if (!cyclic_is_registered(c))
    return;

to cyclic_unregister(), and have cyclic_register() unconditionally start
by a

  cyclic_unregister(c);

and then proceed to initialize it as it does currently. No other
changes, apart from documentation, would be needed.
    


>  common/cyclic.c  | 14 +++++++++++---
>  include/cyclic.h |  9 ++++++---
>  2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/common/cyclic.c b/common/cyclic.c
> index 196797fd61e..53156a704cc 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -27,9 +27,13 @@ struct hlist_head *cyclic_get_list(void)
>  	return (struct hlist_head *)&gd->cyclic_list;
>  }
>  
> -void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
> -		     uint64_t delay_us, const char *name)
> +int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
> +		    uint64_t delay_us, const char *name)
>  {
> +	/* Reassignment of function would corrupt cyclic list, exit */
> +	if (cyclic->func)
> +		return -EALREADY;
> +
>  	memset(cyclic, 0, sizeof(*cyclic));
>  
>  	/* Store values in struct */
> @@ -38,11 +42,15 @@ void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>  	cyclic->delay_us = delay_us;
>  	cyclic->start_time_us = timer_get_us();
>  	hlist_add_head(&cyclic->list, cyclic_get_list());
> +
> +	return 0;
>  }
>  
>  void cyclic_unregister(struct cyclic_info *cyclic)
>  {
> -	hlist_del(&cyclic->list);
> +	/* Unregister only initialized struct cyclic_info */
> +	if (cyclic->func)
> +		hlist_del(&cyclic->list);
>  }

So this already shows how error prone this approach is. You are not
clearing cyclic->func, so if the caller subsequently tries to register
that struct again, he would get -EALREADY, while a subsequent unregister
could would lead to exactly the list corruption you want to avoid.

And unless the caller immediately himself clears ->func, other code in
the client cannot rely on ->func being NULL or not as a proxy for
whether the struct is already registered (and the caller shouldn't do
either of those things, as the struct cyclic_info should be considered
opaque).

Rasmus