| Message ID | 20210425095419.3830298-5-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | close various race windows for swap | expand |
Miaohe Lin <linmiaohe@huawei.com> writes: > When I was investigating the swap code, I found the below possible race > window: > > CPU 1 CPU 2 > ----- ----- > shmem_swapin > swap_cluster_readahead > if (likely(si->flags & (SWP_BLKDEV | SWP_FS_OPS))) { > swapoff > .. > si->swap_file = NULL; > .. > struct inode *inode = si->swap_file->f_mapping->host;[oops!] > > Close this race window by using get/put_swap_device() to guard against > concurrent swapoff. > > Fixes: 8fd2e0b505d1 ("mm: swap: check if swap backing device is congested or not") > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> Thanks! Reviewed-by: "Huang, Ying" <ying.huang@intel.com> > --- > mm/shmem.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/mm/shmem.c b/mm/shmem.c > index 26c76b13ad23..2dafd65b0b42 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -1696,6 +1696,7 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, > struct address_space *mapping = inode->i_mapping; > struct shmem_inode_info *info = SHMEM_I(inode); > struct mm_struct *charge_mm = vma ? vma->vm_mm : current->mm; > + struct swap_info_struct *si; > struct page *page; > swp_entry_t swap; > int error; > @@ -1704,6 +1705,12 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, > swap = radix_to_swp_entry(*pagep); > *pagep = NULL; > > + /* Prevent swapoff from happening to us. */ > + si = get_swap_device(swap); > + if (!si) { > + error = EINVAL; > + goto failed; > + } > /* Look it up and read it in.. */ > page = lookup_swap_cache(swap, NULL, 0); > if (!page) { > @@ -1765,6 +1772,8 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, > swap_free(swap); > > *pagep = page; > + if (si) > + put_swap_device(si); > return 0; > failed: > if (!shmem_confirm_swap(mapping, index, swap)) > @@ -1775,6 +1784,9 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, > put_page(page); > } > > + if (si) > + put_swap_device(si); > + > return error; > }
On Sun, Apr 25, 2021 at 3:54 AM Miaohe Lin <linmiaohe@huawei.com> wrote: > > When I was investigating the swap code, I found the below possible race > window: > > CPU 1 CPU 2 > ----- ----- > shmem_swapin > swap_cluster_readahead > if (likely(si->flags & (SWP_BLKDEV | SWP_FS_OPS))) { > swapoff > .. > si->swap_file = NULL; > .. > struct inode *inode = si->swap_file->f_mapping->host;[oops!] > > Close this race window by using get/put_swap_device() to guard against > concurrent swapoff. > > Fixes: 8fd2e0b505d1 ("mm: swap: check if swap backing device is congested or not") > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/shmem.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/mm/shmem.c b/mm/shmem.c > index 26c76b13ad23..2dafd65b0b42 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -1696,6 +1696,7 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, > struct address_space *mapping = inode->i_mapping; > struct shmem_inode_info *info = SHMEM_I(inode); > struct mm_struct *charge_mm = vma ? vma->vm_mm : current->mm; > + struct swap_info_struct *si; > struct page *page; > swp_entry_t swap; > int error; > @@ -1704,6 +1705,12 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, > swap = radix_to_swp_entry(*pagep); > *pagep = NULL; > > + /* Prevent swapoff from happening to us. */ > + si = get_swap_device(swap); > + if (!si) { > + error = EINVAL; > + goto failed; > + } page is uninitialized? > /* Look it up and read it in.. */ > page = lookup_swap_cache(swap, NULL, 0); > if (!page) { > @@ -1765,6 +1772,8 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, > swap_free(swap); > > *pagep = page; > + if (si) > + put_swap_device(si); > return 0; > failed: > if (!shmem_confirm_swap(mapping, index, swap)) > @@ -1775,6 +1784,9 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, > put_page(page); > } > > + if (si) > + put_swap_device(si); > + > return error; > } > > -- > 2.23.0 > >
On 2021/4/26 14:53, Yu Zhao wrote: > On Sun, Apr 25, 2021 at 3:54 AM Miaohe Lin <linmiaohe@huawei.com> wrote: >> >> When I was investigating the swap code, I found the below possible race >> window: >> >> CPU 1 CPU 2 >> ----- ----- >> shmem_swapin >> swap_cluster_readahead >> if (likely(si->flags & (SWP_BLKDEV | SWP_FS_OPS))) { >> swapoff >> .. >> si->swap_file = NULL; >> .. >> struct inode *inode = si->swap_file->f_mapping->host;[oops!] >> >> Close this race window by using get/put_swap_device() to guard against >> concurrent swapoff. >> >> Fixes: 8fd2e0b505d1 ("mm: swap: check if swap backing device is congested or not") >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/shmem.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/mm/shmem.c b/mm/shmem.c >> index 26c76b13ad23..2dafd65b0b42 100644 >> --- a/mm/shmem.c >> +++ b/mm/shmem.c >> @@ -1696,6 +1696,7 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, >> struct address_space *mapping = inode->i_mapping; >> struct shmem_inode_info *info = SHMEM_I(inode); >> struct mm_struct *charge_mm = vma ? vma->vm_mm : current->mm; >> + struct swap_info_struct *si; >> struct page *page; >> swp_entry_t swap; >> int error; >> @@ -1704,6 +1705,12 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, >> swap = radix_to_swp_entry(*pagep); >> *pagep = NULL; >> >> + /* Prevent swapoff from happening to us. */ >> + si = get_swap_device(swap); >> + if (!si) { >> + error = EINVAL; >> + goto failed; >> + } > > page is uninitialized? > Sorry, my overlook! Compiler should have complained about it but there is none... Many thanks for pointing this out! Will fix it in next version. >> /* Look it up and read it in.. */ >> page = lookup_swap_cache(swap, NULL, 0); >> if (!page) { >> @@ -1765,6 +1772,8 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, >> swap_free(swap); >> >> *pagep = page; >> + if (si) >> + put_swap_device(si); >> return 0; >> failed: >> if (!shmem_confirm_swap(mapping, index, swap)) >> @@ -1775,6 +1784,9 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, >> put_page(page); >> } >> >> + if (si) >> + put_swap_device(si); >> + >> return error; >> } >> >> -- >> 2.23.0 >> >> > . >
diff --git a/mm/shmem.c b/mm/shmem.c index 26c76b13ad23..2dafd65b0b42 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -1696,6 +1696,7 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, struct address_space *mapping = inode->i_mapping; struct shmem_inode_info *info = SHMEM_I(inode); struct mm_struct *charge_mm = vma ? vma->vm_mm : current->mm; + struct swap_info_struct *si; struct page *page; swp_entry_t swap; int error; @@ -1704,6 +1705,12 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, swap = radix_to_swp_entry(*pagep); *pagep = NULL; + /* Prevent swapoff from happening to us. */ + si = get_swap_device(swap); + if (!si) { + error = EINVAL; + goto failed; + } /* Look it up and read it in.. */ page = lookup_swap_cache(swap, NULL, 0); if (!page) { @@ -1765,6 +1772,8 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, swap_free(swap); *pagep = page; + if (si) + put_swap_device(si); return 0; failed: if (!shmem_confirm_swap(mapping, index, swap)) @@ -1775,6 +1784,9 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, put_page(page); } + if (si) + put_swap_device(si); + return error; }
When I was investigating the swap code, I found the below possible race window: CPU 1 CPU 2 ----- ----- shmem_swapin swap_cluster_readahead if (likely(si->flags & (SWP_BLKDEV | SWP_FS_OPS))) { swapoff .. si->swap_file = NULL; .. struct inode *inode = si->swap_file->f_mapping->host;[oops!] Close this race window by using get/put_swap_device() to guard against concurrent swapoff. Fixes: 8fd2e0b505d1 ("mm: swap: check if swap backing device is congested or not") Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/shmem.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)