linux1394-user

ioctl get_info: Bad address

[Carl K. <ca...@pe...>]

new box, used it for a weekend of video, no problem. getting back to
making tests:

(veyepar)juser@cnt2:~$ sudo async-test receive 001e8c00004166f5
/dev/fw0: Success
001e8c00004166f5
001e8c00004166f5 65472 65472
ioctl get_info: Bad address

https://gitorious.org/cfk_misc/fwdiag/blobs/master/async-test.c

(veyepar)juser@cnt2:~$ lspci|grep 1394
04:00.0 FireWire (IEEE 1394): VIA Technologies, Inc. VT6315 Series
Firewire Controller (rev 01)
08:00.0 FireWire (IEEE 1394): VIA Technologies, Inc. VT6306/7/8 [Fire
II(M)] IEEE 1394 OHCI Controller (rev 46)

plug in cable, new dmesg:
[  258.595688] irq_handler: 6 callbacks suppressed
[  258.595692] firewire_ohci: isochronous cycle inconsistent
[  258.595752] firewire_core: phy config: card 0, new root=ffc1, gap_count=5
[  259.091178] firewire_core: created device fw2: GUID 0011066600000566, S400
[  259.091295] firewire_core: created device fw3: GUID 001e8c00004166f5, S400

-- 
Carl K

Re: ioctl get_info: Bad address

[Clemens L. <cl...@la...>]

Carl Karsten wrote:
> ioctl get_info: Bad address
> 
> https://gitorious.org/cfk_misc/fwdiag/blobs/master/async-test.c

	int main(int argc, char *argv[])
	{
		struct fw_cdev_get_info get_info;
		struct fw_cdev_event_bus_reset bus_reset;
		...
		get_info.version = 3;
		get_info.rom_length = 0;
		get_info.rom = 0;
		get_info.bus_reset = (u64)&bus_reset;
		get_info.bus_reset_closure = 0;
		if (ioctl(fd, FW_CDEV_IOC_GET_INFO, &get_info) == -1) {
			perror("ioctl get_info");

Which architectures are you using in userspace and in the kernel?


Regards,
Clemens

Re: ioctl get_info: Bad address

[Clemens L. <cl...@la...>]

I wrote:
> 	get_info.bus_reset = (u64)&bus_reset;

Uh-oh.  32-bit gcc 4.4.4 says "cast from pointer to integer of different
size" and sign-extends the value.

firewire-cdev.c's u64_to_uptr() differs from compat_ptr() in that the
latter correctly masks the source pointer to 32 bits.

This is definitely a bug in async-test.c, but shouldn't the kernel use
compat_ptr() for all pointers when is_compat_task()?  u64_to_uptr()
will definitely be wrong on s390 and tile (not that we care about those).


Regards,
Clemens

Re: ioctl get_info: Bad address

[Stefan R. <st...@s5...>]

On Aug 09 Clemens Ladisch wrote:
> I wrote:
> > 	get_info.bus_reset = (u64)&bus_reset;
> 
> Uh-oh.  32-bit gcc 4.4.4 says "cast from pointer to integer of different
> size" and sign-extends the value.

Hm, an old C book here about ANSI C says that your code is correct,
since the compiler would first cast the pointer into an unsigned integer
of the same size as the pointer's, then go on with integer-to-integer
conversion to whatever target type you have.

Gcc C isn't ANSI C of course.  Their site says:  "A cast from pointer to
integer discards most-significant bits if the pointer representation is
larger than the integer type, sign-extends¹ if the pointer representation
is smaller than the integer type, otherwise the bits are unchanged."  "¹)
Future versions of GCC may zero-extend, or use a target-defined ptr_extend
pattern. Do not rely on sign extension."
http://gcc.gnu.org/onlinedocs/gcc/Arrays-and-pointers-implementation.html

I wonder whether C90/C99 turned the ANSI way around or made it an
implementation specific thing.

Anyhow.  Perhaps not entirely by accident, Kristian for example put the
following into libraw1394's fw-cdev backend:

#define ptr_to_u64(p) ((__u64)(unsigned long)(p))
#define u64_to_ptr(p) ((void *)(unsigned long)(p))

Which looks good to me.

By sheer luck or maybe because I was warned by gcc on x86-32, the simple C
examples in http://user.in-berlin.de/~s5r6/linux1394/utils/ cast to
unsigned long first, too.

> firewire-cdev.c's u64_to_uptr() differs from compat_ptr() in that the
> latter correctly masks the source pointer to 32 bits.
> 
> This is definitely a bug in async-test.c, but shouldn't the kernel use
> compat_ptr() for all pointers when is_compat_task()?  u64_to_uptr()
> will definitely be wrong on s390 and tile (not that we care about those).

Indeed we don't care about s390; they don't have PCI and we don't have any
non-PCI controller driver.

Tile does support PCI (Express), but the userbase of FireWire on Tile (&&
32 bit userland on 64 bit kernel) has silently put up with our bugs so far.

Let's pretend for a minute that we do care:
---
 drivers/firewire/core-cdev.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -214,19 +214,37 @@ struct outbound_phy_packet_event {
 struct inbound_phy_packet_event {
 	struct event event;
 	struct fw_cdev_event_phy_packet phy_packet;
 };
 
+#ifdef CONFIG_COMPAT
+static inline void __user *u64_to_uptr(__u64 value)
+{
+	if (is_compat_task())
+		return compat_ptr(value);
+	else
+		return (void __user *)(unsigned long)value;
+}
+
+static inline __u64 uptr_to_u64(void __user *ptr)
+{
+	if (is_compat_task())
+		return ptr_to_compat(ptr);
+	else
+		return (__u64)(unsigned long)ptr;
+}
+#else
 static inline void __user *u64_to_uptr(__u64 value)
 {
 	return (void __user *)(unsigned long)value;
 }
 
 static inline __u64 uptr_to_u64(void __user *ptr)
 {
 	return (__u64)(unsigned long)ptr;
 }
+#endif /* CONFIG_COMPAT */
 
 static int fw_device_op_open(struct inode *inode, struct file *file)
 {
 	struct fw_device *device;
 	struct client *client;


gcc 4.4.5 x86-64 makes this of it:

			size before	size after	delta
---------------------------------------------------------------
CONFIG_CC_OPTIMIZE_FOR_SIZE=y
core-cdev.s		342946		352407		+9461
core-cdev.o		126544		130600		+4056
firewire-core.ko	454874		458930		+4056
---------------------------------------------------------------
CONFIG_CC_OPTIMIZE_FOR_SIZE=n
core-cdev.s		369209		379158		+9949
core-cdev.o		133488		137616		+4128
firewire-core.ko	481898		486002		+4104

Is it worthwhile?
-- 
Stefan Richter
-=====-==-== =--- -=--=
http://arcgraph.de/sr/

Re: ioctl get_info: Bad address

[Stefan R. <st...@s5...>]

On Aug 09 Stefan Richter wrote:
> +#ifdef CONFIG_COMPAT
> +static inline void __user *u64_to_uptr(__u64 value)
> +{
> +	if (is_compat_task())
> +		return compat_ptr(value);
> +	else
> +		return (void __user *)(unsigned long)value;
> +}
> +
> +static inline __u64 uptr_to_u64(void __user *ptr)
> +{
> +	if (is_compat_task())
> +		return ptr_to_compat(ptr);
> +	else
> +		return (__u64)(unsigned long)ptr;
> +}
[...]
> gcc 4.4.5 x86-64 makes this of it:
> 
> 			size before	size after	delta
> ---------------------------------------------------------------
> CONFIG_CC_OPTIMIZE_FOR_SIZE=y
> core-cdev.s		342946		352407		+9461
> core-cdev.o		126544		130600		+4056
> firewire-core.ko	454874		458930		+4056
> ---------------------------------------------------------------
> CONFIG_CC_OPTIMIZE_FOR_SIZE=n
> core-cdev.s		369209		379158		+9949
> core-cdev.o		133488		137616		+4128
> firewire-core.ko	481898		486002		+4104
> 
> Is it worthwhile?

PS:  Same result if the "inline"s are omitted.
CONFIG_OPTIMIZE_INLINING=n
-- 
Stefan Richter
-=====-==-== =--- -=--=
http://arcgraph.de/sr/

Re: ioctl get_info: Bad address

[Clemens L. <cl...@la...>]

Stefan Richter wrote:
> On Aug 09 Clemens Ladisch wrote:
> > I wrote:
> > > 	get_info.bus_reset = (u64)&bus_reset;
> > 
> > Uh-oh.  32-bit gcc 4.4.4 says "cast from pointer to integer of different
> > size" and sign-extends the value.
> 
> Hm, an old C book here about ANSI C says that your code is correct,
> since the compiler would first cast the pointer into an unsigned integer
> of the same size as the pointer's, then go on with integer-to-integer
> conversion to whatever target type you have.
> [...]
> I wonder whether C90/C99 turned the ANSI way around or made it an
> implementation specific thing.

Both ISO C90 and C99 are identical with the respective ANSI standards.

latest C99 draft: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
6.3.2.3 6:
| Any pointer type may be converted to an integer type.
| Except [for a null pointer], the result is implementation-defined.

C90 draft (third public review): http://flash-gordon.me.uk/ansi.c.txt
3.3.4:
| A pointer may be converted to an integral type.  The size of integer
| required and the result are implementation-defined.  [...]
| An arbitrary integer may be converted to a pointer.  The result is
| implementation-defined./37/
| 
| 37. The mapping functions for converting a pointer to an integer or
| an integer to a pointer are intended to be consistent with the
| addressing structure of the execution environment.

C90 Rationale for 3.3.4:
| Nothing portable can be said about casting integers to pointers, or
| vice versa, since the two are now incommensurate.
| The definition of these conversions adopted in the Standard resembles
| that in the Base Document, but with several significant differences.
| The Base Document required that a pointer successfully converted to
| an integer must be guaranteed to be convertible back to the same
| pointer. This integer-to-pointer conversion is now specified as
| implementation-defined. While a high-quality implementation would
| preserve the same address value whenever possible, it was considered
| impractical to require that the identical representation be preserved.
| The Committee noted that, on some current machine implementations,
| identical representations are required for efficient code generation
| for pointer comparisons and arithmetic operations.
(The "Base Document" is Ritchie's "The C Reference Manual", which was
also published as appendix A of the old K&R.)

> Anyhow.  Perhaps not entirely by accident, Kristian for example put the
> following into libraw1394's fw-cdev backend:
> 
> #define ptr_to_u64(p) ((__u64)(unsigned long)(p))
> #define u64_to_ptr(p) ((void *)(unsigned long)(p))
> 
> Which looks good to me.

Because in Linux, pointers are guaranteed to fit into unsigned long.
C99 has uintptr_t for this (but says it's optional because such
an integer type might not exist).

> > firewire-cdev.c's u64_to_uptr() differs from compat_ptr() in that the
> > latter correctly masks the source pointer to 32 bits.
> > 
> > This is definitely a bug in async-test.c, but shouldn't the kernel use
> > compat_ptr() for all pointers when is_compat_task()?  u64_to_uptr()
> > will definitely be wrong on s390 and tile (not that we care about those).
> 
> Indeed we don't care about s390; they don't have PCI and we don't have any
> non-PCI controller driver.
> 
> Tile does support PCI (Express), but the userbase of FireWire on Tile (&&
> 32 bit userland on 64 bit kernel) has silently put up with our bugs so far.

arch/tile/Kconfig:
| # Please note: TILE-Gx support is not yet finalized; this is
| # the preliminary support.  TILE-Gx drivers are only provided
| # with the alpha or beta test versions for Tilera customers.
| config TILEGX
| 	depends on EXPERIMENTAL
| 	bool "Building with TILE-Gx (64-bit) compiler and toolchain"

> Let's pretend for a minute that we do care:
> ...
> +#ifdef CONFIG_COMPAT
> +static inline void __user *u64_to_uptr(__u64 value)

Nitpick: __u64 is meant to be used (only) in userspace-visible kernel
headers, because u64 is not a reserved identifier in C9x.  Kernel code
can use u64.

> +{
> +	if (is_compat_task())
> +		return compat_ptr(value);
> ...
> 
> gcc 4.4.5 x86-64 makes this of it:
> 
> 			size before	size after	delta
> ---------------------------------------------------------------
> core-cdev.s		342946		352407		+9461
> core-cdev.o		126544		130600		+4056

Is this with debug information?  Because I cannot see how this many
bytes could be actual code, even with inlining (is_compat_task()'s
current_thread_info() call should end up as one pointer read).

So I'd guess the actual cost (.code section) is not nearly that high.

> Is it worthwhile?

Arch-independent code should not depend on knowledge of compat_ptr()'s
implementation details.


Regards,
Clemens

Re: ioctl get_info: Bad address

[Stefan R. <st...@s5...>]

On Aug 10 Clemens Ladisch wrote:
> Stefan Richter wrote:
> > 			size before	size after	delta
> > ---------------------------------------------------------------
> > core-cdev.s		342946		352407		+9461
> > core-cdev.o		126544		130600		+4056
> 
> Is this with debug information?

Yes it is; not sure why I didn't go on to test without it yesterday.

> Because I cannot see how this many
> bytes could be actual code, even with inlining (is_compat_task()'s
> current_thread_info() call should end up as one pointer read).
> 
> So I'd guess the actual cost (.code section) is not nearly that high.

FWIW, this is from one of the six u64_to_uptr call sites,
ioctl_get_info():

==== before:

.LBB942:
	.loc 1 439 0
	movq	8(%rbx), %rax	# <variable>.device, temp.1345
	mov	4(%r12), %edx	# <variable>.get_info.rom_length, <variable>.get_info.rom_length
	movq	752(%rax), %r15	# <variable>.config_rom_length, <variable>.config_rom_length
	movq	744(%rax), %r13	# <variable>.config_rom, D.30770
	salq	$2, %r15	#, tmp77
	cmpq	%rdx, %r15	# <variable>.get_info.rom_length, tmp77
	cmova	%rdx, %r15	# tmp77,, <variable>.get_info.rom_length, tmp77
	call	might_fault	#
	movq	%r13, %rsi	# D.30770,
	movl	%r15d, %edx	# tmp77,
	movq	%r14, %rdi	# D.30762,
	call	_copy_to_user	#

==== after:

.LBB1062:
	.loc 1 457 0
	movq	8(%rbx), %rcx	# <variable>.device, temp.1345
	mov	4(%r12), %r15d	# <variable>.get_info.rom_length, D.30774
	mov	%eax, %r13d	# D.30771, D.30771
	movq	752(%rcx), %rdx	# <variable>.config_rom_length,
	movq	%rdx, -104(%rbp)	#, %sfp
	movq	744(%rcx), %r14	# <variable>.config_rom, D.30779
	movq %gs:kernel_stack,%rdx	#, pfo_ret__
	testb	$2, -8132(%rdx)	#, <variable>.status
	cmove	%rax, %r13	# D.30771,, D.30771, D.33010
	call	might_fault	#
	movq	-104(%rbp), %rdx	# %sfp, tmp84
	movq	%r13, %rdi	# D.33010,
	movq	%r14, %rsi	# D.30779,
	salq	$2, %rdx	#, tmp84
	cmpq	%r15, %rdx	# D.30774, tmp84
	cmova	%r15, %rdx	# tmp84,, D.30774, tmp84
	call	_copy_to_user	#

====

And this appear to be the two uptr_to_u64 call sites:

==== before:

.L197:
	.loc 1 1055 0
	movq	(%rbx), %r14	# <variable>.queue_iso.packets, p
	mov	16(%rbx), %eax	# <variable>.queue_iso.size, D.31346
	movq %gs:kernel_stack,%rcx	#, pfo_ret__
	movq	%r14, %rdx	# p, roksum
	add %rax,%rdx ; sbb %rsi,%rsi ; cmp %rdx,-8120(%rcx) ; sbb $0,%rsi	# D.31346, roksum,, <variable>.addr_limit.seg

==== after:

.L197:
	.loc 1 1073 0
	movq	(%rbx), %rax	# <variable>.queue_iso.packets, D.31353
	movq %gs:kernel_stack,%rcx	#, pfo_ret__
	testb	$2, -8132(%rcx)	#, <variable>.status
	mov	%eax, %r14d	# D.31353, D.31353
	cmove	%rax, %r14	# D.31353,, D.31353, p
	mov	16(%rbx), %eax	# <variable>.queue_iso.size, D.31355
	movq	%r14, %rdx	# p, roksum
	add %rax,%rdx ; sbb %rsi,%rsi ; cmp %rdx,-8120(%rcx) ; sbb $0,%rsi	# D.31355, roksum,, <variable>.addr_limit.seg

====

> > Is it worthwhile?
> 
> Arch-independent code should not depend on knowledge of compat_ptr()'s
> implementation details.

If we don't use compat_ptr(), we don't depend on its details. ;-)
-- 
Stefan Richter
-=====-==-== =--- -=-=-
http://arcgraph.de/sr/

Re: ioctl get_info: Bad address

[Carl K. <ca...@pe...>]

On Tue, Aug 9, 2011 at 10:32 AM, Clemens Ladisch <cl...@la...> wrote:
> Carl Karsten wrote:
>> ioctl get_info: Bad address
>>
>> https://gitorious.org/cfk_misc/fwdiag/blobs/master/async-test.c
>
>        int main(int argc, char *argv[])
>        {
>                struct fw_cdev_get_info get_info;
>                struct fw_cdev_event_bus_reset bus_reset;
>                ...
>                get_info.version = 3;
>                get_info.rom_length = 0;
>                get_info.rom = 0;
>                get_info.bus_reset = (u64)&bus_reset;
>                get_info.bus_reset_closure = 0;
>                if (ioctl(fd, FW_CDEV_IOC_GET_INFO, &get_info) == -1) {
>                        perror("ioctl get_info");
>
> Which architectures are you using in userspace and in the kernel?
>
>

(veyepar)juser@cnt2:~$ file /sbin/async-test
/sbin/async-test: ELF 32-bit LSB executable, Intel 80386, version 1
(GNU/Linux), statically linked, for GNU/Linux 2.6.15, not stripped

(veyepar)juser@cnt2:~$ uname -a
Linux cnt2 2.6.38-11-generic #47-Ubuntu SMP Fri Jul 15 19:27:09 UTC
2011 x86_64 x86_64 x86_64 GNU/Linux


-- 
Carl K

Re: ioctl get_info: Bad address

[Clemens L. <cl...@la...>]

Stefan Richter wrote:
> On Aug 10 Clemens Ladisch wrote:
> > So I'd guess the actual cost (.code section) is not nearly that high.
> 
> FWIW, this is from one of the six u64_to_uptr call sites,
> ioctl_get_info():
> 	...
> 	mov	%eax, %r13d	# D.30771, D.30771
> 	movq %gs:kernel_stack,%rdx	#, pfo_ret__
> 	testb	$2, -8132(%rdx)	#, <variable>.status
> 	cmove	%rax, %r13	# D.30771,, D.30771, D.33010

Bah, the bytes of these few commands are not worth counting.  :-)

> And this appear to be the two uptr_to_u64 call sites: ...

Where current_thread_info() was _already_ being used.

> > > Is it worthwhile?

Definitely.


Carl, if you're still reading this: Stefan's kernel patch will fix your
problem, but you could just as well replace (u64)&something with
(unsigned long)&something in async-test.c.


Regards,
Clemens

Re: ioctl get_info: Bad address

[Stefan R. <st...@s5...>]

On Aug 10 Clemens Ladisch wrote:
> Bah, the bytes of these few commands are not worth counting.  :-)

# CONFIG_DEBUG_INFO is not set
CONFIG_CC_OPTIMIZE_FOR_SIZE=y
# CONFIG_OPTIMIZE_INLINING is not set

			size before	size after	delta
---------------------------------------------------------------
core-cdev.s		87588		89244		+1656
core-cdev.o		25664		25984		 +320

(It's all in slow paths and doesn't affect those who don't set
CONFIG_COMPAT.)

> > > > Is it worthwhile?
> 
> Definitely.

OK, I send a variant to -devel.
-- 
Stefan Richter
-=====-==-== =--- -=-=-
http://arcgraph.de/sr/

Re: ioctl get_info: Bad address

[Carl K. <ca...@pe...>]

On Wed, Aug 10, 2011 at 12:05 PM, Clemens Ladisch <cl...@la...> wrote:
> Stefan Richter wrote:
>> On Aug 10 Clemens Ladisch wrote:
>> > So I'd guess the actual cost (.code section) is not nearly that high.
>>
>> FWIW, this is from one of the six u64_to_uptr call sites,
>> ioctl_get_info():
>>       ...
>>       mov     %eax, %r13d     # D.30771, D.30771
>>       movq %gs:kernel_stack,%rdx      #, pfo_ret__
>>       testb   $2, -8132(%rdx) #, <variable>.status
>>       cmove   %rax, %r13      # D.30771,, D.30771, D.33010
>
> Bah, the bytes of these few commands are not worth counting.  :-)
>
>> And this appear to be the two uptr_to_u64 call sites: ...
>
> Where current_thread_info() was _already_ being used.
>
>> > > Is it worthwhile?
>
> Definitely.
>
>
> Carl, if you're still reading this: Stefan's kernel patch will fix your
> problem, but you could just as well replace (u64)&something with
> (unsigned long)&something in async-test.c.
>

This one, right?

313     get_info.bus_reset = (u64)&bus_reset;


-- 
Carl K

Re: ioctl get_info: Bad address

[Clemens L. <cl...@la...>]

Carl Karsten wrote:
>Clemens Ladisch wrote:
>> replace (u64)&something with
>> (unsigned long)&something in async-test.c.
>
>This one, right?
>
>313     get_info.bus_reset = (u64)&bus_reset;

Yes.


Regards,
Clemens