On Wed, Apr 16, 2003 at 11:01:19PM +1000, Keith Owens wrote:
> On Wed, 16 Apr 2003 17:31:33 +0530,
> "Vamsi Krishna S ." <vamsi@xxxxxxxxxx> wrote:
> >diff -urN -X /home/vamsi/.dontdiff 2420-kdb4.1-pure/kdb/kdbsupport.c
> >2420-kdb4.1/kdb/kdbsupport.c
> >--- 2420-kdb4.1-pure/kdb/kdbsupport.c 2003-04-16 11:38:14.000000000
> >+0530
> >+++ 2420-kdb4.1/kdb/kdbsupport.c 2003-04-16 12:56:42.000000000 +0530
> >+/*
> >+ * from mm/memory.c, adapted to run without any locks to work within kdb
> >+ */
> >+static struct page * kdb_follow_page(struct mm_struct *mm, unsigned long
> >address, int write)
>
> AFAICT this function is identical to follow_page(). Instead of
> duplicating that code and possibly getting out of sync with the real
> follow_page(), change follow_page() so it is extern for CONFIG_KDB=y,
> otherwise it is static.
>
Yes, it is exactly the same as that in mm/memory.c. The reasons for
copying it to kdb/kdbsupport.c were:
- I thought you would like to not touch too many files.
- some kernels have calls to BUG() in follow_page(), we may not want to
BUG out when called from KDB.
On second thoughts, these BUGs are very very unlikely to hit in practise
and what is much more important is to keep it in sync with
the mainline version.. I already spent a few hours debugging the panic
that occured on United Linux kernel with this patch, before realising
that UL kernel can keep page tables in high memory.
So, yes, in my next patch, I will use the function from mm/memory.c
and remove the copy from kdb/kdbsupport.c
> >+static struct page * kdb_get_one_user_page(struct task_struct *tsk,
> >unsigned long start,
> >+ int len, int write)
>
> Why have a write flag? KDB will only read user pages, or are you
> planning more changes that will require write access?
>
Yes, I wanted to add support for "mm" into user space addresses, if you
are willing to accept the patch. I will now send out another patch
which will use the write too.
> >+ /* shouldn't cross a page boundary. temporary restriction. */
> >+ if ((from & PAGE_MASK) != ((from+size) & PAGE_MASK)) {
> >+ kdb_printf("%s: crosses page boundary: from=%08lx, size=%d\n",
> >+ __FUNCTION__, from, size);
> >+ return size;
> >+ }
>
> 'return size' changes the semantics of __kdba_getarea_size. Currently
> it returns 0 or a negative error code, now you are returning a positive
> value on an error. Why? Any failure to get a user page should return
> -EFAULT, the same as a kernel page.
>
No. __kdba_getarea_size returns what it gets from __copy_from_user.
__copy_from_user always returns the number of bytes that are still
not copied (== size when you don't copy anything due to faults). Which
is why you see at many places all over the kernel constructs like:
if (copy_from_user(.. ))
return -EFAULT;
So, I think this is okay. I will send you an updated patch with
- use follow_page() from mm/memory.c
- support for "mm" to user pages
- ia64 support too.
Thanks,
Vamsi.
--
Vamsi Krishna S.
Linux Technology Center,
IBM Software Lab, Bangalore.
Ph: +91 80 5044959
Internet: vamsi@xxxxxxxxxx
|