AIO syscall patch, and problem

AIO syscall patch, and problem

[David Dillow]

The attached patch fixes a mis-numbering of the AIO syscalls under sparc
and sparc64. I would have simply moved them all down, but then
io_getevents would be at 272, and the big WARNING comment says we cannot
use numbers larger than 271, so there you are. I used the oldstat
syscall, since it was commented out.

With this patch, and with appropriate glue added to libio from
kernel.org, I can get it to pass most of the test cases included with
the library on my Ultra1.

However, there is one test case that is still failing on the sparc64 box
-- test 5.t, which mmaps() a file opened O_RDWR with PROT_READ,
MAP_SHARED and then submits the address returned to an aio read() call.

The test case expects this to return -EFAULT, and on x86, it does. On my
machine, it returns -EHOSTDOWN.

I've tracked this down to file_read_actor() in mm/filemap.c:

/* Do it the slow way */
kaddr = kmap(page);
left = __copy_to_user(desc->buf, kaddr + offset, size);
kunmap(page);

if (left) {
	size -= left;
	desc->error = -EFAULT;
}

The problem is __copy_to_user() is being called with a size of 512, but
is returning 576. size then underflows in the conditional, and
aio_complete() eventually gets called with this as its return code,
hence, -EHOSTDOWN.

I briefly looked at __copy_to_user(), but I quickly ran away screaming.
Someone smarted than I will need to go from here.

Dave

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1548  -> 1.1549 
#	include/asm-sparc/unistd.h	1.27    -> 1.28   
#	include/asm-sparc64/unistd.h	1.26    -> 1.27   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/04	dave@thedillows.org	1.1549
# Remove duplicate syscall number for AIO io_destroy() call. Cannot just
# renumber to 272, per WARNING comment just after the list.
# --------------------------------------------
#
diff -Nru a/include/asm-sparc/unistd.h b/include/asm-sparc/unistd.h
--- a/include/asm-sparc/unistd.h	Wed Feb  4 01:11:51 2004
+++ b/include/asm-sparc/unistd.h	Wed Feb  4 01:11:51 2004
@@ -285,7 +285,7 @@
 #define __NR_timer_create	266
 /* #define __NR_vserver		267 Reserved for VSERVER */
 #define __NR_io_setup		268
-#define __NR_io_destroy		268
+#define __NR_io_destroy		232
 #define __NR_io_submit		269
 #define __NR_io_cancel		270
 #define __NR_io_getevents	271
diff -Nru a/include/asm-sparc64/unistd.h b/include/asm-sparc64/unistd.h
--- a/include/asm-sparc64/unistd.h	Wed Feb  4 01:11:51 2004
+++ b/include/asm-sparc64/unistd.h	Wed Feb  4 01:11:51 2004
@@ -287,7 +287,7 @@
 #define __NR_timer_create	266
 /* #define __NR_vserver		267 Reserved for VSERVER */
 #define __NR_io_setup		268
-#define __NR_io_destroy		268
+#define __NR_io_destroy		232
 #define __NR_io_submit		269
 #define __NR_io_cancel		270
 #define __NR_io_getevents	271

Re: AIO syscall patch, and problem

[David S. Miller]

On 04 Feb 2004 02:02:30 -0500
David Dillow <dave@thedillows.org> wrote:

> The attached patch fixes a mis-numbering of the AIO syscalls under sparc
> and sparc64. I would have simply moved them all down, but then
> io_getevents would be at 272, and the big WARNING comment says we cannot
> use numbers larger than 271, so there you are. I used the oldstat
> syscall, since it was commented out.

There must be more to your patch right?  Just changing the numbers
in the unistd.h files does not fix up the syscall table itself too.

I'll look into this and I may fix it differently.

> The problem is __copy_to_user() is being called with a size of 512, but
> is returning 576. size then underflows in the conditional, and
> aio_complete() eventually gets called with this as its return code,
> hence, -EHOSTDOWN.

I'll look into this too, that's a bad bug.

Re: AIO syscall patch, and problem

[David S. Miller]

On 04 Feb 2004 02:02:30 -0500
David Dillow <dave@thedillows.org> wrote:

> The attached patch fixes a mis-numbering of the AIO syscalls under sparc
> and sparc64.

