[PATCH v2] mm/hwpoison: reset hwpoison filter parameters in pfn_inject_exit()

[PATCH v2] mm/hwpoison: reset hwpoison filter parameters in pfn_inject_exit()

[Miaohe Lin]

When hwpoison_inject module is removed, hwpoison_filter_* parameters
should be reset. Otherwise these parameters will have non-default values
at next insmod time.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
v2:
 Add missing call to reset_hwpoison_filter() per Andrew. Thanks.
---
 mm/hwpoison-inject.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
index 7ecaa1900137..0b855cd3433a 100644
--- a/mm/hwpoison-inject.c
+++ b/mm/hwpoison-inject.c
@@ -64,10 +64,22 @@ static int hwpoison_unpoison(void *data, u64 val)
 DEFINE_DEBUGFS_ATTRIBUTE(hwpoison_fops, NULL, hwpoison_inject, "%lli\n");
 DEFINE_DEBUGFS_ATTRIBUTE(unpoison_fops, NULL, hwpoison_unpoison, "%lli\n");
 
-static void __exit pfn_inject_exit(void)
+static inline void reset_hwpoison_filter(void)
 {
 	hwpoison_filter_enable = 0;
+	hwpoison_filter_dev_major = ~0U;
+	hwpoison_filter_dev_minor = ~0U;
+	hwpoison_filter_flags_mask = 0;
+	hwpoison_filter_flags_value = 0;
+#ifdef CONFIG_MEMCG
+	hwpoison_filter_memcg = 0;
+#endif
+}
+
+static void __exit pfn_inject_exit(void)
+{
 	debugfs_remove_recursive(hwpoison_dir);
+	reset_hwpoison_filter();
 }
 
 static int __init pfn_inject_init(void)
-- 
2.33.0

Re: [PATCH v2] mm/hwpoison: reset hwpoison filter parameters in pfn_inject_exit()

[Michal Hocko]

On Tue 16-07-24 11:35:16, Miaohe Lin wrote:
> When hwpoison_inject module is removed, hwpoison_filter_* parameters
> should be reset. Otherwise these parameters will have non-default values
> at next insmod time.

There is a clear layering broken here. We have mm/memory-failure.c using
values and mm/hwpoison-inject.c defining the values. Both with a
potentially different life time. Shouldn't that be fix instead?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] mm/hwpoison: reset hwpoison filter parameters in pfn_inject_exit()

[Miaohe Lin]

On 2024/7/16 16:38, Michal Hocko wrote:
> On Tue 16-07-24 11:35:16, Miaohe Lin wrote:
>> When hwpoison_inject module is removed, hwpoison_filter_* parameters
>> should be reset. Otherwise these parameters will have non-default values
>> at next insmod time.
> 
> There is a clear layering broken here. We have mm/memory-failure.c using
> values and mm/hwpoison-inject.c defining the values. Both with a
> potentially different life time. Shouldn't that be fix instead?

In fact, we have mm/memory-failure.c defining and using these values while they can
only be modified through mm/hwpoison-inject.c from userspace. The common usecase should be:

1. User set hwpoison filter parameters first through mm/hwpoison-inject.c.
2. Then doing memory hwpoison test through mm/hwpoison-inject.c.

hwpoison_filter_* parameters are only used for test from userspace. From this perspective,
this potentially different life time doesn't break anything. Or am I miss something?

Thanks.
.

Re: [PATCH v2] mm/hwpoison: reset hwpoison filter parameters in pfn_inject_exit()

[Michal Hocko]

On Wed 17-07-24 10:23:06, Miaohe Lin wrote:
> On 2024/7/16 16:38, Michal Hocko wrote:
> > On Tue 16-07-24 11:35:16, Miaohe Lin wrote:
> >> When hwpoison_inject module is removed, hwpoison_filter_* parameters
> >> should be reset. Otherwise these parameters will have non-default values
> >> at next insmod time.
> > 
> > There is a clear layering broken here. We have mm/memory-failure.c using
> > values and mm/hwpoison-inject.c defining the values. Both with a
> > potentially different life time. Shouldn't that be fix instead?
> 
> In fact, we have mm/memory-failure.c defining and using these values while they can
> only be modified through mm/hwpoison-inject.c from userspace.

