* [PATCH] dax: Release PMD lock even when there is no PMD support in DAX @ 2018-01-18 13:38 Jan H. Schönherr 2018-01-18 14:07 ` Matthew Wilcox 2018-01-18 16:20 ` Ross Zwisler 0 siblings, 2 replies; 9+ messages in thread From: Jan H. Schönherr @ 2018-01-18 13:38 UTC (permalink / raw) To: Matthew Wilcox, Ross Zwisler; +Cc: Jan H. Schönherr, linux-fsdevel The function follow_pte_pmd() can theoretically return after having acquired a PMD lock, even when DAX was not compiled with CONFIG_FS_DAX_PMD. Release the PMD lock unconditionally. Fixes: f729c8c9b24f ("dax: wrprotect pmd_t in dax_mapping_entry_mkclean") Signed-off-by: Jan H. Schönherr <jschoenh@amazon.de> --- fs/dax.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/dax.c b/fs/dax.c index 9598159..c2ebf10 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -636,8 +636,8 @@ static void dax_mapping_entry_mkclean(struct address_space *mapping, pmd = pmd_mkclean(pmd); set_pmd_at(vma->vm_mm, address, pmdp, pmd); unlock_pmd: - spin_unlock(ptl); #endif + spin_unlock(ptl); } else { if (pfn != pte_pfn(*ptep)) goto unlock_pte; -- 2.9.3.1.gcba166c.dirty ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] dax: Release PMD lock even when there is no PMD support in DAX 2018-01-18 13:38 [PATCH] dax: Release PMD lock even when there is no PMD support in DAX Jan H. Schönherr @ 2018-01-18 14:07 ` Matthew Wilcox 2018-01-18 14:22 ` Matthew Wilcox 2018-01-18 14:22 ` Jan H. Schönherr 2018-01-18 16:20 ` Ross Zwisler 1 sibling, 2 replies; 9+ messages in thread From: Matthew Wilcox @ 2018-01-18 14:07 UTC (permalink / raw) To: Jan H. Schönherr; +Cc: Matthew Wilcox, Ross Zwisler, linux-fsdevel On Thu, Jan 18, 2018 at 02:38:39PM +0100, Jan H. Sch�nherr wrote: > The function follow_pte_pmd() can theoretically return after having > acquired a PMD lock, even when DAX was not compiled with > CONFIG_FS_DAX_PMD. I don't think it can. How would a PMD entry get into a DAX VMA if we compiled the kernel without CONFIG_FS_DAX_PMD? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dax: Release PMD lock even when there is no PMD support in DAX 2018-01-18 14:07 ` Matthew Wilcox @ 2018-01-18 14:22 ` Matthew Wilcox 2018-01-18 14:27 ` Jan H. Schönherr 2018-01-18 14:22 ` Jan H. Schönherr 1 sibling, 1 reply; 9+ messages in thread From: Matthew Wilcox @ 2018-01-18 14:22 UTC (permalink / raw) To: Jan H. Schönherr; +Cc: Matthew Wilcox, Ross Zwisler, linux-fsdevel On Thu, Jan 18, 2018 at 06:07:39AM -0800, Matthew Wilcox wrote: > On Thu, Jan 18, 2018 at 02:38:39PM +0100, Jan H. Sch�nherr wrote: > > The function follow_pte_pmd() can theoretically return after having > > acquired a PMD lock, even when DAX was not compiled with > > CONFIG_FS_DAX_PMD. > > I don't think it can. How would a PMD entry get into a DAX VMA if we > compiled the kernel without CONFIG_FS_DAX_PMD? How about this patch instead? Should shut up sparse nicely. diff --git a/fs/dax.c b/fs/dax.c index 78b72c48374e..fea1b64d111b 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -586,6 +586,12 @@ pgoff_address(pgoff_t pgoff, struct vm_area_struct *vma) return address; } +#ifdef CONFIG_FS_DAX_PMD +#define dax_pmd(pmdp) pmdp +#else +#define dax_pmd(pmdp) 0 +#endif + /* Walk all mappings of a given index of a file and writeprotect them */ static void dax_mapping_entry_mkclean(struct address_space *mapping, pgoff_t index, unsigned long pfn) @@ -621,8 +627,7 @@ static void dax_mapping_entry_mkclean(struct address_space *mapping, * * See Documentation/vm/mmu_notifier.txt */ - if (pmdp) { -#ifdef CONFIG_FS_DAX_PMD + if (dax_pmd(pmdp)) { pmd_t pmd; if (pfn != pmd_pfn(*pmdp)) @@ -638,7 +643,6 @@ static void dax_mapping_entry_mkclean(struct address_space *mapping, set_pmd_at(vma->vm_mm, address, pmdp, pmd); unlock_pmd: spin_unlock(ptl); -#endif } else { if (pfn != pte_pfn(*ptep)) goto unlock_pte; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] dax: Release PMD lock even when there is no PMD support in DAX 2018-01-18 14:22 ` Matthew Wilcox @ 2018-01-18 14:27 ` Jan H. Schönherr 2018-01-18 14:35 ` Jan H. Schönherr 0 siblings, 1 reply; 9+ messages in thread From: Jan H. Schönherr @ 2018-01-18 14:27 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Matthew Wilcox, Ross Zwisler, linux-fsdevel On 01/18/2018 03:22 PM, Matthew Wilcox wrote: > On Thu, Jan 18, 2018 at 06:07:39AM -0800, Matthew Wilcox wrote: >> On Thu, Jan 18, 2018 at 02:38:39PM +0100, Jan H. Schönherr wrote: >>> The function follow_pte_pmd() can theoretically return after having >>> acquired a PMD lock, even when DAX was not compiled with >>> CONFIG_FS_DAX_PMD. >> >> I don't think it can. How would a PMD entry get into a DAX VMA if we >> compiled the kernel without CONFIG_FS_DAX_PMD? > > How about this patch instead? Should shut up sparse nicely. It would still skip the unlock, in case pmdp is !=NULL (and locked) after follow_pte_pmd(). So it wouldn't address, what I intended to address with the patch. Regards Jan > > diff --git a/fs/dax.c b/fs/dax.c > index 78b72c48374e..fea1b64d111b 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -586,6 +586,12 @@ pgoff_address(pgoff_t pgoff, struct vm_area_struct *vma) > return address; > } > > +#ifdef CONFIG_FS_DAX_PMD > +#define dax_pmd(pmdp) pmdp > +#else > +#define dax_pmd(pmdp) 0 > +#endif > + > /* Walk all mappings of a given index of a file and writeprotect them */ > static void dax_mapping_entry_mkclean(struct address_space *mapping, > pgoff_t index, unsigned long pfn) > @@ -621,8 +627,7 @@ static void dax_mapping_entry_mkclean(struct address_space *mapping, > * > * See Documentation/vm/mmu_notifier.txt > */ > - if (pmdp) { > -#ifdef CONFIG_FS_DAX_PMD > + if (dax_pmd(pmdp)) { > pmd_t pmd; > > if (pfn != pmd_pfn(*pmdp)) > @@ -638,7 +643,6 @@ static void dax_mapping_entry_mkclean(struct address_space *mapping, > set_pmd_at(vma->vm_mm, address, pmdp, pmd); > unlock_pmd: > spin_unlock(ptl); > -#endif > } else { > if (pfn != pte_pfn(*ptep)) > goto unlock_pte; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dax: Release PMD lock even when there is no PMD support in DAX 2018-01-18 14:27 ` Jan H. Schönherr @ 2018-01-18 14:35 ` Jan H. Schönherr 0 siblings, 0 replies; 9+ messages in thread From: Jan H. Schönherr @ 2018-01-18 14:35 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Matthew Wilcox, Ross Zwisler, linux-fsdevel On 01/18/2018 03:27 PM, Jan H. Schönherr wrote: > It would still skip the unlock, in case pmdp is !=NULL (and locked) after follow_pte_pmd(). > So it wouldn't address, what I intended to address with the patch. Small correction: we'd take the "else" branch, which would do some stuff it's not supposed to do when follow_pte_pmd() returns with pmdp!=NULL. And we'd actually do a unlock, but thinking it's a PTE, not a PMD. So, defensive-wise, may point still stands, that we're not correctly handling an unexpectedly returned PMD entry. Regards Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dax: Release PMD lock even when there is no PMD support in DAX 2018-01-18 14:07 ` Matthew Wilcox 2018-01-18 14:22 ` Matthew Wilcox @ 2018-01-18 14:22 ` Jan H. Schönherr 1 sibling, 0 replies; 9+ messages in thread From: Jan H. Schönherr @ 2018-01-18 14:22 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Matthew Wilcox, Ross Zwisler, linux-fsdevel On 01/18/2018 03:07 PM, Matthew Wilcox wrote: > On Thu, Jan 18, 2018 at 02:38:39PM +0100, Jan H. Schönherr wrote: >> The function follow_pte_pmd() can theoretically return after having >> acquired a PMD lock, even when DAX was not compiled with >> CONFIG_FS_DAX_PMD. > > I don't think it can. How would a PMD entry get into a DAX VMA if we > compiled the kernel without CONFIG_FS_DAX_PMD? > Maybe it can not in happy cases. But the PMD parts in follow_pte_pmd() are compiled in unconditionally. So, if there's an issue elsewhere, and for some weird reason we get a PMD entry in the page table, it would screw the lock balance. I haven't run into an actual issue with this, it's just supposed to be defensive. Regards Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dax: Release PMD lock even when there is no PMD support in DAX 2018-01-18 13:38 [PATCH] dax: Release PMD lock even when there is no PMD support in DAX Jan H. Schönherr 2018-01-18 14:07 ` Matthew Wilcox @ 2018-01-18 16:20 ` Ross Zwisler 2018-01-25 16:34 ` Ross Zwisler 1 sibling, 1 reply; 9+ messages in thread From: Ross Zwisler @ 2018-01-18 16:20 UTC (permalink / raw) To: Jan H. Schönherr; +Cc: Matthew Wilcox, Ross Zwisler, linux-fsdevel On Thu, Jan 18, 2018 at 02:38:39PM +0100, Jan H. Sch�nherr wrote: > The function follow_pte_pmd() can theoretically return after having > acquired a PMD lock, even when DAX was not compiled with > CONFIG_FS_DAX_PMD. > > Release the PMD lock unconditionally. > > Fixes: f729c8c9b24f ("dax: wrprotect pmd_t in dax_mapping_entry_mkclean") > Signed-off-by: Jan H. Sch�nherr <jschoenh@amazon.de> > --- > fs/dax.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/dax.c b/fs/dax.c > index 9598159..c2ebf10 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -636,8 +636,8 @@ static void dax_mapping_entry_mkclean(struct address_space *mapping, > pmd = pmd_mkclean(pmd); > set_pmd_at(vma->vm_mm, address, pmdp, pmd); > unlock_pmd: > - spin_unlock(ptl); > #endif > + spin_unlock(ptl); > } else { > if (pfn != pte_pfn(*ptep)) > goto unlock_pte; Sure, this seems fine to me. This seems simple and correct - you're right that we aren't taking the PTL on the PMD conditionally based on whether CONFIG_DAX_PMD is defined, so it doesn't make sense to release it conditionally. I think if we ever hit this lock imbalance we're totally insane anyway, but it the fix is correct and doesn't mess with our code flow. You can add: Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dax: Release PMD lock even when there is no PMD support in DAX 2018-01-18 16:20 ` Ross Zwisler @ 2018-01-25 16:34 ` Ross Zwisler 2018-01-25 21:29 ` Jan H. Schönherr 0 siblings, 1 reply; 9+ messages in thread From: Ross Zwisler @ 2018-01-25 16:34 UTC (permalink / raw) To: Andrew Morton Cc: Ross Zwisler, Jan H. Schönherr, Matthew Wilcox, linux-fsdevel On Thu, Jan 18, 2018 at 09:20:13AM -0700, Ross Zwisler wrote: > On Thu, Jan 18, 2018 at 02:38:39PM +0100, Jan H. Sch�nherr wrote: > > The function follow_pte_pmd() can theoretically return after having > > acquired a PMD lock, even when DAX was not compiled with > > CONFIG_FS_DAX_PMD. > > > > Release the PMD lock unconditionally. > > > > Fixes: f729c8c9b24f ("dax: wrprotect pmd_t in dax_mapping_entry_mkclean") > > Signed-off-by: Jan H. Sch�nherr <jschoenh@amazon.de> > > --- > > fs/dax.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/dax.c b/fs/dax.c > > index 9598159..c2ebf10 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -636,8 +636,8 @@ static void dax_mapping_entry_mkclean(struct address_space *mapping, > > pmd = pmd_mkclean(pmd); > > set_pmd_at(vma->vm_mm, address, pmdp, pmd); > > unlock_pmd: > > - spin_unlock(ptl); > > #endif > > + spin_unlock(ptl); > > } else { > > if (pfn != pte_pfn(*ptep)) > > goto unlock_pte; > > Sure, this seems fine to me. This seems simple and correct - you're right > that we aren't taking the PTL on the PMD conditionally based on whether > CONFIG_DAX_PMD is defined, so it doesn't make sense to release it > conditionally. I think if we ever hit this lock imbalance we're totally > insane anyway, but it the fix is correct and doesn't mess with our code flow. > > You can add: > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> Ah, I just realized that this patch didn't CC Andrew, and he's the one that usually takes our DAX patches. Andrew, can you pick this up? Here's the fsdevel patchwork: https://patchwork.kernel.org/patch/10173255/ Thanks, - Ross ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dax: Release PMD lock even when there is no PMD support in DAX 2018-01-25 16:34 ` Ross Zwisler @ 2018-01-25 21:29 ` Jan H. Schönherr 0 siblings, 0 replies; 9+ messages in thread From: Jan H. Schönherr @ 2018-01-25 21:29 UTC (permalink / raw) To: Ross Zwisler, Andrew Morton, Matthew Wilcox, linux-fsdevel On 01/25/2018 05:34 PM, Ross Zwisler wrote: > Ah, I just realized that this patch didn't CC Andrew, and he's the one that > usually takes our DAX patches. > > Andrew, can you pick this up? Here's the fsdevel patchwork: > > https://patchwork.kernel.org/patch/10173255/ Thanks for that, I didn't know. Let me know, if I should resend instead (in case it makes things easier). Regards Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-01-25 21:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-18 13:38 [PATCH] dax: Release PMD lock even when there is no PMD support in DAX Jan H. Schönherr 2018-01-18 14:07 ` Matthew Wilcox 2018-01-18 14:22 ` Matthew Wilcox 2018-01-18 14:27 ` Jan H. Schönherr 2018-01-18 14:35 ` Jan H. Schönherr 2018-01-18 14:22 ` Jan H. Schönherr 2018-01-18 16:20 ` Ross Zwisler 2018-01-25 16:34 ` Ross Zwisler 2018-01-25 21:29 ` Jan H. Schönherr
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).