Re: [BUGFIX][PATCH 2/3] kcore: fix vread/vwrite to be aware ofholes.

[KAMEZAWA Hiroyuki]

On Tue, 4 Aug 2009 17:23:16 +0800
Amerigo Wang <xiyou.wangcong@xxxxxxxxx> wrote:

> On Mon, Aug 03, 2009 at 08:18:45PM +0900, KAMEZAWA Hiroyuki wrote:
> >From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> >
> >vread/vwrite access vmalloc area without checking there is a page or not.
> >In most case, this works well.
> >
> >In old ages, the caller of get_vm_ara() is only IOREMAP and there is no
> >memory hole within vm_struct's [addr...addr + size - PAGE_SIZE]
> >( -PAGE_SIZE is for a guard page.)
> >
> >After per-cpu-alloc patch, it uses get_vm_area() for reserve continuous
> >virtual address but remap _later_. There tend to be a hole in valid vmalloc
> >area in vm_struct lists.
> >Then, skip the hole (not mapped page) is necessary.
> >This patch updates vread/vwrite() for avoiding memory hole.
> >
> >Routines which access vmalloc area without knowing for which addr is used
> >are
> >  - /proc/kcore
> >  - /dev/kmem
> >
> >kcore checks IOREMAP, /dev/kmem doesn't. After this patch, IOREMAP is
> >checked and /dev/kmem will avoid to read/write it.
> >Fixes to /proc/kcore will be in the next patch in series.
> >
> >Changelog v2->v3:
> > - fixed typos.
> > - use kmap. (if not using kmap, we have to add lock here.)
> 
> 
> Hmm.. I missed this.
> 
> 
> > - fixed PAGE_MASK miss-use.
> >Changelog v1->v2:
> > - enhanced comments.
> > - treat IOREMAP as hole always.
> > - zero-fill memory hole if [addr...addr+size] includes valid pages.
> > - returns 0 if [addr...addr+size) includes no valid pages.
> >
> >Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> 
> 
> This time it looks much better now, but it still has a small problem.
> Please check it below.
> 
> 
thanks.


> >---
> > mm/vmalloc.c |  182 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 159 insertions(+), 23 deletions(-)
> >
> 
> <snip>
> 
> 
> >+
> >+/**
> >+ *	vread() -  read vmalloc area in a safe way.
> >+ *	@buf:		buffer for reading data
> >+ *	@addr:		vm address.
> >+ *	@count:		number of bytes to be read.
> >+ *
> >+ *	Returns # of bytes which addr and buf should be increased.
> >+ *	(same to count).
> >+ *	If [addr...addr+count) doesn't includes any valid area, returns 0.
> 
> 
> If I read it correctly, your code doesn't do what you described here,
> it doesn't return 0 when there is no valid area.
> 

Hmm.. valid area is ambiguous, ok, how about this ?

If [addr...addr+count) has no intersects with exisiting vm_struct, returns 0.

Thanks,
-Kame




> 
> >+ *
> >+ *	This function checks that addr is a valid vmalloc'ed area, and
> >+ *	copy data from that area to a given buffer. If the given memory range of
> >+ *	[addr...addr+count) includes some valid address, data is copied to
> >+ *	proper area of @buf. If there are memory holes, they'll be zero-filled.
> >+ *	IOREMAP area is treated as memory hole and no copy is done.
> >+ *
> >+ *	Note: In usual ops, vread() is never necessary because the caller should
> >+ *	know vmalloc() area is valid and can use memcpy(). This is for routines
> >+ *	which have to access vmalloc area without any informaion, as /dev/kmem.
> >+ *
> >+ *	The caller should guarantee KM_USER1 is not used.
> >+ */
> >+
> > long vread(char *buf, char *addr, unsigned long count)
> > {
> > 	struct vm_struct *tmp;
> > 	char *vaddr, *buf_start = buf;
> >+	unsigned long buflen = count;
> > 	unsigned long n;
> > 
> > 	/* Don't allow overflow */
> >@@ -1640,7 +1739,7 @@
> > 		count = -(unsigned long) addr;
> > 
> > 	read_lock(&vmlist_lock);
> >-	for (tmp = vmlist; tmp; tmp = tmp->next) {
> >+	for (tmp = vmlist; count && tmp; tmp = tmp->next) {
> > 		vaddr = (char *) tmp->addr;
> > 		if (addr >= vaddr + tmp->size - PAGE_SIZE)
> > 			continue;
> >@@ -1653,32 +1752,66 @@
> > 			count--;
> > 		}
> > 		n = vaddr + tmp->size - PAGE_SIZE - addr;
> >-		do {
> >-			if (count == 0)
> >-				goto finished;
> >-			*buf = *addr;
> >-			buf++;
> >-			addr++;
> >-			count--;
> >-		} while (--n > 0);
> >+		if (n > count)
> >+			n = count;
> >+		if (!(tmp->flags & VM_IOREMAP))
> >+			aligned_vread(buf, addr, n);
> >+		else /* IOREMAP area is treated as memory hole */
> >+			memset(buf, 0, n);
> >+		buf += n;
> >+		addr += n;
> >+		count -= n;
> > 	}
> > finished:
> > 	read_unlock(&vmlist_lock);
> >-	return buf - buf_start;
> >+
> >+	if (buf == buf_start)
> >+		return 0;
> >+	/* zero-fill memory holes */
> >+	if (buf != buf_start + buflen)
> >+		memset(buf, 0, buflen - (buf - buf_start));
> >+
> >+	return buflen;
> > }
> 

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