The syscall tables were right, the header was wrong.  This patch below
is the correct fix.  I don't know how the AIO tests passed for you
with your posted patch, maybe nothing depends upon io_destroy() and
subsequently numbered AIO syscalls doing anything :-)

I'll look at the copy_*_user() issue now.

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/02/06 19:32:56-08:00 davem@nuts.davemloft.net 
#   [SPARC]: Fix AIO syscall numbering.
# 
# include/asm-sparc64/unistd.h
#   2004/02/06 19:30:08-08:00 davem@nuts.davemloft.net +7 -7
#   [SPARC]: Fix AIO syscall numbering.
# 
# include/asm-sparc/unistd.h
#   2004/02/06 19:30:08-08:00 davem@nuts.davemloft.net +7 -7
#   [SPARC]: Fix AIO syscall numbering.
# 
diff -Nru a/include/asm-sparc/unistd.h b/include/asm-sparc/unistd.h
--- a/include/asm-sparc/unistd.h	Fri Feb  6 19:33:12 2004
+++ b/include/asm-sparc/unistd.h	Fri Feb  6 19:33:12 2004
@@ -285,14 +285,14 @@
 #define __NR_timer_create	266
 /* #define __NR_vserver		267 Reserved for VSERVER */
 #define __NR_io_setup		268
-#define __NR_io_destroy		268
-#define __NR_io_submit		269
-#define __NR_io_cancel		270
-#define __NR_io_getevents	271
-/* WARNING: You MAY NOT add syscall numbers larger than 271, since
+#define __NR_io_destroy		269
+#define __NR_io_submit		270
+#define __NR_io_cancel		271
+#define __NR_io_getevents	272
+/* WARNING: You MAY NOT add syscall numbers larger than 272, since
  *          all of the syscall tables in the Sparc kernel are
- *          sized to have 272 entries (starting at zero).  Therefore
- *          find a free slot in the 0-271 range.
+ *          sized to have 273 entries (starting at zero).  Therefore
+ *          find a free slot in the 0-272 range.
  */
 
 #define _syscall0(type,name) \
diff -Nru a/include/asm-sparc64/unistd.h b/include/asm-sparc64/unistd.h
--- a/include/asm-sparc64/unistd.h	Fri Feb  6 19:33:12 2004
+++ b/include/asm-sparc64/unistd.h	Fri Feb  6 19:33:12 2004
@@ -287,14 +287,14 @@
 #define __NR_timer_create	266
 /* #define __NR_vserver		267 Reserved for VSERVER */
 #define __NR_io_setup		268
-#define __NR_io_destroy		268
-#define __NR_io_submit		269
-#define __NR_io_cancel		270
-#define __NR_io_getevents	271
-/* WARNING: You MAY NOT add syscall numbers larger than 271, since
+#define __NR_io_destroy		269
+#define __NR_io_submit		270
+#define __NR_io_cancel		271
+#define __NR_io_getevents	272
+/* WARNING: You MAY NOT add syscall numbers larger than 272, since
  *          all of the syscall tables in the Sparc kernel are
- *          sized to have 272 entries (starting at zero).  Therefore
- *          find a free slot in the 0-271 range.
+ *          sized to have 273 entries (starting at zero).  Therefore
+ *          find a free slot in the 0-272 range.
  */
 
 #define _syscall0(type,name) \

Re: AIO syscall patch, and problem

[David Dillow]

On Fri, 2004-02-06 at 22:36, David S. Miller wrote:
> On 04 Feb 2004 02:02:30 -0500
> David Dillow <dave@thedillows.org> wrote:
> 
> > The attached patch fixes a mis-numbering of the AIO syscalls under sparc
> > and sparc64.
> 
> The syscall tables were right, the header was wrong.  This patch below
> is the correct fix.  I don't know how the AIO tests passed for you
> with your posted patch, maybe nothing depends upon io_destroy() and
> subsequently numbered AIO syscalls doing anything :-)

Teaches me to redo a patch before sending it in.... I did the same thing
you did -- renumber to 272 -- and did all my testing (on sparc64) with
that. I redid the patch with the other numbering to be compatible with
sparc (and subsequently forgot about fixing up the table...) Doh!

And of course, I didn't test the one I sent -- it was trivial! I'll
blame the lure of sleep.... :)