[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <204874d5.b00e.196b5d5ea1d.Coremail.00107082@163.com>
Date: Sat, 10 May 2025 00:16:57 +0800 (CST)
From: "David Wang" <00107082@....com>
To: "Suren Baghdasaryan" <surenb@...gle.com>
Cc: kent.overstreet@...ux.dev, akpm@...ux-foundation.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] alloc_tag: add timestamp to codetag iterator
At 2025-05-10 00:10:01, "Suren Baghdasaryan" <surenb@...gle.com> wrote:
>On Thu, May 8, 2025 at 10:52 PM David Wang <00107082@....com> wrote:
>>
>> Codetag iterator use <id,address> pair to guarantee the
>> validness. But both id and address can be reused, there is
>> theoretical possibility when module inserted right after
>> another module removed, kmalloc return an address kfree by
>> previous module and IDR key reuse the key recently removed.
>>
>> Add timestamp to codetag_module and code_iterator, the
>> timestamp is generated by a clock which is strickly
>> incremented whenever a module is loaded. An iterator is
>> valid if and only if its timestamp match codetag_module's.
>>
>> Signed-off-by: David Wang <00107082@....com>
>> ---
>> include/linux/codetag.h | 1 +
>> lib/codetag.c | 12 ++++++++++--
>> 2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/codetag.h b/include/linux/codetag.h
>> index d14dbd26b370..61d43c3fbd19 100644
>> --- a/include/linux/codetag.h
>> +++ b/include/linux/codetag.h
>> @@ -54,6 +54,7 @@ struct codetag_iterator {
>> struct codetag_module *cmod;
>> unsigned long mod_id;
>> struct codetag *ct;
>> + unsigned long timestamp;
>
>The way you implement this, it is not really a timestamp, more like a
>unique id or sequence number. I would suggest calling it mod_seq or
>something similar. Maybe: cttype->next_mod_seq, iter->mod_seq and
>cmod->mod_seq.
Copy that~
>
>> };
>>
>> #ifdef MODULE
>> diff --git a/lib/codetag.c b/lib/codetag.c
>> index 42aadd6c1454..973bfa5dd5db 100644
>> --- a/lib/codetag.c
>> +++ b/lib/codetag.c
>> @@ -13,6 +13,8 @@ struct codetag_type {
>> struct idr mod_idr;
>> struct rw_semaphore mod_lock; /* protects mod_idr */
>
>The comment above should be expanded to say that mod_lock also
>protects accesses to cttype->clock, iter->timestamp and
>cmod->timestamp.
Good catch~!
Thanks
>
>> struct codetag_type_desc desc;
>> + /* generates timestamp for module load */
>> + unsigned long clock;
>> };
>>
>> struct codetag_range {
>> @@ -23,6 +25,7 @@ struct codetag_range {
>> struct codetag_module {
>> struct module *mod;
>> struct codetag_range range;
>> + unsigned long timestamp;
>> };
>>
>> static DEFINE_MUTEX(codetag_lock);
>> @@ -48,6 +51,7 @@ struct codetag_iterator codetag_get_ct_iter(struct codetag_type *cttype)
>> .cmod = NULL,
>> .mod_id = 0,
>> .ct = NULL,
>> + .timestamp = 0,
>> };
>>
>> return iter;
>> @@ -91,11 +95,13 @@ struct codetag *codetag_next_ct(struct codetag_iterator *iter)
>> if (!cmod)
>> break;
>>
>> - if (cmod != iter->cmod) {
>> + if (!iter->cmod || iter->timestamp != cmod->timestamp) {
>> iter->cmod = cmod;
>> + iter->timestamp = cmod->timestamp;
>> ct = get_first_module_ct(cmod);
>> - } else
>> + } else {
>> ct = get_next_module_ct(iter);
>> + }
>>
>> if (ct)
>> break;
>> @@ -190,6 +196,8 @@ static int codetag_module_init(struct codetag_type *cttype, struct module *mod)
>> cmod->range = range;
>>
>> down_write(&cttype->mod_lock);
>> + cttype->clock++;
>> + cmod->timestamp = cttype->clock;
>> err = idr_alloc(&cttype->mod_idr, cmod, 0, 0, GFP_KERNEL);
>> if (err >= 0) {
>> cttype->count += range_size(cttype, &range);
>> --
>> 2.39.2
>>
Powered by blists - more mailing lists