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

[Marek Vasut <marek.vasut@mailbox.org>]

On 1/20/25 10:17 AM, Rasmus Villemoes wrote:
> 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.

True

> And if the caller doesn't
> know whether the cyclic_info is already registered or not, he can't do a
> memset() of it.

This is what can happen right now, which is dangerous and what this 
series attempts to address.

> 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.

That I can do, but it will be a bit slower.

> 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.

This would be terribly overloaded interface, no, let's not do that.

Better introduce a dedicated function for that kind of period adjustment.

> 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;

I don't think this works, because that struct cyclic_info contains 
.next_call member, which is updated over time, so this exact match would 
not work as-is. I have something like this now:

diff --git a/common/cyclic.c b/common/cyclic.c
index 807a3d73f67..d721a21a575 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -27,11 +27,29 @@ struct hlist_head *cyclic_get_list(void)
  	return (struct hlist_head *)&gd->cyclic_list;
  }

+static int cyclic_already_registered(struct cyclic_info *cyclic)
+{
+	struct cyclic_info *cycliclst;
+	struct hlist_node *tmp;
+
+	/* Reassignment of function would corrupt cyclic list, exit */
+	hlist_for_each_entry_safe(cycliclst, tmp, cyclic_get_list(), list) {
+		if (cycliclst->func == cyclic->func &&
+		    cycliclst->name == cyclic->name && // or strcmp() ?
+		    cycliclst->delay_us == cyclic->delay_us &&
+		    cycliclst->start_time_us == cyclic->start_time_us)
+			return -EALREADY;	/* Match found */
+	}
+
+	/* Match not found */
+	return 0;
+}
+
  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)
+	if (!cyclic_already_registered(cyclic))
  		return -EALREADY;

  	memset(cyclic, 0, sizeof(*cyclic));
@@ -49,7 +67,7 @@ int cyclic_register(struct cyclic_info *cyclic, 
cyclic_func_t func,
  void cyclic_unregister(struct cyclic_info *cyclic)
  {
  	/* Unregister only initialized struct cyclic_info */
-	if (cyclic->func)
+	if (cyclic_already_registered(cyclic))
  		hlist_del(&cyclic->list);
  }

[...]

>>   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.

I would expect the caller should clear the structure before attempting 
to register it again. Shall we actually memset() the structure in 
cyclic_unregister() too ?

> 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).