dax: Release PMD lock even when there is no PMD support in DAX

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.

Quoted 3 lines · Jan H. Sch�nherr

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?

Quoted 7 lines · Matthew Wilcox

Quoted 3 lines · Jan H. Schönherr

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.

fs/dax.c +7 -3

index 78b72c48374e..fea1b64d111b 100644
@@ -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;

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.

Quoted 3 lines · Jan H. Schönherr

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?

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 <[email protected]>

---
 fs/dax.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/dax.c +0 -0

index 9598159..c2ebf10 100644

Quoted 17 lines · Jan H. Sch�nherr

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 <[email protected]>

---
 fs/dax.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/dax.c +0 -0

index 9598159..c2ebf10 100644

fs/dax.c static void dax_mapping_entry_mkclean(struct address_space *mapping, · line 643

Quoted 9 lines

fs/dax.c +1 -1

@@ -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 <[email protected]>

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/