| 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
| ||
|
Date: Wed, 1 Feb 2012 16:51:08 +0800
From: Zumeng Chen <zumeng.chen@...driver.com>
To: Hugh Dickins <hughd@...gle.com>
CC: <linux-kernel@...r.kernel.org>, <torvalds@...ux-foundation.org>,
<mingo@...e.hu>, <ralf@...ux-mips.org>,
<bruce.ashfield@...driver.com>, <linux-mm@...ck.org>,
<linux-mips@...ux-mips.org>
Subject: Re: [PATCH 1/1] mm: msync: fix issues of sys_msync on tmpfs
于 2012年01月22日 04:53, Hugh Dickins 写道:
> On Fri, 20 Jan 2012, Zumeng Chen wrote:
>
>> This patch fixes two issues as follows:
> Two issues? You write of a cache aliasing issue, I don't see a second.
Hi Hugh,
I'm very sorry for delay reply due to the Spring Festival holiday.
Yes, I missed one due to just a cosmetic one :-)
>> For some filesystem with fsync == noop_fsync, there is not so much thing
>> to do, so sys_msync just passes by for all arches but some CPUs.
>>
>> For some CPUs with cache aliases(dmesg|grep alias), it maybe has an issue,
>> which reported by msync test suites in ltp-full when the memory of memset
>> used by msync01 runs into cache alias randomly.
>>
>> Consider the following scenario used by msync01 in ltp-full:
>> fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0666))< 0);
>> .../* initialization fildes by write(fildes); */
>> addr = mmap(0, page_sz, PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED,
>> fildes, 0);
>> /* set buf with memset */
>> memset(addr + OFFSET_1, 1, BUF_SIZE);
>>
>> /* msync the addr before using, or MS_SYNC*/
>> msync(addr, page_sz, MS_ASYNC)
> Actually, that MS_ASYNC msync() does nothing at all but validate its
> arguments these days, even on filesystems with an effective fsync().
> We write out dirty pages to disk in good time, msync() or not.
You are absolutely right here, although MS_SYNC is still
failure in ltp's msync01.
>> /* Tries to read fildes */
>> lseek(fildes, (off_t) OFFSET_1, SEEK_SET) != (off_t) OFFSET_1) {
>> nread = read(fildes, read_buf, sizeof(read_buf));
> My grasp of cache aliasing issues is very poor, please excuse me if I'm
> wrong; but I don't think a patch to msync() should be necessary at all
> (and ltp's msync01 test is testing nothing more than args to msync()).
Agreed, you are right. It is not a good way to fix this issue
in mm level.
> In the case of tmpfs, that read() system call above should route through
> to mm/shmem.c do_shmem_file_read(), which contains these same lines as
> the more common mm/filemap.c do_generic_file_read():
>
> /* If users can be writing to this page using arbitrary
> * virtual addresses, take care about potential aliasing
> * before reading the page on the kernel side.
> */
> if (mapping_writably_mapped(mapping))
> flush_dcache_page(page);
Yes, this is good start here, thanks Hugh.
> The mapping_writably_mapped() test ought to be succeeding in this case
> (please check with printk's, perhaps some change has messed that up),
> so flush_dcache_page(page) should be called, and any aliasing issues
> resolved before the data is copied back to userspace.
I guess we have to flush from INDEX_BASE to waysize instead
of only current page passed from msync.
> I assume your problem is on MIPS, since you copied Ralf and linux-mips:
> if the MIPS flush_dcache_page() is not working right, then you'll need
> to follow up with them.
Since some MIPS CPUs have cache alias, so we have to
flush_dcache_range instead of flush_dcache_page to avoid
cache alias. I'll send to Ralf for more knowledges.
Regards,
Zumeng
> Hugh
>
>> /* Then test the result */
>> if (read_buf[count] != 1) {
>>
>> The test result is random too for CPUs with cache alias. So in this
>> situation, we have to flush the related vma to make sure the read is
>> correct.
>>
>> Signed-off-by: Zumeng Chen<zumeng.chen@...driver.com>
>> ---
>> mm/msync.c | 30 ++++++++++++++++++++++++++++++
>> 1 files changed, 30 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/msync.c b/mm/msync.c
>> index 632df45..0021a7e 100644
>> --- a/mm/msync.c
>> +++ b/mm/msync.c
>> @@ -13,6 +13,14 @@
>> #include<linux/file.h>
>> #include<linux/syscalls.h>
>> #include<linux/sched.h>
>> +#include<asm/cacheflush.h>
>> +
>> +/* Cache aliases should be taken into accounts when msync. */
>> +#ifdef cpu_has_dc_aliases
>> +#define CPU_HAS_CACHE_ALIAS cpu_has_dc_aliases
>> +#else
>> +#define CPU_HAS_CACHE_ALIAS 0
>> +#endif
>>
>> /*
>> * MS_SYNC syncs the entire file - including mappings.
>> @@ -78,6 +86,28 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
>> }
>> file = vma->vm_file;
>> start = vma->vm_end;
>> +
>> + /*
>> + * For some filesystems with fsync == noop_fsync, msync just
>> + * passes by but some CPUs.
>> + * For CPUs with cache alias, msync has to flush the related
>> + * vma explicitly to make sure data coherency between memory
>> + * and cache, which includes MS_SYNC or MS_ASYNC. That is to
>> + * say, cache aliases should not be an async factor, so does
>> + * msync on other arches without cache aliases.
>> + */
>> + if (file&& file->f_op&& file->f_op->fsync == noop_fsync) {
>> + if (CPU_HAS_CACHE_ALIAS)
>> + flush_cache_range(vma, vma->vm_start,
>> + vma->vm_end);
>> + if (start>= end) {
>> + error = 0;
>> + goto out_unlock;
>> + }
>> + vma = find_vma(mm, start);
>> + continue;
>> + }
>> +
>> if ((flags& MS_SYNC)&& file&&
>> (vma->vm_flags& VM_SHARED)) {
>> get_file(file);
>> --
>> 1.7.0.4
--
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/
Powered by blists - more mailing lists