linux-kernel - Re: [RFC][PATCH] vmalloc: simplify vread()/vwrite()

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: <20100107025054.GA11252@localhost>
Date:	Thu, 7 Jan 2010 10:50:54 +0800
From:	Wu Fengguang <fengguang.wu@...el.com>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Tejun Heo <tj@...nel.org>, Ingo Molnar <mingo@...e.hu>,
	Nick Piggin <npiggin@...e.de>,
	Andi Kleen <andi@...stfloor.org>,
	Hugh Dickins <hugh.dickins@...cali.co.uk>,
	Christoph Lameter <cl@...ux-foundation.org>,
	Linux Memory Management List <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>,
	"Huang, Ying" <ying.huang@...el.com>
Subject: Re: [RFC][PATCH] vmalloc: simplify vread()/vwrite()

On Thu, Jan 07, 2010 at 09:38:25AM +0800, KAMEZAWA Hiroyuki wrote:
> On Thu, 7 Jan 2010 09:24:59 +0800
> Wu Fengguang <fengguang.wu@...el.com> wrote:
> 
> > vread()/vwrite() is only called from kcore/kmem to access one page at
> > a time.  So the logic can be vastly simplified.
> > 
> I recommend you to rename the function because safety of function is
> changed and you can show what callers are influenced.

OK.
 
> > The changes are:
> > - remove the vmlist walk and rely solely on vmalloc_to_page()
> > - replace the VM_IOREMAP check with (page && page_is_ram(pfn))
> > 
> > The VM_IOREMAP check is introduced in commit d0107eb07320b for per-cpu
> > alloc. Kame, would you double check if this change is OK for that
> > purpose?
> > 
> I think VM_IOREMAP is for avoiding access to device configuration area and
> unexpected breakage in device. Then, VM_IOREMAP are should be skipped by
> the caller. (My patch _just_ moves the avoidance of callers to vread()/vwrite())

"device configuration area" is not RAM, so testing of RAM would be
able to skip them?

> 
> > The page_is_ram() check is necessary because kmap_atomic() is not
> > designed to work with non-RAM pages.
> > 
> I think page_is_ram() is not a complete method...on x86, it just check
> e820's memory range. checking VM_IOREMAP is better, I think.

(double check) Not complete or not safe?

EFI seems to not update e820 table by default.  Ying, do you know why?

> > Even for a RAM page, we don't own the page, and cannot assume it's a
> > _PAGE_CACHE_WB page. So I wonder whether it's necessary to do another
> > patch to call reserve_memtype() before kmap_atomic() to ensure cache
> > consistency?
> > 
> > TODO: update comments accordingly
> > 
> 
> BTW, f->f_pos problem on 64bit machine still exists and this patch is still
> hard to test. I stopped that because anyone doesn't show any interests.

I'm using your patch :)

I feel most inconfident on this patch, so submitted it for RFC first.
I'll then submit a full patch series including your f_pos fix.

> I have no objection to your direction.
> 
> but please rewrite the function explanation as
> "addr" should be page alinged and bufsize should be multiple of page size."
> and change the function names.

OK, I'll rename it to vread_page().

Thanks,
Fengguang
--
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/