| Message ID | 20220608144031.829-3-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup and fixup patches for swap | expand |
Miaohe Lin <linmiaohe@huawei.com> writes: > si->inuse_pages could still be accessed concurrently now. The plain reads > outside si->lock critical section, i.e. swap_show and si_swapinfo, which > results in data races. But these should be ok because they're just used > for showing swap info. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > Reviewed-by: David Hildenbrand <david@redhat.com> > --- > mm/swapfile.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index d2bead7b8b70..3fa26f6971e9 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -2646,7 +2646,7 @@ static int swap_show(struct seq_file *swap, void *v) > } > > bytes = si->pages << (PAGE_SHIFT - 10); > - inuse = si->inuse_pages << (PAGE_SHIFT - 10); > + inuse = READ_ONCE(si->inuse_pages) << (PAGE_SHIFT - 10); > > file = si->swap_file; > len = seq_file_path(swap, file, " \t\n\\"); > @@ -3265,7 +3265,7 @@ void si_swapinfo(struct sysinfo *val) > struct swap_info_struct *si = swap_info[type]; > > if ((si->flags & SWP_USED) && !(si->flags & SWP_WRITEOK)) > - nr_to_be_unused += si->inuse_pages; > + nr_to_be_unused += READ_ONCE(si->inuse_pages); > } > val->freeswap = atomic_long_read(&nr_swap_pages) + nr_to_be_unused; > val->totalswap = total_swap_pages + nr_to_be_unused; READ_ONCE() should be paired with WRITE_ONCE(). So, change the writer side too? Best Regards, Huang, Ying
On 2022/6/20 15:54, Huang, Ying wrote: > Miaohe Lin <linmiaohe@huawei.com> writes: > >> si->inuse_pages could still be accessed concurrently now. The plain reads >> outside si->lock critical section, i.e. swap_show and si_swapinfo, which >> results in data races. But these should be ok because they're just used >> for showing swap info. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> Reviewed-by: David Hildenbrand <david@redhat.com> >> --- >> mm/swapfile.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/mm/swapfile.c b/mm/swapfile.c >> index d2bead7b8b70..3fa26f6971e9 100644 >> --- a/mm/swapfile.c >> +++ b/mm/swapfile.c >> @@ -2646,7 +2646,7 @@ static int swap_show(struct seq_file *swap, void *v) >> } >> >> bytes = si->pages << (PAGE_SHIFT - 10); >> - inuse = si->inuse_pages << (PAGE_SHIFT - 10); >> + inuse = READ_ONCE(si->inuse_pages) << (PAGE_SHIFT - 10); >> >> file = si->swap_file; >> len = seq_file_path(swap, file, " \t\n\\"); >> @@ -3265,7 +3265,7 @@ void si_swapinfo(struct sysinfo *val) >> struct swap_info_struct *si = swap_info[type]; >> >> if ((si->flags & SWP_USED) && !(si->flags & SWP_WRITEOK)) >> - nr_to_be_unused += si->inuse_pages; >> + nr_to_be_unused += READ_ONCE(si->inuse_pages); >> } >> val->freeswap = atomic_long_read(&nr_swap_pages) + nr_to_be_unused; >> val->totalswap = total_swap_pages + nr_to_be_unused; > > READ_ONCE() should be paired with WRITE_ONCE(). So, change the writer > side too? READ_ONCE() is used to fix the complaint of concurrent accessing to si->inuse_pages from KCSAN here. The similar commit is 218209487c3d ("mm/swapfile: fix data races in try_to_unuse()"). IMHO, it's fine to see a not-uptodate value of si->inuse_pages because it's just used for showing swap info. So WRITE_ONCE() is not obligatory. Or am I miss something? > > Best Regards, > Huang, Ying Thanks! > . >
On Mon, Jun 20, 2022 at 05:04:50PM +0800, Miaohe Lin wrote: > On 2022/6/20 15:54, Huang, Ying wrote: > > Miaohe Lin <linmiaohe@huawei.com> writes: > > > >> si->inuse_pages could still be accessed concurrently now. The plain reads > >> outside si->lock critical section, i.e. swap_show and si_swapinfo, which > >> results in data races. But these should be ok because they're just used > >> for showing swap info. > >> > >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > >> Reviewed-by: David Hildenbrand <david@redhat.com> > >> --- > >> mm/swapfile.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/mm/swapfile.c b/mm/swapfile.c > >> index d2bead7b8b70..3fa26f6971e9 100644 > >> --- a/mm/swapfile.c > >> +++ b/mm/swapfile.c > >> @@ -2646,7 +2646,7 @@ static int swap_show(struct seq_file *swap, void *v) > >> } > >> > >> bytes = si->pages << (PAGE_SHIFT - 10); > >> - inuse = si->inuse_pages << (PAGE_SHIFT - 10); > >> + inuse = READ_ONCE(si->inuse_pages) << (PAGE_SHIFT - 10); > >> > >> file = si->swap_file; > >> len = seq_file_path(swap, file, " \t\n\\"); > >> @@ -3265,7 +3265,7 @@ void si_swapinfo(struct sysinfo *val) > >> struct swap_info_struct *si = swap_info[type]; > >> > >> if ((si->flags & SWP_USED) && !(si->flags & SWP_WRITEOK)) > >> - nr_to_be_unused += si->inuse_pages; > >> + nr_to_be_unused += READ_ONCE(si->inuse_pages); > >> } > >> val->freeswap = atomic_long_read(&nr_swap_pages) + nr_to_be_unused; > >> val->totalswap = total_swap_pages + nr_to_be_unused; > > > > READ_ONCE() should be paired with WRITE_ONCE(). So, change the writer > > side too? > > READ_ONCE() is used to fix the complaint of concurrent accessing to si->inuse_pages from KCSAN here. > The similar commit is 218209487c3d ("mm/swapfile: fix data races in try_to_unuse()"). IMHO, it's fine I think the fix 218209487c3d is incomplete. The write side in swap_range_free() should also be fixed. Otherwise, IIUC, it cannot stop KCSAN complaining. > to see a not-uptodate value of si->inuse_pages because it's just used for showing swap info. So > WRITE_ONCE() is not obligatory. Or am I miss something? > > > > > Best Regards, > > Huang, Ying > > Thanks! > > > . > > > >
On 2022/6/20 17:23, Muchun Song wrote: > On Mon, Jun 20, 2022 at 05:04:50PM +0800, Miaohe Lin wrote: >> On 2022/6/20 15:54, Huang, Ying wrote: >>> Miaohe Lin <linmiaohe@huawei.com> writes: >>> >>>> si->inuse_pages could still be accessed concurrently now. The plain reads >>>> outside si->lock critical section, i.e. swap_show and si_swapinfo, which >>>> results in data races. But these should be ok because they're just used >>>> for showing swap info. >>>> >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> Reviewed-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> mm/swapfile.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/mm/swapfile.c b/mm/swapfile.c >>>> index d2bead7b8b70..3fa26f6971e9 100644 >>>> --- a/mm/swapfile.c >>>> +++ b/mm/swapfile.c >>>> @@ -2646,7 +2646,7 @@ static int swap_show(struct seq_file *swap, void *v) >>>> } >>>> >>>> bytes = si->pages << (PAGE_SHIFT - 10); >>>> - inuse = si->inuse_pages << (PAGE_SHIFT - 10); >>>> + inuse = READ_ONCE(si->inuse_pages) << (PAGE_SHIFT - 10); >>>> >>>> file = si->swap_file; >>>> len = seq_file_path(swap, file, " \t\n\\"); >>>> @@ -3265,7 +3265,7 @@ void si_swapinfo(struct sysinfo *val) >>>> struct swap_info_struct *si = swap_info[type]; >>>> >>>> if ((si->flags & SWP_USED) && !(si->flags & SWP_WRITEOK)) >>>> - nr_to_be_unused += si->inuse_pages; >>>> + nr_to_be_unused += READ_ONCE(si->inuse_pages); >>>> } >>>> val->freeswap = atomic_long_read(&nr_swap_pages) + nr_to_be_unused; >>>> val->totalswap = total_swap_pages + nr_to_be_unused; >>> >>> READ_ONCE() should be paired with WRITE_ONCE(). So, change the writer >>> side too? >> >> READ_ONCE() is used to fix the complaint of concurrent accessing to si->inuse_pages from KCSAN here. >> The similar commit is 218209487c3d ("mm/swapfile: fix data races in try_to_unuse()"). IMHO, it's fine > > I think the fix 218209487c3d is incomplete. The write side in swap_range_free() should > also be fixed. Otherwise, IIUC, it cannot stop KCSAN complaining. I tend to agree with you. READ_ONCE() should be paired with WRITE_ONCE() theoretically. But WRITTE_ONCE() is ignored while the commit is introduced. Add Qian Cai for helping verify it. It's very kind of @Qian Cai if he could tell us whether WRITTE_ONCE() is ignored deliberately. Thanks all of you. :) > >> to see a not-uptodate value of si->inuse_pages because it's just used for showing swap info. So >> WRITE_ONCE() is not obligatory. Or am I miss something? >> >>> >>> Best Regards, >>> Huang, Ying >> >> Thanks! >> >>> . >>> >> >> > . >
On 2022/6/20 20:23, Miaohe Lin wrote: > On 2022/6/20 17:23, Muchun Song wrote: >> On Mon, Jun 20, 2022 at 05:04:50PM +0800, Miaohe Lin wrote: >>> On 2022/6/20 15:54, Huang, Ying wrote: >>>> Miaohe Lin <linmiaohe@huawei.com> writes: >>>> >>>>> si->inuse_pages could still be accessed concurrently now. The plain reads >>>>> outside si->lock critical section, i.e. swap_show and si_swapinfo, which >>>>> results in data races. But these should be ok because they're just used >>>>> for showing swap info. >>>>> >>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>>> Reviewed-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> mm/swapfile.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/mm/swapfile.c b/mm/swapfile.c >>>>> index d2bead7b8b70..3fa26f6971e9 100644 >>>>> --- a/mm/swapfile.c >>>>> +++ b/mm/swapfile.c >>>>> @@ -2646,7 +2646,7 @@ static int swap_show(struct seq_file *swap, void *v) >>>>> } >>>>> >>>>> bytes = si->pages << (PAGE_SHIFT - 10); >>>>> - inuse = si->inuse_pages << (PAGE_SHIFT - 10); >>>>> + inuse = READ_ONCE(si->inuse_pages) << (PAGE_SHIFT - 10); >>>>> >>>>> file = si->swap_file; >>>>> len = seq_file_path(swap, file, " \t\n\\"); >>>>> @@ -3265,7 +3265,7 @@ void si_swapinfo(struct sysinfo *val) >>>>> struct swap_info_struct *si = swap_info[type]; >>>>> >>>>> if ((si->flags & SWP_USED) && !(si->flags & SWP_WRITEOK)) >>>>> - nr_to_be_unused += si->inuse_pages; >>>>> + nr_to_be_unused += READ_ONCE(si->inuse_pages); >>>>> } >>>>> val->freeswap = atomic_long_read(&nr_swap_pages) + nr_to_be_unused; >>>>> val->totalswap = total_swap_pages + nr_to_be_unused; >>>> >>>> READ_ONCE() should be paired with WRITE_ONCE(). So, change the writer >>>> side too? >>> >>> READ_ONCE() is used to fix the complaint of concurrent accessing to si->inuse_pages from KCSAN here. >>> The similar commit is 218209487c3d ("mm/swapfile: fix data races in try_to_unuse()"). IMHO, it's fine >> >> I think the fix 218209487c3d is incomplete. The write side in swap_range_free() should >> also be fixed. Otherwise, IIUC, it cannot stop KCSAN complaining. > > I tend to agree with you. READ_ONCE() should be paired with WRITE_ONCE() theoretically. But WRITTE_ONCE() > is ignored while the commit is introduced. Add Qian Cai for helping verify it. It's very kind of @Qian Cai > if he could tell us whether WRITTE_ONCE() is ignored deliberately. Update the email address of Qian Cai. > > Thanks all of you. :) > >> >>> to see a not-uptodate value of si->inuse_pages because it's just used for showing swap info. So >>> WRITE_ONCE() is not obligatory. Or am I miss something? >>> >>>> >>>> Best Regards, >>>> Huang, Ying >>> >>> Thanks! >>> >>>> . >>>> >>> >>> >> . >> >
On Mon, Jun 20, 2022 at 08:32:27PM +0800, Miaohe Lin wrote: > >>>>> --- a/mm/swapfile.c > >>>>> +++ b/mm/swapfile.c > >>>>> @@ -2646,7 +2646,7 @@ static int swap_show(struct seq_file *swap, void *v) > >>>>> } > >>>>> > >>>>> bytes = si->pages << (PAGE_SHIFT - 10); > >>>>> - inuse = si->inuse_pages << (PAGE_SHIFT - 10); > >>>>> + inuse = READ_ONCE(si->inuse_pages) << (PAGE_SHIFT - 10); > >>>>> > >>>>> file = si->swap_file; > >>>>> len = seq_file_path(swap, file, " \t\n\\"); > >>>>> @@ -3265,7 +3265,7 @@ void si_swapinfo(struct sysinfo *val) > >>>>> struct swap_info_struct *si = swap_info[type]; > >>>>> > >>>>> if ((si->flags & SWP_USED) && !(si->flags & SWP_WRITEOK)) > >>>>> - nr_to_be_unused += si->inuse_pages; > >>>>> + nr_to_be_unused += READ_ONCE(si->inuse_pages); > >>>>> } > >>>>> val->freeswap = atomic_long_read(&nr_swap_pages) + nr_to_be_unused; > >>>>> val->totalswap = total_swap_pages + nr_to_be_unused; > >>>> > >>>> READ_ONCE() should be paired with WRITE_ONCE(). So, change the writer > >>>> side too? > >>> > >>> READ_ONCE() is used to fix the complaint of concurrent accessing to si->inuse_pages from KCSAN here. > >>> The similar commit is 218209487c3d ("mm/swapfile: fix data races in try_to_unuse()"). IMHO, it's fine > >> > >> I think the fix 218209487c3d is incomplete. The write side in swap_range_free() should > >> also be fixed. Otherwise, IIUC, it cannot stop KCSAN complaining. > > > > I tend to agree with you. READ_ONCE() should be paired with WRITE_ONCE() theoretically. But WRITTE_ONCE() > > is ignored while the commit is introduced. Add Qian Cai for helping verify it. It's very kind of @Qian Cai > > if he could tell us whether WRITTE_ONCE() is ignored deliberately. The write side should be protected by the lock swap_info_struct::lock. Is that not the case here?
On Mon, Jun 20, 2022 at 09:46:47AM -0400, Qian Cai wrote: > On Mon, Jun 20, 2022 at 08:32:27PM +0800, Miaohe Lin wrote: > > >>>>> --- a/mm/swapfile.c > > >>>>> +++ b/mm/swapfile.c > > >>>>> @@ -2646,7 +2646,7 @@ static int swap_show(struct seq_file *swap, void *v) > > >>>>> } > > >>>>> > > >>>>> bytes = si->pages << (PAGE_SHIFT - 10); > > >>>>> - inuse = si->inuse_pages << (PAGE_SHIFT - 10); > > >>>>> + inuse = READ_ONCE(si->inuse_pages) << (PAGE_SHIFT - 10); > > >>>>> > > >>>>> file = si->swap_file; > > >>>>> len = seq_file_path(swap, file, " \t\n\\"); > > >>>>> @@ -3265,7 +3265,7 @@ void si_swapinfo(struct sysinfo *val) > > >>>>> struct swap_info_struct *si = swap_info[type]; > > >>>>> > > >>>>> if ((si->flags & SWP_USED) && !(si->flags & SWP_WRITEOK)) > > >>>>> - nr_to_be_unused += si->inuse_pages; > > >>>>> + nr_to_be_unused += READ_ONCE(si->inuse_pages); > > >>>>> } > > >>>>> val->freeswap = atomic_long_read(&nr_swap_pages) + nr_to_be_unused; > > >>>>> val->totalswap = total_swap_pages + nr_to_be_unused; > > >>>> > > >>>> READ_ONCE() should be paired with WRITE_ONCE(). So, change the writer > > >>>> side too? > > >>> > > >>> READ_ONCE() is used to fix the complaint of concurrent accessing to si->inuse_pages from KCSAN here. > > >>> The similar commit is 218209487c3d ("mm/swapfile: fix data races in try_to_unuse()"). IMHO, it's fine > > >> > > >> I think the fix 218209487c3d is incomplete. The write side in swap_range_free() should > > >> also be fixed. Otherwise, IIUC, it cannot stop KCSAN complaining. > > > > > > I tend to agree with you. READ_ONCE() should be paired with WRITE_ONCE() theoretically. But WRITTE_ONCE() > > > is ignored while the commit is introduced. Add Qian Cai for helping verify it. It's very kind of @Qian Cai > > > if he could tell us whether WRITTE_ONCE() is ignored deliberately. > > The write side should be protected by the lock swap_info_struct::lock. Is > that not the case here? > The lock does not protect the read sides. So the write side should be fixed by WRITTE_ONCE(). Thanks.
On Mon, Jun 20, 2022 at 10:20:07PM +0800, Muchun Song wrote: > The lock does not protect the read sides. So the write side should be > fixed by WRITTE_ONCE(). https://lwn.net/Articles/816854/ "Unmarked writes (aligned and up to word size) can be treated as if they had used WRITE_ONCE() by building with CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y (also selected by default). Experience has shown that compilers are much less likely to destructively optimize in-kernel writes than reads. Some developers might therefore choose to use READ_ONCE() but omit the corresponding WRITE_ONCE(). Other developers might prefer the documentation benefits and long-term peace of mind accruing from explicit use of WRITE_ONCE()..."
Qian Cai <quic_qiancai@quicinc.com> writes: > On Mon, Jun 20, 2022 at 10:20:07PM +0800, Muchun Song wrote: >> The lock does not protect the read sides. So the write side should be >> fixed by WRITTE_ONCE(). > > https://lwn.net/Articles/816854/ > > "Unmarked writes (aligned and up to word size) can be treated as if they had > used WRITE_ONCE() by building with > CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y (also selected by default). > Experience has shown that compilers are much less likely to destructively > optimize in-kernel writes than reads. Some developers might therefore > choose to use READ_ONCE() but omit the corresponding WRITE_ONCE(). Other > developers might prefer the documentation benefits and long-term peace of > mind accruing from explicit use of WRITE_ONCE()..." Thanks for pointing me to this great article. So although not required by KCSAN strictly, WRITE_ONCE() is still good for documentation, etc. Just like we have done for swap_info_struct->highest_bit, etc. Best Regards, Huang, Ying
On Tue, Jun 21, 2022 at 09:14:00AM +0800, Huang, Ying wrote: > Qian Cai <quic_qiancai@quicinc.com> writes: > > > On Mon, Jun 20, 2022 at 10:20:07PM +0800, Muchun Song wrote: > >> The lock does not protect the read sides. So the write side should be > >> fixed by WRITTE_ONCE(). > > > > https://lwn.net/Articles/816854/ > > > > "Unmarked writes (aligned and up to word size) can be treated as if they had > > used WRITE_ONCE() by building with > > CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y (also selected by default). All right, CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC help us avoid KCSAN complaining. > > Experience has shown that compilers are much less likely to destructively > > optimize in-kernel writes than reads. Some developers might therefore > > choose to use READ_ONCE() but omit the corresponding WRITE_ONCE(). Other > > developers might prefer the documentation benefits and long-term peace of > > mind accruing from explicit use of WRITE_ONCE()..." > > Thanks for pointing me to this great article. So although not required > by KCSAN strictly, WRITE_ONCE() is still good for documentation, etc. > Just like we have done for swap_info_struct->highest_bit, etc. > +1 > Best Regards, > Huang, Ying >
On 2022/6/21 11:39, Muchun Song wrote: > On Tue, Jun 21, 2022 at 09:14:00AM +0800, Huang, Ying wrote: >> Qian Cai <quic_qiancai@quicinc.com> writes: >> >>> On Mon, Jun 20, 2022 at 10:20:07PM +0800, Muchun Song wrote: >>>> The lock does not protect the read sides. So the write side should be >>>> fixed by WRITTE_ONCE(). >>> >>> https://lwn.net/Articles/816854/ >>> >>> "Unmarked writes (aligned and up to word size) can be treated as if they had >>> used WRITE_ONCE() by building with >>> CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y (also selected by default). > > All right, CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC help us avoid KCSAN > complaining. > >>> Experience has shown that compilers are much less likely to destructively >>> optimize in-kernel writes than reads. Some developers might therefore >>> choose to use READ_ONCE() but omit the corresponding WRITE_ONCE(). Other >>> developers might prefer the documentation benefits and long-term peace of >>> mind accruing from explicit use of WRITE_ONCE()..." >> >> Thanks for pointing me to this great article. So although not required >> by KCSAN strictly, WRITE_ONCE() is still good for documentation, etc. >> Just like we have done for swap_info_struct->highest_bit, etc. >> > > +1 I tend to agree with Muchun & Huang, Ying. Thanks all of you. > >> Best Regards, >> Huang, Ying >> > . >
diff --git a/mm/swapfile.c b/mm/swapfile.c index d2bead7b8b70..3fa26f6971e9 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -2646,7 +2646,7 @@ static int swap_show(struct seq_file *swap, void *v) } bytes = si->pages << (PAGE_SHIFT - 10); - inuse = si->inuse_pages << (PAGE_SHIFT - 10); + inuse = READ_ONCE(si->inuse_pages) << (PAGE_SHIFT - 10); file = si->swap_file; len = seq_file_path(swap, file, " \t\n\\"); @@ -3265,7 +3265,7 @@ void si_swapinfo(struct sysinfo *val) struct swap_info_struct *si = swap_info[type]; if ((si->flags & SWP_USED) && !(si->flags & SWP_WRITEOK)) - nr_to_be_unused += si->inuse_pages; + nr_to_be_unused += READ_ONCE(si->inuse_pages); } val->freeswap = atomic_long_read(&nr_swap_pages) + nr_to_be_unused; val->totalswap = total_swap_pages + nr_to_be_unused;