Yes, this is exactly what I mean by broken layering that should be
fixed.

> The common usecase should be:
> 
> 1. User set hwpoison filter parameters first through mm/hwpoison-inject.c.
> 2. Then doing memory hwpoison test through mm/hwpoison-inject.c.

Why does this need to be done through different modules? Why it cannot
be part of the memory-filure.c?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] mm/hwpoison: reset hwpoison filter parameters in pfn_inject_exit()

[Miaohe Lin]

On 2024/7/17 14:18, Michal Hocko wrote:
> On Wed 17-07-24 10:23:06, Miaohe Lin wrote:
>> On 2024/7/16 16:38, Michal Hocko wrote:
>>> On Tue 16-07-24 11:35:16, Miaohe Lin wrote:
>>>> When hwpoison_inject module is removed, hwpoison_filter_* parameters
>>>> should be reset. Otherwise these parameters will have non-default values
>>>> at next insmod time.
>>>
>>> There is a clear layering broken here. We have mm/memory-failure.c using
>>> values and mm/hwpoison-inject.c defining the values. Both with a
>>> potentially different life time. Shouldn't that be fix instead?
>>
>> In fact, we have mm/memory-failure.c defining and using these values while they can
>> only be modified through mm/hwpoison-inject.c from userspace.
> 
> Yes, this is exactly what I mean by broken layering that should be
> fixed.
> 
>> The common usecase should be:
>>
>> 1. User set hwpoison filter parameters first through mm/hwpoison-inject.c.
>> 2. Then doing memory hwpoison test through mm/hwpoison-inject.c.
> 
> Why does this need to be done through different modules? Why it cannot
> be part of the memory-filure.c?

This is a bold idea for me. :) I think it can be part of the memory-filure.c.
So CONFIG_HWPOISON_INJECT should be removed from the world and then make
hwpoison-inject default on when MEMORY_FAILURE is configured?

Thanks.
.

Re: [PATCH v2] mm/hwpoison: reset hwpoison filter parameters in pfn_inject_exit()

[Michal Hocko]

On Wed 17-07-24 14:59:40, Miaohe Lin wrote:
> On 2024/7/17 14:18, Michal Hocko wrote:
[...]
> > Why does this need to be done through different modules? Why it cannot
> > be part of the memory-filure.c?
> 
> This is a bold idea for me. :) I think it can be part of the memory-filure.c.
> So CONFIG_HWPOISON_INJECT should be removed from the world and then make
> hwpoison-inject default on when MEMORY_FAILURE is configured?

I would start by moving code into mm/hwpoison-inject.c. Whether this
should also remove CONFIG_HWPOISON_INJECT is a different question. I
am not really sure a debugfs interface warrants a config option. Anyway
two things for two separate patches
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] mm/hwpoison: reset hwpoison filter parameters in pfn_inject_exit()

[Miaohe Lin]

On 2024/7/17 15:25, Michal Hocko wrote:
> On Wed 17-07-24 14:59:40, Miaohe Lin wrote:
>> On 2024/7/17 14:18, Michal Hocko wrote:
> [...]
>>> Why does this need to be done through different modules? Why it cannot
>>> be part of the memory-filure.c?
>>
>> This is a bold idea for me. :) I think it can be part of the memory-filure.c.
>> So CONFIG_HWPOISON_INJECT should be removed from the world and then make
>> hwpoison-inject default on when MEMORY_FAILURE is configured?
> 
> I would start by moving code into mm/hwpoison-inject.c. Whether this
> should also remove CONFIG_HWPOISON_INJECT is a different question. I
> am not really sure a debugfs interface warrants a config option. Anyway
> two things for two separate patches
> 

Thanks for your suggestion. :) Will try to do it when I am free.
Thanks.
.