| Message ID | 20220824071909.192535-2-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mm: fix null-ptr-deref in kswapd_is_running() | expand |
On 24.08.22 09:19, Kefeng Wang wrote: > The pgdat->kswapd could be accessed concurrently by kswapd_run() and > kcompactd(), it don't be protected by any lock, which could leads to > data races, adding READ/WRITE_ONCE() to slince it. Okay, I think this patch here makes it clearer that we really just want proper synchronization instead of hacking around it. What speaks against protecting pgdat->kswapd this using some proper locking primitive? > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > mm/compaction.c | 4 +++- > mm/vmscan.c | 8 ++++---- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index 640fa76228dd..aa1cfe47f046 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1983,7 +1983,9 @@ static inline bool is_via_compact_memory(int order) > > static bool kswapd_is_running(pg_data_t *pgdat) > { > - return pgdat->kswapd && task_is_running(pgdat->kswapd); > + struct task_struct *t = READ_ONCE(pgdat->kswapd); > + > + return t && task_is_running(t); > } > > /* > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 08c6497f76c3..65b19ca8c8ee 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -4644,7 +4644,7 @@ void kswapd_run(int nid) > pg_data_t *pgdat = NODE_DATA(nid); > struct task_struct *t; > > - if (pgdat->kswapd) > + if (READ_ONCE(pgdat->kswapd)) > return; > > t = kthread_run(kswapd, pgdat, "kswapd%d", nid); > @@ -4653,7 +4653,7 @@ void kswapd_run(int nid) > BUG_ON(system_state < SYSTEM_RUNNING); > pr_err("Failed to start kswapd on node %d\n", nid); > } else { > - pgdat->kswapd = t; > + WRITE_ONCE(pgdat->kswapd, t); > } > } > > @@ -4663,11 +4663,11 @@ void kswapd_run(int nid) > */ > void kswapd_stop(int nid) > { > - struct task_struct *kswapd = NODE_DATA(nid)->kswapd; > + struct task_struct *kswapd = READ_ONCE(NODE_DATA(nid)->kswapd); > > if (kswapd) { > kthread_stop(kswapd); > - NODE_DATA(nid)->kswapd = NULL; > + WRITE_ONCE(NODE_DATA(nid)->kswapd, NULL); > } > } >
On 2022/8/24 16:24, David Hildenbrand wrote: > On 24.08.22 09:19, Kefeng Wang wrote: >> The pgdat->kswapd could be accessed concurrently by kswapd_run() and >> kcompactd(), it don't be protected by any lock, which could leads to >> data races, adding READ/WRITE_ONCE() to slince it. > Okay, I think this patch here makes it clearer that we really just want > proper synchronization instead of hacking around it. > > What speaks against protecting pgdat->kswapd this using some proper > locking primitive? So add a new lock into struct pglist_data to protect pgdat->kswapd，other option, thanks. >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> mm/compaction.c | 4 +++- >> mm/vmscan.c | 8 ++++---- >> 2 files changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index 640fa76228dd..aa1cfe47f046 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -1983,7 +1983,9 @@ static inline bool is_via_compact_memory(int order) >> >> static bool kswapd_is_running(pg_data_t *pgdat) >> { >> - return pgdat->kswapd && task_is_running(pgdat->kswapd); >> + struct task_struct *t = READ_ONCE(pgdat->kswapd); >> + >> + return t && task_is_running(t); >> } >> >> /* >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index 08c6497f76c3..65b19ca8c8ee 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -4644,7 +4644,7 @@ void kswapd_run(int nid) >> pg_data_t *pgdat = NODE_DATA(nid); >> struct task_struct *t; >> >> - if (pgdat->kswapd) >> + if (READ_ONCE(pgdat->kswapd)) >> return; >> >> t = kthread_run(kswapd, pgdat, "kswapd%d", nid); >> @@ -4653,7 +4653,7 @@ void kswapd_run(int nid) >> BUG_ON(system_state < SYSTEM_RUNNING); >> pr_err("Failed to start kswapd on node %d\n", nid); >> } else { >> - pgdat->kswapd = t; >> + WRITE_ONCE(pgdat->kswapd, t); >> } >> } >> >> @@ -4663,11 +4663,11 @@ void kswapd_run(int nid) >> */ >> void kswapd_stop(int nid) >> { >> - struct task_struct *kswapd = NODE_DATA(nid)->kswapd; >> + struct task_struct *kswapd = READ_ONCE(NODE_DATA(nid)->kswapd); >> >> if (kswapd) { >> kthread_stop(kswapd); >> - NODE_DATA(nid)->kswapd = NULL; >> + WRITE_ONCE(NODE_DATA(nid)->kswapd, NULL); >> } >> } >> >
On 2022/8/24 16:24, David Hildenbrand wrote: > On 24.08.22 09:19, Kefeng Wang wrote: >> The pgdat->kswapd could be accessed concurrently by kswapd_run() and >> kcompactd(), it don't be protected by any lock, which could leads to >> data races, adding READ/WRITE_ONCE() to slince it. > Okay, I think this patch here makes it clearer that we really just want > proper synchronization instead of hacking around it. > > What speaks against protecting pgdat->kswapd this using some proper > locking primitive? as comments about kswapd in struct pglist_data, pgdat->kswapd should be protected by mem_hotplug_begin/done(), how about this way? diff --git a/mm/compaction.c b/mm/compaction.c index 640fa76228dd..62018f35242a 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -1983,7 +1983,13 @@ static inline bool is_via_compact_memory(int order) static bool kswapd_is_running(pg_data_t *pgdat) { - return pgdat->kswapd && task_is_running(pgdat->kswapd); + bool running; + + mem_hotplug_begin(); + running = pgdat->kswapd && task_is_running(pgdat->kswapd); + mem_hotplug_end(); + + return running; }
On 25.08.22 04:34, Kefeng Wang wrote: > > On 2022/8/24 16:24, David Hildenbrand wrote: >> On 24.08.22 09:19, Kefeng Wang wrote: >>> The pgdat->kswapd could be accessed concurrently by kswapd_run() and >>> kcompactd(), it don't be protected by any lock, which could leads to >>> data races, adding READ/WRITE_ONCE() to slince it. >> Okay, I think this patch here makes it clearer that we really just want >> proper synchronization instead of hacking around it. >> >> What speaks against protecting pgdat->kswapd this using some proper >> locking primitive? > > as comments about kswapd in struct pglist_data, pgdat->kswapd should be > > protected by mem_hotplug_begin/done(), how about this way? > > diff --git a/mm/compaction.c b/mm/compaction.c > index 640fa76228dd..62018f35242a 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1983,7 +1983,13 @@ static inline bool is_via_compact_memory(int order) > > static bool kswapd_is_running(pg_data_t *pgdat) > { > - return pgdat->kswapd && task_is_running(pgdat->kswapd); > + bool running; > + > + mem_hotplug_begin(); > + running = pgdat->kswapd && task_is_running(pgdat->kswapd); > + mem_hotplug_end(); > + > + return running; > } I'd much rather just use a dedicated lock that does not involve memory hotplug.
On 2022/8/25 16:22, David Hildenbrand wrote: > On 25.08.22 04:34, Kefeng Wang wrote: >> On 2022/8/24 16:24, David Hildenbrand wrote: >>> On 24.08.22 09:19, Kefeng Wang wrote: >>>> The pgdat->kswapd could be accessed concurrently by kswapd_run() and >>>> kcompactd(), it don't be protected by any lock, which could leads to >>>> data races, adding READ/WRITE_ONCE() to slince it. >>> Okay, I think this patch here makes it clearer that we really just want >>> proper synchronization instead of hacking around it. >>> >>> What speaks against protecting pgdat->kswapd this using some proper >>> locking primitive? >> as comments about kswapd in struct pglist_data, pgdat->kswapd should be >> >> protected by mem_hotplug_begin/done(), how about this way? >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index 640fa76228dd..62018f35242a 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -1983,7 +1983,13 @@ static inline bool is_via_compact_memory(int order) >> >> static bool kswapd_is_running(pg_data_t *pgdat) >> { >> - return pgdat->kswapd && task_is_running(pgdat->kswapd); >> + bool running; >> + >> + mem_hotplug_begin(); >> + running = pgdat->kswapd && task_is_running(pgdat->kswapd); >> + mem_hotplug_end(); >> + >> + return running; >> } > I'd much rather just use a dedicated lock that does not involve memory > hotplug. The issue only occurred due memory hotplug, without mem-hotplug, the kswapd won't stop or re-run, there is no above issue too, add a new lock would be duplicated, but the scope of protection is smaller, I could repost with new lock if no more comment. > >
diff --git a/mm/compaction.c b/mm/compaction.c index 640fa76228dd..aa1cfe47f046 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -1983,7 +1983,9 @@ static inline bool is_via_compact_memory(int order) static bool kswapd_is_running(pg_data_t *pgdat) { - return pgdat->kswapd && task_is_running(pgdat->kswapd); + struct task_struct *t = READ_ONCE(pgdat->kswapd); + + return t && task_is_running(t); } /* diff --git a/mm/vmscan.c b/mm/vmscan.c index 08c6497f76c3..65b19ca8c8ee 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -4644,7 +4644,7 @@ void kswapd_run(int nid) pg_data_t *pgdat = NODE_DATA(nid); struct task_struct *t; - if (pgdat->kswapd) + if (READ_ONCE(pgdat->kswapd)) return; t = kthread_run(kswapd, pgdat, "kswapd%d", nid); @@ -4653,7 +4653,7 @@ void kswapd_run(int nid) BUG_ON(system_state < SYSTEM_RUNNING); pr_err("Failed to start kswapd on node %d\n", nid); } else { - pgdat->kswapd = t; + WRITE_ONCE(pgdat->kswapd, t); } } @@ -4663,11 +4663,11 @@ void kswapd_run(int nid) */ void kswapd_stop(int nid) { - struct task_struct *kswapd = NODE_DATA(nid)->kswapd; + struct task_struct *kswapd = READ_ONCE(NODE_DATA(nid)->kswapd); if (kswapd) { kthread_stop(kswapd); - NODE_DATA(nid)->kswapd = NULL; + WRITE_ONCE(NODE_DATA(nid)->kswapd, NULL); } }
The pgdat->kswapd could be accessed concurrently by kswapd_run() and kcompactd(), it don't be protected by any lock, which could leads to data races, adding READ/WRITE_ONCE() to slince it. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/compaction.c | 4 +++- mm/vmscan.c | 8 ++++---- 2 files changed, 7 insertions(+), 5 deletions(-)