[PATCH v3 0/3] preadv & pwritev syscalls.

[PATCH v3 0/3] preadv & pwritev syscalls.

[Gerd Hoffmann]

  Hi,

Next revision of the patch series, with the alignment issue fixed by
swapping the last two arguments as suggested by arch maintainers.

I've dropped the now-obsolete wrappers for mips.  Ralf, please
double-check.

thanks,
  Gerd

Signed-off-by: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/compat.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index e5f49f5..aab2234 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1187,6 +1187,9 @@ compat_sys_readv(unsigned long fd, const struct compat_iovec __user *vec, unsign
 	ret = compat_do_readv_writev(READ, file, vec, vlen, &file->f_pos);
 
 out:
+	if (ret > 0)
+		add_rchar(current, ret);
+	inc_syscr(current);
 	fput(file);
 	return ret;
 }
@@ -1210,6 +1213,9 @@ compat_sys_writev(unsigned long fd, const struct compat_iovec __user *vec, unsig
 	ret = compat_do_readv_writev(WRITE, file, vec, vlen, &file->f_pos);
 
 out:
+	if (ret > 0)
+		add_wchar(current, ret);
+	inc_syscw(current);
 	fput(file);
 	return ret;
 }
-- 
1.5.6.5

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/3] Add missing accounting calls to compat_sys_{readv,writev}.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 fs/compat.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index e5f49f5..aab2234 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1187,6 +1187,9 @@ compat_sys_readv(unsigned long fd, const struct compat_iovec __user *vec, unsign
 	ret = compat_do_readv_writev(READ, file, vec, vlen, &file->f_pos);
 
 out:
+	if (ret > 0)
+		add_rchar(current, ret);
+	inc_syscr(current);
 	fput(file);
 	return ret;
 }
@@ -1210,6 +1213,9 @@ compat_sys_writev(unsigned long fd, const struct compat_iovec __user *vec, unsig
 	ret = compat_do_readv_writev(WRITE, file, vec, vlen, &file->f_pos);
 
 out:
+	if (ret > 0)
+		add_wchar(current, ret);
+	inc_syscw(current);
 	fput(file);
 	return ret;
 }
-- 
1.5.6.5

[PATCH v3 2/3] Add preadv and pwritev system calls.

[Gerd Hoffmann]

This patch adds preadv and pwritev system calls.  These syscalls are a
pretty straightforward combination of pread and readv (same for write).
They are quite useful for doing vectored I/O in threaded applications.
Using lseek+readv instead opens race windows you'll have to plug with
locking.

Other systems have such system calls too, for example NetBSD, check
here: http://www.daemon-systems.org/man/preadv.2.html

The application-visible interface provided by glibc should look like
this to be compatible to the existing implementations in the *BSD family:

  ssize_t preadv(int d, const struct iovec *iov, int iovcnt, off_t offset);
  ssize_t pwritev(int d, const struct iovec *iov, int iovcnt, off_t offset);

Note that the system call uses another argument ordering (last two args
swapped) to avoid unaligned 64bit arguments on the stack.

The patch sports the actual system call implementation and the windup in
the x86 system call tables.  Other archs follow as separate patches.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 arch/x86/ia32/ia32entry.S          |    2 +
 arch/x86/include/asm/unistd_32.h   |    2 +
 arch/x86/include/asm/unistd_64.h   |    4 ++
 arch/x86/kernel/syscall_table_32.S |    2 +
 fs/compat.c                        |   61 ++++++++++++++++++++++++++++++++++++
 fs/read_write.c                    |   48 ++++++++++++++++++++++++++++
 6 files changed, 119 insertions(+), 0 deletions(-)

diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index 256b00b..9a8501b 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -826,4 +826,6 @@ ia32_sys_call_table:
 	.quad sys_dup3			/* 330 */
 	.quad sys_pipe2
 	.quad sys_inotify_init1
+	.quad compat_sys_preadv
+	.quad compat_sys_pwritev
 ia32_syscall_end:
