[Bluez-devel] [PATCH] Fix abuse of kernel headers.

[Bluez-devel] [PATCH] Fix abuse of kernel headers.

[David Woodhouse]

Thou Shalt Not Use Private Kernel Headers In Userspace.
Thou Shalt Not Use Private Kernel Headers In Userspace.
Thou Shalt Not Use Private Kernel Headers In Userspace.
Thou Shalt Not Use Private Kernel Headers In Userspace.

--- bluez-hcidump-1.5/parser/parser.h~	Tue Oct 15 18:18:45 2002
+++ bluez-hcidump-1.5/parser/parser.h	Fri Apr 25 14:17:39 2003
@@ -29,8 +29,6 @@
 #include <sys/types.h>
 #include <netinet/in.h>
 
-#include <asm/unaligned.h>
-
 struct frame {
 	void	*data;
 	int	data_len;
@@ -102,7 +100,7 @@
 		printf("%*c", (level*2), ' ');
 }
 
-/* get_uXX functions do byte swaping */
+/* get_uXX functions do byte swapping */
 
 static inline __u8 get_u8(struct frame *frm)
 {
@@ -114,28 +112,35 @@
 
 static inline __u16 get_u16(struct frame *frm)
 {
-	__u16 *u16_ptr = frm->ptr;
+	__u8 *u8_ptr = frm->ptr;
 	frm->ptr += 2;
 	frm->len -= 2;
-	return ntohs(get_unaligned(u16_ptr));
+	return (u8_ptr[0] << 8) | u8_ptr[1];
 }
 
 static inline __u32 get_u32(struct frame *frm)
 {
-	__u32 *u32_ptr = frm->ptr;
+	__u8 *u8_ptr = frm->ptr;
 	frm->ptr += 4;
 	frm->len -= 4;
-	return ntohl(get_unaligned(u32_ptr));
+	return (u8_ptr[0] << 24) |(u8_ptr[1] << 16) | (u8_ptr[2] << 8) | u8_ptr[3];
 }
 
 static inline __u64 get_u64(struct frame *frm)
 {
-	__u64 *u64_ptr = frm->ptr;
-	__u64 u64 = get_unaligned(u64_ptr), tmp;
+	__u8 *u8_ptr = frm->ptr;
+	__u64 u64;
+
 	frm->ptr += 8;
 	frm->len -= 8;
-	tmp = ntohl(u64 & 0xffffffff);
-	u64 = (tmp << 32) | ntohl(u64 >> 32);
+	u64 = (__u64)u8_ptr[0] << 56;
+	u64 |= (__u64)u8_ptr[1] << 48;
+	u64 |= (__u64)u8_ptr[2] << 40;
+	u64 |= (__u64)u8_ptr[3] << 32;
+	u64 |= (__u64)u8_ptr[4] << 24;
+	u64 |= (__u64)u8_ptr[5] << 16;
+	u64 |= (__u64)u8_ptr[6] << 8;
+	u64 |= (__u64)u8_ptr[7];
 	return u64;
 }
 




-- 
dwmw2



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Fix abuse of kernel headers.

[Max Krasnyansky]

At 06:25 AM 4/25/2003, David Woodhouse wrote:
>Thou Shalt Not Use Private Kernel Headers In Userspace.
Yes. But asm/unaligned.h is the integral part of the glibc headers.


>-       return ntohs(get_unaligned(u16_ptr));
>+       return (u8_ptr[0] << 8) | u8_ptr[1];
I'd rather see reimplementation of get_unaligned() in hcidump headers 
than this. We have sdp_get/put_unaligned() btw.

Max




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Fix abuse of kernel headers.

[David Woodhouse]

On Fri, 2003-04-25 at 18:43, Max Krasnyansky wrote:
> At 06:25 AM 4/25/2003, David Woodhouse wrote:
> >Thou Shalt Not Use Private Kernel Headers In Userspace.
> Yes. But asm/unaligned.h is the integral part of the glibc headers.

Take a look at the PPC version.

The rule is that you're not permitted to use _anything_ in asm/ or
linux/ directly from userspace. We know people do, but they shouldn't,
and if it breaks they get to keep both pieces. 

> 
> >-       return ntohs(get_unaligned(u16_ptr));
> >+       return (u8_ptr[0] << 8) | u8_ptr[1];
> I'd rather see reimplementation of get_unaligned() in hcidump headers 
> than this. We have sdp_get/put_unaligned() btw.

I looked at that but AFAIK the C standard doesn't actually guarantee the
packed structure method will work, and since we were always byteswapping
in the hcidump case anyway it didn't seem worth implementing a safe
generic version.

-- 
dwmw2




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Fix abuse of kernel headers.

[Max Krasnyansky]

At 02:16 PM 4/25/2003, David Woodhouse wrote:
>On Fri, 2003-04-25 at 18:43, Max Krasnyansky wrote:
>> At 06:25 AM 4/25/2003, David Woodhouse wrote:
>> >Thou Shalt Not Use Private Kernel Headers In Userspace.
>> Yes. But asm/unaligned.h is the integral part of the glibc headers.
>
>Take a look at the PPC version.

/*
 * The PowerPC can do unaligned accesses itself in big endian mode. 
 *
 * The strange macros are there to make sure these can't
 * be misused in a way that makes them not work on other
 * architectures where unaligned accesses aren't as simple.
 */

#define get_unaligned(ptr) (*(ptr))

#define put_unaligned(val, ptr) ((void)( *(ptr) = (val) ))

Looks ok to me ;-). But I do remember some problem on some arch.
Which was the reason why I implemented sdp_get_unaligned() instead
of using <asm/unaligned.h>

