linux-kernel - Re: cache alias in mmap + write

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]

Date:	Thu, 21 Jan 2010 10:10:04 +0900 (JST)
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	anfei <anfei.zhou@...il.com>
Cc:	kosaki.motohiro@...fujitsu.com, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, linux@....linux.org.uk, jamie@...reable.org
Subject: Re: cache alias in mmap + write

> On Wed, Jan 20, 2010 at 06:10:11PM +0900, KOSAKI Motohiro wrote:
> > Hello,
> > 
> > > diff --git a/mm/filemap.c b/mm/filemap.c
> > > index 96ac6b0..07056fb 100644
> > > --- a/mm/filemap.c
> > > +++ b/mm/filemap.c
> > > @@ -2196,6 +2196,9 @@ again:
> > >  		if (unlikely(status))
> > >  			break;
> > >  
> > > +		if (mapping_writably_mapped(mapping))
> > > +			flush_dcache_page(page);
> > > +
> > >  		pagefault_disable();
> > >  		copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
> > >  		pagefault_enable();
> > 
> > I'm not sure ARM cache coherency model. but I guess correct patch is here.
> > 
> > +		if (mapping_writably_mapped(mapping))
> > +			flush_dcache_page(page);
> > +
> >  		pagefault_disable();
> >  		copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
> >  		pagefault_enable();
> > -		flush_dcache_page(page);
> > 
> > Why do we need to call flush_dcache_page() twice?
> > 
> The latter flush_dcache_page is used to flush the kernel changes
> (iov_iter_copy_from_user_atomic), which makes the userspace to see the
> write,  and the one I added is used to flush the userspace changes.
> And I think it's better to split this function into two:
> 	flush_dcache_user_page(page);
> 	kmap_atomic(page);
> 	write to  page;
> 	kunmap_atomic(page);
> 	flush_dcache_kern_page(page);
> But currently there is no such API.

Why can't we create new api? this your pseudo code looks very fine to me.


note: if you don't like to create new api. I can agree your current patch.
but I have three requests.
 1. Move flush_dcache_page() into iov_iter_copy_from_user_atomic().
    Your above explanation indicate it is real intention. plus, change
    iov_iter_copy_from_user_atomic() fixes fuse too.
 2. Add some commnet. almost developer only have x86 machine. so, arm
    specific trick need additional explicit explanation. otherwise anybody
    might break this code in the future.
 3. Resend the patch. original mail isn't good patch format. please consider
    to reduce akpm suffer.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/