diff --git a/arch/x86/include/asm/unistd_32.h b/arch/x86/include/asm/unistd_32.h
index f2bba78..6e72d74 100644
--- a/arch/x86/include/asm/unistd_32.h
+++ b/arch/x86/include/asm/unistd_32.h
@@ -338,6 +338,8 @@
 #define __NR_dup3		330
 #define __NR_pipe2		331
 #define __NR_inotify_init1	332
+#define __NR_preadv		333
+#define __NR_pwritev		334
 
 #ifdef __KERNEL__
 
diff --git a/arch/x86/include/asm/unistd_64.h b/arch/x86/include/asm/unistd_64.h
index d2e415e..f818294 100644
--- a/arch/x86/include/asm/unistd_64.h
+++ b/arch/x86/include/asm/unistd_64.h
@@ -653,6 +653,10 @@ __SYSCALL(__NR_dup3, sys_dup3)
 __SYSCALL(__NR_pipe2, sys_pipe2)
 #define __NR_inotify_init1			294
 __SYSCALL(__NR_inotify_init1, sys_inotify_init1)
+#define __NR_preadv				295
+__SYSCALL(__NR_preadv, sys_preadv)
+#define __NR_pwritev				296
+__SYSCALL(__NR_pwritev, sys_pwritev)
 
 
 #ifndef __NO_STUBS
diff --git a/arch/x86/kernel/syscall_table_32.S b/arch/x86/kernel/syscall_table_32.S
index d44395f..a1a5506 100644
--- a/arch/x86/kernel/syscall_table_32.S
+++ b/arch/x86/kernel/syscall_table_32.S
@@ -332,3 +332,5 @@ ENTRY(sys_call_table)
 	.long sys_dup3			/* 330 */
 	.long sys_pipe2
 	.long sys_inotify_init1
+	.long sys_preadv
+	.long sys_pwritev
diff --git a/fs/compat.c b/fs/compat.c
index aab2234..0837a1a 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1220,6 +1220,67 @@ out:
 	return ret;
 }
 
+asmlinkage ssize_t
+compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
+                  loff_t pos, unsigned long vlen)
+{
+	struct file *file;
+	ssize_t ret = -EBADF;
+
+	if (pos < 0)
+		return -EINVAL;
+
+	file = fget(fd);
+	if (!file)
+		return -EBADF;
+
+	if (!(file->f_mode & FMODE_READ))
+		goto out;
+
+	ret = -EINVAL;
+	if (!file->f_op || (!file->f_op->aio_read && !file->f_op->read))
+		goto out;
+
+	ret = compat_do_readv_writev(READ, file, vec, vlen, &pos);
+
+out:
+	if (ret > 0)
+		add_rchar(current, ret);
+	inc_syscr(current);
+	fput(file);
+	return ret;
+}
+
+asmlinkage ssize_t
+compat_sys_pwritev(unsigned long fd, const struct compat_iovec __user *vec,
+                   loff_t pos, unsigned long vlen)
+{
+	struct file *file;
+	ssize_t ret = -EBADF;
+
+	if (pos < 0)
+		return -EINVAL;
+
+	file = fget(fd);
+	if (!file)
+		return -EBADF;
+	if (!(file->f_mode & FMODE_WRITE))
+		goto out;
+
+	ret = -EINVAL;
+	if (!file->f_op || (!file->f_op->aio_write && !file->f_op->write))
+		goto out;
+
+	ret = compat_do_readv_writev(WRITE, file, vec, vlen, &pos);
+
+out:
+	if (ret > 0)
+		add_wchar(current, ret);
+	inc_syscw(current);
+	fput(file);
+	return ret;
+}
+
 asmlinkage long
 compat_sys_vmsplice(int fd, const struct compat_iovec __user *iov32,
 		    unsigned int nr_segs, unsigned int flags)
diff --git a/fs/read_write.c b/fs/read_write.c
index 969a6d9..3fe761d 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -701,6 +701,54 @@ sys_writev(unsigned long fd, const struct iovec __user *vec, unsigned long vlen)
 	return ret;
 }
 
+asmlinkage ssize_t sys_preadv(unsigned long fd, const struct iovec __user *vec,
+                              loff_t pos, unsigned long vlen)
+{
+	struct file *file;
+	ssize_t ret = -EBADF;
+	int fput_needed;
+
+	if (pos < 0)
+		return -EINVAL;
+
+	file = fget_light(fd, &fput_needed);
+	if (file) {
+		ret = -ESPIPE;
+		if (file->f_mode & FMODE_PREAD)
+			ret = vfs_readv(file, vec, vlen, &pos);
+		fput_light(file, fput_needed);
+	}
+
+	if (ret > 0)
+		add_rchar(current, ret);
+	inc_syscr(current);
+	return ret;
+}
+
+asmlinkage ssize_t sys_pwritev(unsigned long fd, const struct iovec __user *vec,
+                               loff_t pos, unsigned long vlen)
+{
+	struct file *file;
+	ssize_t ret = -EBADF;
+	int fput_needed;
+
+	if (pos < 0)
+		return -EINVAL;
+
+	file = fget_light(fd, &fput_needed);
+	if (file) {
+		ret = -ESPIPE;
+		if (file->f_mode & FMODE_PWRITE)
+			ret = vfs_writev(file, vec, vlen, &pos);
+		fput_light(file, fput_needed);
+	}
+
+	if (ret > 0)
+		add_wchar(current, ret);
+	inc_syscw(current);
+	return ret;
+}
+
 static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 			   size_t count, loff_t max)
 {
-- 
1.5.6.5

[PATCH v3 3/3] MIPS: Add preadv(2) and pwritev(2) syscalls.

[Gerd Hoffmann]

From: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>

From: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
Signed-off-by: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
Signed-off-by: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/mips/include/asm/unistd.h |   18 ++++++++++++------
 arch/mips/kernel/scall32-o32.S |    2 ++
 arch/mips/kernel/scall64-64.S  |    2 ++
 arch/mips/kernel/scall64-n32.S |    2 ++
 arch/mips/kernel/scall64-o32.S |    2 ++
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/unistd.h b/arch/mips/include/asm/unistd.h
index a73e153..4000501 100644
--- a/arch/mips/include/asm/unistd.h
+++ b/arch/mips/include/asm/unistd.h
@@ -350,16 +350,18 @@
 #define __NR_dup3			(__NR_Linux + 327)
 #define __NR_pipe2			(__NR_Linux + 328)
 #define __NR_inotify_init1		(__NR_Linux + 329)
+#define __NR_preadv			(__NR_Linux + 330)
+#define __NR_pwritev			(__NR_Linux + 331)
 
 /*
  * Offset of the last Linux o32 flavoured syscall
  */
-#define __NR_Linux_syscalls		329
+#define __NR_Linux_syscalls		331
 
 #endif /* _MIPS_SIM == _MIPS_SIM_ABI32 */
 
 #define __NR_O32_Linux			4000
-#define __NR_O32_Linux_syscalls		329
+#define __NR_O32_Linux_syscalls		331
 
 #if _MIPS_SIM == _MIPS_SIM_ABI64
 
@@ -656,16 +658,18 @@
 #define __NR_dup3			(__NR_Linux + 286)
 #define __NR_pipe2			(__NR_Linux + 287)
 #define __NR_inotify_init1		(__NR_Linux + 288)
+#define __NR_preadv			(__NR_Linux + 289)
+#define __NR_pwritev			(__NR_Linux + 290)
 
 /*
  * Offset of the last Linux 64-bit flavoured syscall
  */
-#define __NR_Linux_syscalls		288
+#define __NR_Linux_syscalls		290
 
 #endif /* _MIPS_SIM == _MIPS_SIM_ABI64 */
 
 #define __NR_64_Linux			5000
-#define __NR_64_Linux_syscalls		288
+#define __NR_64_Linux_syscalls		290
 
 #if _MIPS_SIM == _MIPS_SIM_NABI32
 
@@ -966,16 +970,18 @@
 #define __NR_dup3			(__NR_Linux + 290)
 #define __NR_pipe2			(__NR_Linux + 291)
 #define __NR_inotify_init1		(__NR_Linux + 292)
+#define __NR_preadv			(__NR_Linux + 293)
+#define __NR_pwritev			(__NR_Linux + 294)
 
 /*
  * Offset of the last N32 flavoured syscall
  */
-#define __NR_Linux_syscalls		292
+#define __NR_Linux_syscalls		294
 
 #endif /* _MIPS_SIM == _MIPS_SIM_NABI32 */
 
 #define __NR_N32_Linux			6000
-#define __NR_N32_Linux_syscalls		292
+#define __NR_N32_Linux_syscalls		294
 
 #ifdef __KERNEL__
 
diff --git a/arch/mips/kernel/scall32-o32.S b/arch/mips/kernel/scall32-o32.S
index d0916a5..4a8b3e8 100644
--- a/arch/mips/kernel/scall32-o32.S
+++ b/arch/mips/kernel/scall32-o32.S
@@ -650,6 +650,8 @@ einval:	li	v0, -ENOSYS
 	sys	sys_dup3		3
 	sys	sys_pipe2		2
 	sys	sys_inotify_init1	1
+	sys	sys_preadv		6	/* 4330 */
+	sys	sys_pwritev		6
 	.endm
 
 	/* We pre-compute the number of _instruction_ bytes needed to
diff --git a/arch/mips/kernel/scall64-64.S b/arch/mips/kernel/scall64-64.S
index a9e1716..217e3ce 100644
--- a/arch/mips/kernel/scall64-64.S
+++ b/arch/mips/kernel/scall64-64.S
@@ -487,4 +487,6 @@ sys_call_table:
 	PTR	sys_dup3
 	PTR	sys_pipe2
 	PTR	sys_inotify_init1
+	PTR	sys_preadv
+	PTR	sys_pwritev			/* 5390 */
 	.size	sys_call_table,.-sys_call_table
diff --git a/arch/mips/kernel/scall64-n32.S b/arch/mips/kernel/scall64-n32.S
index 30f3b63..f340963 100644
--- a/arch/mips/kernel/scall64-n32.S
+++ b/arch/mips/kernel/scall64-n32.S
@@ -413,4 +413,6 @@ EXPORT(sysn32_call_table)
 	PTR	sys_dup3			/* 5290 */
 	PTR	sys_pipe2
 	PTR	sys_inotify_init1
+	PTR	sys_preadv
+	PTR	sys_pwritev
 	.size	sysn32_call_table,.-sysn32_call_table
diff --git a/arch/mips/kernel/scall64-o32.S b/arch/mips/kernel/scall64-o32.S
index fefef4a..b1d281a 100644
--- a/arch/mips/kernel/scall64-o32.S
+++ b/arch/mips/kernel/scall64-o32.S
@@ -533,4 +533,6 @@ sys_call_table:
 	PTR	sys_dup3
 	PTR	sys_pipe2
 	PTR	sys_inotify_init1
+	PTR	compat_sys_preadv		/* 4330 */
+	PTR	compat_sys_pwritev
 	.size	sys_call_table,.-sys_call_table
-- 
1.5.6.5

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[Ralf Baechle]

On Mon, Dec 15, 2008 at 12:36:14PM +0100, Gerd Hoffmann wrote:

> Next revision of the patch series, with the alignment issue fixed by
> swapping the last two arguments as suggested by arch maintainers.
> 
> I've dropped the now-obsolete wrappers for mips.  Ralf, please
> double-check.

It fixes the alignment issue but still won't work; on MIPS 32-bit userspace
will pass the 64-bit argument in two registers but the 64-bit kernel code
will assume it to be passed in a single registers.  It'd be ugly but passing
a pointer to a 64-bit argument would solve the issue; something like this:

sys_preadv(unsigned long fd, const struct iovec __user *vec,
                  unsigned long vlen, loff_t __user *pos);
compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
                  unsigned long vlen, loff_t __user *pos);

sys_splice uses loff_t __user * arguments as well and that's why it's
doesn't need any compat wrapper.

I'm surprised this works for x86; does x86-64 code really expect 64-bit
arguments as 2 32-bit arguments?

Patch 1/3 looks like it's a bug fix and you may want to submit this
separate from the remainder of the series for 2.6.28?

  Ralf
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[David Miller]

From: Ralf Baechle <ralf@linux-mips.org>
Date: Mon, 15 Dec 2008 16:03:11 +0000

> On Mon, Dec 15, 2008 at 12:36:14PM +0100, Gerd Hoffmann wrote:
> 
> > Next revision of the patch series, with the alignment issue fixed by
> > swapping the last two arguments as suggested by arch maintainers.
> > 
> > I've dropped the now-obsolete wrappers for mips.  Ralf, please
> > double-check.
> 
> It fixes the alignment issue but still won't work; on MIPS 32-bit userspace
> will pass the 64-bit argument in two registers but the 64-bit kernel code
> will assume it to be passed in a single registers.

Same on 32-bit sparc, and I assume 32-bit powerpc as well.

> I'm surprised this works for x86; does x86-64 code really expect 64-bit
> arguments as 2 32-bit arguments?

Args get pushed onto the stack, so perhaps it works by accident :)

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[Gerd Hoffmann]

Ralf Baechle wrote:
> On Mon, Dec 15, 2008 at 12:36:14PM +0100, Gerd Hoffmann wrote:
> 
>> Next revision of the patch series, with the alignment issue fixed by
>> swapping the last two arguments as suggested by arch maintainers.
>>
>> I've dropped the now-obsolete wrappers for mips.  Ralf, please
>> double-check.
> 
> It fixes the alignment issue but still won't work; on MIPS 32-bit userspace
> will pass the 64-bit argument in two registers but the 64-bit kernel code
> will assume it to be passed in a single registers.  It'd be ugly but passing
> a pointer to a 64-bit argument would solve the issue; something like this:
>
> sys_preadv(unsigned long fd, const struct iovec __user *vec,
>                   unsigned long vlen, loff_t __user *pos);
> compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
>                   unsigned long vlen, loff_t __user *pos);

Suggestion from the s390 front was to explicitly pass high and low part
of pos as two arguments.  A bit ugly too, but should work fine as well
and it avoids the user pointer dereference.  What do you think about this?

> sys_splice uses loff_t __user * arguments as well and that's why it's
> doesn't need any compat wrapper.

Well, avoiding the compat wrapper altogether unfortunately isn't going
to work because struct iovec looks different in 32 and 64 bit.

> I'm surprised this works for x86; does x86-64 code really expect 64-bit
> arguments as 2 32-bit arguments?

Args are passed on the stack, not in registers.

> Patch 1/3 looks like it's a bug fix and you may want to submit this
> separate from the remainder of the series for 2.6.28?

Yep, it is a separate bugfix.  Spotted by Christoph while reviewing v1,
but there are no other dependencies to this patch series.

cheers,
  Gerd

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[Ralf Baechle]

On Mon, Dec 15, 2008 at 09:57:24PM +0100, Gerd Hoffmann wrote:

> > It fixes the alignment issue but still won't work; on MIPS 32-bit userspace
> > will pass the 64-bit argument in two registers but the 64-bit kernel code
> > will assume it to be passed in a single registers.  It'd be ugly but passing
> > a pointer to a 64-bit argument would solve the issue; something like this:
> >
> > sys_preadv(unsigned long fd, const struct iovec __user *vec,
> >                   unsigned long vlen, loff_t __user *pos);
> > compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
> >                   unsigned long vlen, loff_t __user *pos);
> 
> Suggestion from the s390 front was to explicitly pass high and low part
> of pos as two arguments.  A bit ugly too, but should work fine as well
> and it avoids the user pointer dereference.  What do you think about this?

That's what the wrapper which you deleted, was doing ;-)  So yes, I like
it.  It just raises one new problem, endianess - are arguments being passed
as low/high or high/low?  On MIPS we've been solving the issue with the
merge_64() macro which is defined depending on the byte order:

#ifdef __MIPSEB__
#define merge_64(r1, r2) ((((r1) & 0xffffffffUL) << 32) + ((r2) & 0xffffffffUL))
#endif
#ifdef __MIPSEL__
#define merge_64(r1, r2) ((((r2) & 0xffffffffUL) << 32) + ((r1) & 0xffffffffUL))
#endif

The actual syscall wrapper could use it like:

asmlinkage int compat_sys_pwritev(unsigned long fd,
       const struct compat_iovec __user *vec,
       unsigned a3, unsigned a4, unsigned long vlen)
{
	loff_t offset = merge_64(a3, a4);
...

If merge_64() was a standard macro in <asm/compat.h> on all architectures,
compat_sys_pwrite() could invoke it directly and the need for the compat
wrapper around the compat wrapper would go away.

> > I'm surprised this works for x86; does x86-64 code really expect 64-bit
> > arguments as 2 32-bit arguments?
> 
> Args are passed on the stack, not in registers.

Same as 32-bit MIPS for argument #4 and up which will be passed on the
stack - but the requirement for passing a long long is for it to be
passed as an aligned pair of arguments and the original patch got
that wrong.

  Ralf

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[Kyle McMartin]

On Tue, Dec 16, 2008 at 04:05:02PM +0000, Ralf Baechle wrote:
> If merge_64() was a standard macro in <asm/compat.h> on all architectures,
> compat_sys_pwrite() could invoke it directly and the need for the compat
> wrapper around the compat wrapper would go away.
> 

+1 for this.

AFAIK, there's issues on s390 and sparc (at least) with sign extension,
that they need to wrap anyway, we should really sort that out in the
generic compat wrappers as well.

cheers, Kyle

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[Gerd Hoffmann]

Ralf Baechle wrote:
> On Mon, Dec 15, 2008 at 09:57:24PM +0100, Gerd Hoffmann wrote:
> 
>>> It fixes the alignment issue but still won't work; on MIPS 32-bit userspace
>>> will pass the 64-bit argument in two registers but the 64-bit kernel code
>>> will assume it to be passed in a single registers.  It'd be ugly but passing
>>> a pointer to a 64-bit argument would solve the issue; something like this:
>>>
>>> sys_preadv(unsigned long fd, const struct iovec __user *vec,
>>>                   unsigned long vlen, loff_t __user *pos);
>>> compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
>>>                   unsigned long vlen, loff_t __user *pos);
>> Suggestion from the s390 front was to explicitly pass high and low part
>> of pos as two arguments.  A bit ugly too, but should work fine as well
>> and it avoids the user pointer dereference.  What do you think about this?
> 
> That's what the wrapper which you deleted, was doing ;-)  So yes, I like
> it.

Yep, but I'm trying to find a way to have it work without per-arch
wrappers ...

> It just raises one new problem, endianess - are arguments being passed
> as low/high or high/low?  On MIPS we've been solving the issue with the
> merge_64() macro which is defined depending on the byte order:
> 
> #ifdef __MIPSEB__
> #define merge_64(r1, r2) ((((r1) & 0xffffffffUL) << 32) + ((r2) & 0xffffffffUL))
> #endif
> #ifdef __MIPSEL__
> #define merge_64(r1, r2) ((((r2) & 0xffffffffUL) << 32) + ((r1) & 0xffffffffUL))
> #endif
> 
> The actual syscall wrapper could use it like:
> 
> asmlinkage int compat_sys_pwritev(unsigned long fd,
>        const struct compat_iovec __user *vec,
>        unsigned a3, unsigned a4, unsigned long vlen)
> {
> 	loff_t offset = merge_64(a3, a4);
> ...

i.e. the ordering of the splitted argument depends on the os endianness?
What is the reason for this?

I'd prefer to have the ordering coded explicitly instead, like this:

asmlinkage int compat_sys_pwritev(unsigned long fd,
       const struct compat_iovec __user *vec, unsigned long vlen,
       unsigned pos_low, unsigned pos_high)
{
	loff_t pos = pos_low | (loff_t)pos_high << 32;
        [ ... ]

cheers,
  Gerd


--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[Kyle McMartin]

On Tue, Dec 16, 2008 at 05:48:58PM +0100, Gerd Hoffmann wrote:
> i.e. the ordering of the splitted argument depends on the os endianness?
> What is the reason for this?
> 

Eh? The splitting will occur at the C ABI level, not as a result of
glibc (though, it could be done that way if you really wanted, but then
you're just moving the wrapper up the chain.)

A 64-bit value on a 32-bit machine will look like, in a register pair,
like it looks in memory, with MSB/LSB ordering.

> I'd prefer to have the ordering coded explicitly instead, like this:
> 
> asmlinkage int compat_sys_pwritev(unsigned long fd,
>        const struct compat_iovec __user *vec, unsigned long vlen,
>        unsigned pos_low, unsigned pos_high)
> {
> 	loff_t pos = pos_low | (loff_t)pos_high << 32;
>         [ ... ]
> 

Sadly this isn't possible without a wrapper unless you can guarantee
it was passed low high instead of high low. Unless you munge it in
userspace, you can't.

regards, Kyle

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[Arnd Bergmann]

On Tuesday 16 December 2008, Kyle McMartin wrote:
> > I'd prefer to have the ordering coded explicitly instead, like this:
> > 
> > asmlinkage int compat_sys_pwritev(unsigned long fd,
> >        const struct compat_iovec __user *vec, unsigned long vlen,
> >        unsigned pos_low, unsigned pos_high)
> > {
> >       loff_t pos = pos_low | (loff_t)pos_high << 32;
> >         [ ... ]
> > 
> 
> Sadly this isn't possible without a wrapper unless you can guarantee
> it was passed low high instead of high low. Unless you munge it in
> userspace, you can't.
> 

Exactly, we have to munge it in userspace anyway because of the
argument ordering requirement we get if we pass it as a single u64.
I vote for the explicit split as well, this will give us a straightforward
wrapper in user space, unlike the swapped arguments that are just asking
for trouble, and will make it easy to have a common compat_sys_pwritev
function.

	Arnd <><

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[Gerd Hoffmann]

Kyle McMartin wrote:
> On Tue, Dec 16, 2008 at 05:48:58PM +0100, Gerd Hoffmann wrote:
>> i.e. the ordering of the splitted argument depends on the os endianness?
>> What is the reason for this?
> 
> Eh? The splitting will occur at the C ABI level, not as a result of
> glibc (though, it could be done that way if you really wanted, but then
> you're just moving the wrapper up the chain.)

Ah, ok.  You'll just declare 64bit arg for userspace, gcc splits it into
two 32bit in native byte order, and then the kernel picks up the two
32bit values and has to reassemble them correctly, right?

The application-visible API must be compatible to the existing
implementations, i.e. this ...

   pread(long fd, struct iovec *vec, long vlen, off_t pos);

... prototype with the unaligned 64bit pos argument (in 32compat case).

Looks like there is no way around wrapping stuff then for the archs
wanting aligned 64bit values.  The only choice we have is to do the
wrap-o-magic in glibc or in the kernel.

I'd tend handle the wrapping in kernel space then because it is less
confusing and we have to wrap only in case the ABI for $arch requires it.

Comments?

  Gerd
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/3] preadv & pwritev syscalls.

[Heiko Carstens]

On Tue, Dec 16, 2008 at 10:34:49PM +0100, Gerd Hoffmann wrote:
> Kyle McMartin wrote:
> > On Tue, Dec 16, 2008 at 05:48:58PM +0100, Gerd Hoffmann wrote:
> >> i.e. the ordering of the splitted argument depends on the os endianness?
> >> What is the reason for this?
> > 
> > Eh? The splitting will occur at the C ABI level, not as a result of
> > glibc (though, it could be done that way if you really wanted, but then
> > you're just moving the wrapper up the chain.)
> 
> Ah, ok.  You'll just declare 64bit arg for userspace, gcc splits it into
> two 32bit in native byte order, and then the kernel picks up the two
> 32bit values and has to reassemble them correctly, right?
> 
> The application-visible API must be compatible to the existing
> implementations, i.e. this ...
> 
>    pread(long fd, struct iovec *vec, long vlen, off_t pos);
> 
> ... prototype with the unaligned 64bit pos argument (in 32compat case).
> 
> Looks like there is no way around wrapping stuff then for the archs
> wanting aligned 64bit values.  The only choice we have is to do the
> wrap-o-magic in glibc or in the kernel.
> 
> I'd tend handle the wrapping in kernel space then because it is less
> confusing and we have to wrap only in case the ABI for $arch requires it.
> 
> Comments?

Please do it just the way that Arnd suggested: explicitly pass the upper
and lower part of loff_t as separate arguments.
It's the most simple approach.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html