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

[Marek Vasut]

On 1/22/25 10:25 AM, Stefan Roese wrote:
> Hi Marek,
> Hi Rasmus,
> 
> On 20.01.25 10:17, Rasmus Villemoes wrote:
>> On Sat, Jan 18 2025, Marek Vasut <marek.vasut+renesas at 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).
> 
> I like the approach suggest by Rasmus. Marek, do you see any flaws using
> this version? If not, please send an updated version.
I do see a couple of issues, please see my reply.