| Message ID | 20180118133839.20587-1-jschoenh@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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>
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
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
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;
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(-)