>The rule is that you're not permitted to use _anything_ in asm/ or
>linux/ directly from userspace. We know people do, but they shouldn't,
>and if it breaks they get to keep both pieces. 
>> >-       return ntohs(get_unaligned(u16_ptr));
>> >+       return (u8_ptr[0] << 8) | u8_ptr[1];
>> I'd rather see reimplementation of get_unaligned() in hcidump headers 
>> than this. We have sdp_get/put_unaligned() btw.
>
>I looked at that but AFAIK the C standard doesn't actually guarantee the
>packed structure method will work
I don't think packet structures are in the standard. But compiler has to 
guarantee that. Otherwise we'd have big problems (all hci structures are packed).

>and since we were always byteswapping
No we don't. ntohs does nothing on big endian machines.

Max



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Fix abuse of kernel headers.

[David Woodhouse]

On Fri, 2003-04-25 at 23:49, Max Krasnyansky wrote:
> >Take a look at the PPC version.

> Looks ok to me ;-). But I do remember some problem on some arch.
> Which was the reason why I implemented sdp_get_unaligned() instead
> of using <asm/unaligned.h>

It's wrapped in '#ifdef __KERNEL__' on the build box I used. 

> >I looked at that but AFAIK the C standard doesn't actually guarantee the
> > packed structure method will work 

> I don't think packet structures are in the standard. But compiler has to 
> guarantee that. Otherwise we'd have big problems (all hci structures
> are packed).

The people I consulted weren't convinced that the compiler must generate
code which handles the _structure_ being misaligned. Expecting it to
emitting code to correctly read an int which is three bytes from the
beginning of the struct is OK; expecting it to emit code which can
handle misalignment when the member being accessed is at a normal offset
from the start of the structure isn't necessarily safe.
 
> >and since we were always byteswapping
> No we don't. ntohs does nothing on big endian machines.

Sorry, I should have said 'since we were always byteswapping on i386
anyway'. 

Do it that way on ARM and you end up individually loading bytes and
shifting, then you swap them all round afterwards... :)

-- 
dwmw2




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Fix abuse of kernel headers.

[Max Krasnyansky]

At 04:28 PM 4/25/2003, David Woodhouse wrote:
>On Fri, 2003-04-25 at 23:49, Max Krasnyansky wrote:
>> >Take a look at the PPC version.
>
>> Looks ok to me ;-). But I do remember some problem on some arch.
>> Which was the reason why I implemented sdp_get_unaligned() instead
>> of using <asm/unaligned.h>
>
>It's wrapped in '#ifdef __KERNEL__' on the build box I used. 
Yep. That's it. I remember some problem with get_unaligned() but I couldn't remember 
what exactly it was. Thanks for the reminder :)

>> >I looked at that but AFAIK the C standard doesn't actually guarantee the
>> > packed structure method will work 
>
>> I don't think packet structures are in the standard. But compiler has to 
>> guarantee that. Otherwise we'd have big problems (all hci structures
>> are packed).
>
>The people I consulted weren't convinced that the compiler must generate
>code which handles the _structure_ being misaligned. Expecting it to
>emitting code to correctly read an int which is three bytes from the
>beginning of the struct is OK; expecting it to emit code which can
>handle misalignment when the member being accessed is at a normal offset
>from the start of the structure isn't necessarily safe.
Here is what info gcc has to say about that
"`packed'
     The `packed' attribute specifies that a variable or structure field
     should have the smallest possible alignment--one byte for a
     variable, and one bit for a field, unless you specify a larger
     value with the `aligned' attribute.

     Here is a structure in which the field `x' is packed, so that it
     immediately follows `a':

          struct foo
          {
            char a;
            int x[2] __attribute__ ((packed));
          };
"

See. It doesn't matter where the field is, compiler has to generate the 
code that can deal with any alignment.

Max



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Fix abuse of kernel headers.

[David Woodhouse]

On Mon, 2003-04-28 at 22:49, Max Krasnyansky wrote:
> See. It doesn't matter where the field is, compiler has to generate the 
> code that can deal with any alignment.

Consulting the GCC wizards locally confirms that you're correct. 

-- 
dwmw2



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel