net: define __packed for the userspace code

PATCH In review Changli Gao 3 participants 8 messages Aug 22, 2010 – Aug 23, 2010

Subsystems (3)

Networking Network drivers Sparse checker

lore

Share & tools

lore

Changli Gao

author

2010-08-22 11:12 UTC #0 lore raw

PATCH 1/1 net: define __packed for the userspace code

This commit

commit bc10502dba37d3b210efd9f3867212298f13b78e
Author: Eric Dumazet <[email protected]>
Date: Thu Jun 3 03:21:52 2010 -0700

net: use __packed annotation

makes use of __packed in the userspace code. So we'd better define __packed for the userspace code too.

Signed-off-by Changli Gao <[email protected]>

---
 include/linux/compiler.h |    4 ++++
 include/linux/if_pppox.h |    4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

include/linux/compiler.h +4 -0

index c1a62c5..515b174 100644
@@ -304,4 +304,8 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+#ifndef __packed
+# define __packed __attribute__((packed))
+#endif
+
 #endif /* __LINUX_COMPILER_H */

Changli Gao

author

2010-08-22 11:23 UTC #1 lore raw

Quoted 11 lines · Changli Gao

This commit
commit bc10502dba37d3b210efd9f3867212298f13b78e
Author: Eric Dumazet <[email protected]>
Date: Thu Jun 3 03:21:52 2010 -0700
net: use __packed annotation
makes use of __packed in the userspace code. So we'd better define __packed for the userspace code too.

Oh, sorry. This patch can't work as include/linux/compiler.h isn't exported to the userspace. But where should we define __packed for the userspace code? include/linux/types.h?

Signature · 2 lines

Regards,
Changli Gao([email protected])

Arnd Bergmann

2010-08-22 13:08 UTC #2 lore raw

Quoted 9 lines · On Sunday 22 August 2010 13:23:42 Changli Gao wrote:

Quoted 8 lines · Changli Gao
This commit
commit bc10502dba37d3b210efd9f3867212298f13b78e
Author: Eric Dumazet <[email protected]>
Date: Thu Jun 3 03:21:52 2010 -0700
net: use __packed annotation

Quoted 7 lines

Quoted 3 lines
makes use of __packed in the userspace code. So we'd better define __packed for the userspace code too.
Oh, sorry. This patch can't work as include/linux/compiler.h isn't exported to the userspace. But where should we define __packed for the userspace code? include/linux/types.h?

I would try to avoid making those structures packed to start with.

Quoted 1 line

From what I can see, they structures annotated in the above commit

mostly don't even require explicit packing because they are already packed. Not marking them packed makes the code portable to non-gcc compilers.

Arnd

Changli Gao

author

2010-08-22 13:47 UTC #3 lore raw

Quoted 18 lines · Arnd Bergmann

Quoted 9 lines · On Sunday 22 August 2010 13:23:42 Changli Gao wrote:
Quoted 8 lines · Changli Gao
This commit
commit bc10502dba37d3b210efd9f3867212298f13b78e
Author: Eric Dumazet <[email protected]>
Date: Thu Jun 3 03:21:52 2010 -0700
net: use __packed annotation
Quoted 7 lines
Quoted 3 lines
makes use of __packed in the userspace code. So we'd better define __packed for the userspace code too.
Oh, sorry. This patch can't work as include/linux/compiler.h isn't exported to the userspace. But where should we define __packed for the userspace code? include/linux/types.h?

Quoted 6 lines

I would try to avoid making those structures packed to start with. From what I can see, they structures annotated in the above commit mostly don't even require explicit packing because they are already packed. Not marking them packed makes the code portable to non-gcc compilers.

Maybe __packed is used somewhere to hint that some members of a structure maybe unaligned.

Signature · 2 lines

Regards,
Changli Gao([email protected])

David Miller

maintainer

2010-08-23 01:36 UTC #4 lore raw

From: Changli Gao <[email protected]>
Date: Sun, 22 Aug 2010 21:47:06 +0800

Quoted 26 lines · Arnd Bergmann

Quoted 18 lines · On Sunday 22 August 2010 13:23:42 Changli Gao wrote:
Quoted 9 lines · Changli Gao
Quoted 8 lines
This commit
commit bc10502dba37d3b210efd9f3867212298f13b78e
Author: Eric Dumazet <[email protected]>
Date: Thu Jun 3 03:21:52 2010 -0700
net: use __packed annotation
Quoted 7 lines
Quoted 3 lines
makes use of __packed in the userspace code. So we'd better define __packed for the userspace code too.
Oh, sorry. This patch can't work as include/linux/compiler.h isn't exported to the userspace. But where should we define __packed for the userspace code? include/linux/types.h?
Quoted 6 lines
I would try to avoid making those structures packed to start with. From what I can see, they structures annotated in the above commit mostly don't even require explicit packing because they are already packed. Not marking them packed makes the code portable to non-gcc compilers.

