The stop_scan_thread() and start_scan_thread() cannot really solve
the problem of concurrent accessing the global jiffies_scan_wait.
kmemleak_write kmemleak_scan_thread
while (!kthread_should_stop())
stop_scan_thread
jiffies_scan_wait = xxx timeout = jiffies_scan_wait
start_scan_thread
We could replace these with a READ_ONCE() when reading
jiffies_scan_wait. It also can prevent compiler from reordering the
jiffies_scan_wait which is in while loop.
Signed-off-by: Yanfei Xu <[email protected]>
---
mm/kmemleak.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 92a2d4885808..5ccf3969b7fe 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1567,7 +1567,7 @@ static int kmemleak_scan_thread(void *arg)
}
while (!kthread_should_stop()) {
- signed long timeout = jiffies_scan_wait;
+ signed long timeout = READ_ONCE(jiffies_scan_wait);
mutex_lock(&scan_mutex);
kmemleak_scan();
@@ -1812,11 +1812,8 @@ static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
ret = kstrtoul(buf + 5, 0, &secs);
if (ret < 0)
goto out;
- stop_scan_thread();
- if (secs) {
+ if (secs)
jiffies_scan_wait = msecs_to_jiffies(secs * 1000);
- start_scan_thread();
- }
} else if (strncmp(buf, "scan", 4) == 0)
kmemleak_scan();
else if (strncmp(buf, "dump=", 5) == 0)
--
2.27.0
On Wed, Jun 09, 2021 at 11:56:57PM +0800, Yanfei Xu wrote:
> The stop_scan_thread() and start_scan_thread() cannot really solve
> the problem of concurrent accessing the global jiffies_scan_wait.
>
> kmemleak_write kmemleak_scan_thread
> while (!kthread_should_stop())
> stop_scan_thread
> jiffies_scan_wait = xxx timeout = jiffies_scan_wait
> start_scan_thread
>
> We could replace these with a READ_ONCE() when reading
> jiffies_scan_wait. It also can prevent compiler from reordering the
> jiffies_scan_wait which is in while loop.
I'm ok with READ_ONCE but your patch introduces functional changes.
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 92a2d4885808..5ccf3969b7fe 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -1567,7 +1567,7 @@ static int kmemleak_scan_thread(void *arg)
> }
>
> while (!kthread_should_stop()) {
> - signed long timeout = jiffies_scan_wait;
> + signed long timeout = READ_ONCE(jiffies_scan_wait);
>
> mutex_lock(&scan_mutex);
> kmemleak_scan();
> @@ -1812,11 +1812,8 @@ static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
> ret = kstrtoul(buf + 5, 0, &secs);
> if (ret < 0)
> goto out;
> - stop_scan_thread();
> - if (secs) {
> + if (secs)
> jiffies_scan_wait = msecs_to_jiffies(secs * 1000);
For symmetry, I'd add a WRITE_ONCE here as well.
> - start_scan_thread();
> - }
The reason for stop/start_scan_thread() wasn't to protect against
jiffies_scan_wait access but rather to force a new delay. Let's say you
start by default with a 10min delay between scans (default) but you want
to lower it to 1min. With the above removal of stop/start, you'd still
have to wait for 10min until the scanning thread will notice the change.
Also, with secs=0, the expectations is that the thread won't be
restarted but this is removed by your patch.
--
Catalin
On 6/11/21 4:59 PM, Catalin Marinas wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On Wed, Jun 09, 2021 at 11:56:57PM +0800, Yanfei Xu wrote:
>> The stop_scan_thread() and start_scan_thread() cannot really solve
>> the problem of concurrent accessing the global jiffies_scan_wait.
>>
>> kmemleak_write kmemleak_scan_thread
>> while (!kthread_should_stop())
>> stop_scan_thread
>> jiffies_scan_wait = xxx timeout = jiffies_scan_wait
>> start_scan_thread
>>
>> We could replace these with a READ_ONCE() when reading
>> jiffies_scan_wait. It also can prevent compiler from reordering the
>> jiffies_scan_wait which is in while loop.
>
> I'm ok with READ_ONCE but your patch introduces functional changes.
>
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index 92a2d4885808..5ccf3969b7fe 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -1567,7 +1567,7 @@ static int kmemleak_scan_thread(void *arg)
>> }
>>
>> while (!kthread_should_stop()) {
>> - signed long timeout = jiffies_scan_wait;
>> + signed long timeout = READ_ONCE(jiffies_scan_wait);
>>
>> mutex_lock(&scan_mutex);
>> kmemleak_scan();
>> @@ -1812,11 +1812,8 @@ static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
>> ret = kstrtoul(buf + 5, 0, &secs);
>> if (ret < 0)
>> goto out;
>> - stop_scan_thread();
>> - if (secs) {
>> + if (secs)
>> jiffies_scan_wait = msecs_to_jiffies(secs * 1000);
>
> For symmetry, I'd add a WRITE_ONCE here as well.
>
>> - start_scan_thread();
>> - }
>
> The reason for stop/start_scan_thread() wasn't to protect against
> jiffies_scan_wait access but rather to force a new delay. Let's say you
> start by default with a 10min delay between scans (default) but you want
> to lower it to 1min. With the above removal of stop/start, you'd still
> have to wait for 10min until the scanning thread will notice the change.
> Also, with secs=0, the expectations is that the thread won't be
> restarted but this is removed by your patch.
>
I see.
Thanks for your explain and sorry for my bad introduction. Will send a v2.
Thanks,
Yanfei
> --
> Catalin
>
On 6/11/21 7:17 PM, Xu, Yanfei wrote:
>
>
> On 6/11/21 4:59 PM, Catalin Marinas wrote:
>> [Please note: This e-mail is from an EXTERNAL e-mail address]
>>
>> On Wed, Jun 09, 2021 at 11:56:57PM +0800, Yanfei Xu wrote:
>>> The stop_scan_thread() and start_scan_thread() cannot really solve
>>> the problem of concurrent accessing the global jiffies_scan_wait.
>>>
>>> kmemleak_write kmemleak_scan_thread
>>> while (!kthread_should_stop())
>>> stop_scan_thread
>>> jiffies_scan_wait = xxx timeout = jiffies_scan_wait
>>> start_scan_thread
>>>
>>> We could replace these with a READ_ONCE() when reading
>>> jiffies_scan_wait. It also can prevent compiler from reordering the
>>> jiffies_scan_wait which is in while loop.
>>
>> I'm ok with READ_ONCE but your patch introduces functional changes.
>>
>>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>>> index 92a2d4885808..5ccf3969b7fe 100644
>>> --- a/mm/kmemleak.c
>>> +++ b/mm/kmemleak.c
>>> @@ -1567,7 +1567,7 @@ static int kmemleak_scan_thread(void *arg)
>>> }
>>>
>>> while (!kthread_should_stop()) {
>>> - signed long timeout = jiffies_scan_wait;
>>> + signed long timeout = READ_ONCE(jiffies_scan_wait);
>>>
>>> mutex_lock(&scan_mutex);
>>> kmemleak_scan();
>>> @@ -1812,11 +1812,8 @@ static ssize_t kmemleak_write(struct file
>>> *file, const char __user *user_buf,
>>> ret = kstrtoul(buf + 5, 0, &secs);
>>> if (ret < 0)
>>> goto out;
>>> - stop_scan_thread();
>>> - if (secs) {
>>> + if (secs)
>>> jiffies_scan_wait = msecs_to_jiffies(secs *
>>> 1000);
>>
>> For symmetry, I'd add a WRITE_ONCE here as well.
>>
>>> - start_scan_thread();
>>> - }
>>
>> The reason for stop/start_scan_thread() wasn't to protect against
>> jiffies_scan_wait access but rather to force a new delay. Let's say you
>> start by default with a 10min delay between scans (default) but you want
>> to lower it to 1min. With the above removal of stop/start, you'd still
>> have to wait for 10min until the scanning thread will notice the change.
>> Also, with secs=0, the expectations is that the thread won't be
>> restarted but this is removed by your patch.
>>
>
> I see.
> Thanks for your explain and sorry for my bad introduction. Will send a v2.
>
Hi Catalin and Andrew,
I sent the v2 patch which is renamed to:
[PATCH] mm/kmemleak: fix the possible wrong memory scanning period
I have tested it on qemux86, and hope you can help to review. Thanks.
--Yanfei
> Thanks,
> Yanfei
>
>> --
>> Catalin
>>