* [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: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 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 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.