Quoted 2 lines

Maybe __packed is used somewhere to hint that some members of a structure maybe unaligned.

I don't think this it the reason it was being used here.

Any, for one thing, we definitely cannot remove the existing packed markers or else we will break every single userland tool out there using these socket address structures.

Even the first two members (sa_family_t and unsigned int) will be positioned differently if we remove the marker.

I suspect the packed attribute is there to make sure the pppo* socket address structures fit within the generic socket address object size. (see struct __kernel_sockaddr_storage and struct sockaddr).

As to the problem at-hand, I think we need to use __attribute__((packed)) here. And that's what I'll commit into net-2.6 and net-next-2.6

Changli Gao

author

2010-08-23 02:29 UTC #5 lore raw

Quoted 10 lines · David Miller

I don't think this it the reason it was being used here.
Any, for one thing, we definitely cannot remove the existing packed markers or else we will break every single userland tool out there using these socket address structures.
Even the first two members (sa_family_t and unsigned int) will be positioned differently if we remove the marker.

Quoted 7 lines

I suspect the packed attribute is there to make sure the pppo* socket address structures fit within the generic socket address object size. (see struct __kernel_sockaddr_storage and struct sockaddr).
As to the problem at-hand, I think we need to use __attribute__((packed)) here. And that's what I'll commit into net-2.6 and net-next-2.6

Do you mean that use the __attribute__((packed)) annotation in all of these files:

localhost linux # grep "\<__packed\>" usr/include/ -r | uniq usr/include/linux/if_hippi.h:} __packed; usr/include/linux/if_fddi.h:} __packed; usr/include/linux/nbd.h:} __packed; usr/include/linux/ncp.h:} __packed; usr/include/linux/rfkill.h:} __packed; usr/include/linux/if_pppox.h:} __packed; usr/include/linux/phonet.h:} __packed;
usr/include/linux/ipv6.h:} __packed; /* required for some archs */
usr/include/linux/ipv6.h:} __packed; usr/include/linux/if_ether.h:} __packed;

Signature · 2 lines

Regards,
Changli Gao([email protected])

David Miller

maintainer

2010-08-23 02:35 UTC #6 lore raw

From: Changli Gao <[email protected]>
Date: Mon, 23 Aug 2010 10:29:49 +0800

Quoted 14 lines

Do you mean that use the __attribute__((packed)) annotation in all of these files:
localhost linux # grep "\<__packed\>" usr/include/ -r | uniq usr/include/linux/if_hippi.h:} __packed; usr/include/linux/if_fddi.h:} __packed; usr/include/linux/nbd.h:} __packed; usr/include/linux/ncp.h:} __packed; usr/include/linux/rfkill.h:} __packed; usr/include/linux/if_pppox.h:} __packed; usr/include/linux/phonet.h:} __packed;
usr/include/linux/ipv6.h:} __packed; /* required for some archs */
usr/include/linux/ipv6.h:} __packed; usr/include/linux/if_ether.h:} __packed;

It seems so, yes.

There is no way that anybody has tried to compile anything in userspace using these headers with the __packed usage there.

If they would, they would surely see a compile failure.

David Miller

maintainer

2010-08-23 04:15 UTC #7 lore raw

From: David Miller <[email protected]>
Date: Sun, 22 Aug 2010 19:36:09 -0700 (PDT)

Quoted 18 lines

From: Changli Gao <[email protected]>
Date: Mon, 23 Aug 2010 10:29:49 +0800
Quoted 14 lines
Do you mean that use the __attribute__((packed)) annotation in all of these files:
localhost linux # grep "\<__packed\>" usr/include/ -r | uniq usr/include/linux/if_hippi.h:} __packed; usr/include/linux/if_fddi.h:} __packed; usr/include/linux/nbd.h:} __packed; usr/include/linux/ncp.h:} __packed; usr/include/linux/rfkill.h:} __packed; usr/include/linux/if_pppox.h:} __packed; usr/include/linux/phonet.h:} __packed;
usr/include/linux/ipv6.h:} __packed; /* required for some archs */
usr/include/linux/ipv6.h:} __packed; usr/include/linux/if_ether.h:} __packed;

Quoted 1 line

It seems so, yes.

Changli I applied your patch to convert these __packed tags back to the original expansion.

Thanks!