linux-ext4 - Re: [PATCH 11/18] dax: Fix condition for filling of PMD holes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160502131636.GF17362@quack2.suse.cz>
Date:	Mon, 2 May 2016 15:16:36 +0200
From:	Jan Kara <jack@...e.cz>
To:	Ross Zwisler <ross.zwisler@...ux.intel.com>
Cc:	Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
	linux-ext4@...r.kernel.org, linux-mm@...ck.org,
	Dan Williams <dan.j.williams@...el.com>,
	linux-nvdimm@...ts.01.org, Matthew Wilcox <willy@...ux.intel.com>
Subject: Re: [PATCH 11/18] dax: Fix condition for filling of PMD holes

On Fri 29-04-16 13:08:05, Ross Zwisler wrote:
> On Mon, Apr 18, 2016 at 11:35:34PM +0200, Jan Kara wrote:
> > Currently dax_pmd_fault() decides to fill a PMD-sized hole only if
> > returned buffer has BH_Uptodate set. However that doesn't get set for
> > any mapping buffer so that branch is actually a dead code. The
> > BH_Uptodate check doesn't make any sense so just remove it.
> 
> I'm not sure about this one.  In my testing (which was a while ago) I was
> also never able to exercise this code path and create huge zero pages.   My
> concern is that by removing the buffer_uptodate() check, we will all of a
> sudden start running through a code path that was previously unreachable.
> 
> AFAICT the buffer_uptodate() was part of the original PMD commit.  Did we ever
> get buffers with BH_Uptodate set?  Has this code ever been run?  Does it work?
> 
> I suppose this concern is mitigated by the fact that later in this series you 
> disable the PMD path entirely, but maybe we should just leave it as is and
> turn it off, then clean it up if/when we reenable it when we add multi-order
> radix tree locking for PMDs?

Well, I did this as a separate commit exactly because I'm not sure about
the impact since nobody was actually able to test this code. So we can
easily bisect later if we find issues. The code just didn't make sense to
me that way and later in the series I update it to use radix tree locking
which would be hard to do without having code which actually makes some
sense. So I'd prefer to keep this change...

								Honza

> > Signed-off-by: Jan Kara <jack@...e.cz>
> > ---
> >  fs/dax.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 237581441bc1..42bf65b4e752 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -878,7 +878,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
> >  		goto fallback;
> >  	}
> >  
> > -	if (!write && !buffer_mapped(&bh) && buffer_uptodate(&bh)) {
> > +	if (!write && !buffer_mapped(&bh)) {
> >  		spinlock_t *ptl;
> >  		pmd_t entry;
> >  		struct page *zero_page = get_huge_zero_page();
> > -- 
> > 2.6.6
> > 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html