All of lore.kernel.org
 help / color / mirror / Atom feed
* [COMPAT] Preventing namespace pollution
@ 2010-11-22  1:29 Arnaud Lacombe
  2010-11-22  1:30 ` [PATCH] compat/2.6.22: limit " Arnaud Lacombe
  2010-11-22 17:59 ` [COMPAT] Preventing " Luis R. Rodriguez
  0 siblings, 2 replies; 7+ messages in thread
From: Arnaud Lacombe @ 2010-11-22  1:29 UTC (permalink / raw)
  To: Luis R. Rodriguez; +Cc: Luis R. Rodriguez, linux-wireless, Arnaud Lacombe

Hi folks,

The second part of the changes I made as part of my work on the
compatibility headers concerned namespace pollution.

The problem was the following. My module is doing relatively high
level stuff in the kernel. It interracts with the network stack,
netfilter, the socket layer, etc. It does not deals with low-level
driver stuff. The issue was that including <linux/compat.h> bring a
_huge_ quantity of dependency with it, interrupt, thread, sysfs...
This is problematic as for some compilation unit, I should include as
few Linux header as possible (understand: if I don't include
<linux/tcp.h>, I don't want it included behind my back).

In order to workaround the problem, I simply wrapped every accessors
and struct definition within #ifdef checking if the parent header is
in use or not.

Considering the following exemple:

2.6.22:
 static inline unsigned char *
 skb_network_header(const struct sk_buff *skb)
 {
        return skb->nh.raw;
 }

 [...]

 static inline struct iphdr *
 ip_hdr(const struct sk_buff *skb)
 {
        return (struct iphdr *)skb_network_header(skb);
 }

 static inline unsigned int
 ip_hdrlen(const struct sk_buff *skb)
 {
        return ip_hdr(skb)->ihl * 4;
 }

Every compilation unit would need to have both <linux/skbuff.h>,
<linux/ip.h> and <net/ip.h> included. If the useful code does not
deals about networking, that's just pollution.

The solution I propose would transform this to:

2.6.22:
 /* <linux/skbuff.h> */
 #ifdef _LINUX_SKBUFF_H
 static inline unsigned char *
 skb_network_header(const struct sk_buff *skb)
 {
        return skb->nh.raw;
 }
 #endif

 [...]

 /* <linux/ip.h> */
 #ifdef _LINUX_IP_H
 static inline struct iphdr *
 ip_hdr(const struct sk_buff *skb)
 {
        return (struct iphdr *)skb_network_header(skb);
 }
 #endif

 /* <net/ip.h> */
 #ifdef _IP_H
 static inline unsigned int
 ip_hdrlen(const struct sk_buff *skb)
 {
        return ip_hdr(skb)->ihl * 4;
 }
 #endif

That way, if the original code did not include the headers, it will
just not get the compat definition. The macro checked is the one used
as re-inclusion guard from the parent header which should be stable,
but even if it changes, supporting the new is trivial. With this
solution, the compat headers no longer needs to import all the headers
it is providing compatibililty to, but just let the original source
tells it what it needs compatibility for.

I will send as answer to mail a proof of concept for 2.6.22

Comments welcome,
 - Arnaud

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] compat/2.6.22: limit namespace pollution
  2010-11-22  1:29 [COMPAT] Preventing namespace pollution Arnaud Lacombe
@ 2010-11-22  1:30 ` Arnaud Lacombe
  2010-11-22  1:32   ` Arnaud Lacombe
  2010-11-22 18:01   ` Luis R. Rodriguez
  2010-11-22 17:59 ` [COMPAT] Preventing " Luis R. Rodriguez
  1 sibling, 2 replies; 7+ messages in thread
From: Arnaud Lacombe @ 2010-11-22  1:30 UTC (permalink / raw)
  To: linux-wireless; +Cc: lrodriguez, Arnaud Lacombe

Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>
---
 include/linux/compat-2.6.22.h |   38 ++++++++++++++++++++++++++------------
 1 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/include/linux/compat-2.6.22.h b/include/linux/compat-2.6.22.h
index 7c0c615..4fefe49 100644
--- a/include/linux/compat-2.6.22.h
+++ b/include/linux/compat-2.6.22.h
@@ -3,19 +3,19 @@
 
 #include <linux/version.h>
 
 /* Compat work for 2.6.21 */
 #if (LINUX_VERSION_CODE < KERNEL_VERSION(2,6,22))
 
-#include <linux/pci.h>
-#include <linux/skbuff.h>
+/* <linux/netdevice.h> */
+#ifdef _LINUX_NETDEVICE_H
+#ifdef CONFIG_AX25
+#error "Compat reuses the AX.25 pointer so that may not be enabled!"
+#endif
 
 /* reuse ax25_ptr */
 #define ieee80211_ptr ax25_ptr
-
-#ifdef CONFIG_AX25
-#error Compat reuses the AX.25 pointer so that may not be enabled!
 #endif
 
+/* <linux/skbuff.h> */
+#ifdef _LINUX_SKBUFF_H
 static inline unsigned char *skb_mac_header(const struct sk_buff *skb)
 {
 	return skb->mac.raw;
@@ -61,11 +61,6 @@ static inline unsigned char *skb_tail_pointer(const struct sk_buff *skb)
 	return skb->tail;
 }
 
-static inline struct iphdr *ip_hdr(const struct sk_buff *skb)
-{
-	return (struct iphdr *)skb_network_header(skb);
-}
-
 static inline void skb_copy_from_linear_data(const struct sk_buff *skb,
 					     void *to,
 					     const unsigned int len)
@@ -79,17 +74,26 @@ static inline void skb_copy_from_linear_data_offset(const struct sk_buff *skb,
 {
 	memcpy(to, skb->data + offset, len);
 }
+#endif /* _LINUX_SKBUFF_H */
 
+/* <linux/ip.h> */
+#ifdef _LINUX_IP_H
+static inline struct iphdr *ip_hdr(const struct sk_buff *skb)
+{
+	return (struct iphdr *)skb_network_header(skb);
+}
+#endif
 
+/* <net/ip.h> */
+#ifdef _IP_H
 static inline unsigned int ip_hdrlen(const struct sk_buff *skb)
 {
 	return ip_hdr(skb)->ihl * 4;
 }
+#endif
 
+/* <linux/tcp.h> */
+#ifdef _LINUX_TCP_H
 static inline struct tcphdr *tcp_hdr(const struct sk_buff *skb)
 {
 	return (struct tcphdr *)skb_transport_header(skb);
@@ -104,7 +108,10 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb)
 {
 	return (tcp_hdr(skb)->doff - 5) * 4;
 }
+#endif
 
+/* <linux/compiler.h> */
+#ifdef __LINUX_COMPILER_H
 #if defined(__GNUC__)
 #define __maybe_unused		__attribute__((unused))
 #define uninitialized_var(x)	x = x
@@ -117,12 +124,19 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb)
 #ifndef uninitialized_var
 #define uninitialized_var(x)	x
 #endif
+#endif /* __LINUX_COMPILER_H */
 
+/* <net/netlink.h> */
+#ifdef __NET_NETLINK_H
 /* This will lead to very weird behaviour... */
 #define NLA_BINARY NLA_STRING
+#endif
 
+/* <linux/list.h> */
+#ifdef _LINUX_LIST_H
 #define list_first_entry(ptr, type, member) \
         list_entry((ptr)->next, type, member)
+#endif
 
 #endif /* (LINUX_VERSION_CODE < KERNEL_VERSION(2,6,22)) */
 
-- 
1.7.2.30.gc37d7.dirty


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] compat/2.6.22: limit namespace pollution
  2010-11-22  1:30 ` [PATCH] compat/2.6.22: limit " Arnaud Lacombe
@ 2010-11-22  1:32   ` Arnaud Lacombe
  2010-11-22 18:01   ` Luis R. Rodriguez
  1 sibling, 0 replies; 7+ messages in thread
From: Arnaud Lacombe @ 2010-11-22  1:32 UTC (permalink / raw)
  To: linux-wireless; +Cc: lrodriguez, Arnaud Lacombe

Hi,

On Sun, Nov 21, 2010 at 8:30 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>
> ---
>  include/linux/compat-2.6.22.h |   38 ++++++++++++++++++++++++++------------
>  1 files changed, 26 insertions(+), 12 deletions(-)
>
Please ignore the diffstat, I manually edited the patch to remove a
couple of nits.

A.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [COMPAT] Preventing namespace pollution
  2010-11-22  1:29 [COMPAT] Preventing namespace pollution Arnaud Lacombe
  2010-11-22  1:30 ` [PATCH] compat/2.6.22: limit " Arnaud Lacombe
@ 2010-11-22 17:59 ` Luis R. Rodriguez
  1 sibling, 0 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2010-11-22 17:59 UTC (permalink / raw)
  To: Arnaud Lacombe; +Cc: linux-wireless

On Sun, Nov 21, 2010 at 5:29 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> Hi folks,
>
> The second part of the changes I made as part of my work on the
> compatibility headers concerned namespace pollution.
>
> The problem was the following. My module is doing relatively high
> level stuff in the kernel. It interracts with the network stack,
> netfilter, the socket layer, etc. It does not deals with low-level
> driver stuff. The issue was that including <linux/compat.h> bring a
> _huge_ quantity of dependency with it, interrupt, thread, sysfs...
> This is problematic as for some compilation unit, I should include as
> few Linux header as possible (understand: if I don't include
> <linux/tcp.h>, I don't want it included behind my back).
>
> In order to workaround the problem, I simply wrapped every accessors
> and struct definition within #ifdef checking if the parent header is
> in use or not.
>
> Considering the following exemple:
>
> 2.6.22:
>  static inline unsigned char *
>  skb_network_header(const struct sk_buff *skb)
>  {
>        return skb->nh.raw;
>  }
>
>  [...]
>
>  static inline struct iphdr *
>  ip_hdr(const struct sk_buff *skb)
>  {
>        return (struct iphdr *)skb_network_header(skb);
>  }
>
>  static inline unsigned int
>  ip_hdrlen(const struct sk_buff *skb)
>  {
>        return ip_hdr(skb)->ihl * 4;
>  }
>
> Every compilation unit would need to have both <linux/skbuff.h>,
> <linux/ip.h> and <net/ip.h> included. If the useful code does not
> deals about networking, that's just pollution.
>
> The solution I propose would transform this to:
>
> 2.6.22:
>  /* <linux/skbuff.h> */
>  #ifdef _LINUX_SKBUFF_H
>  static inline unsigned char *
>  skb_network_header(const struct sk_buff *skb)
>  {
>        return skb->nh.raw;
>  }
>  #endif
>
>  [...]
>
>  /* <linux/ip.h> */
>  #ifdef _LINUX_IP_H
>  static inline struct iphdr *
>  ip_hdr(const struct sk_buff *skb)
>  {
>        return (struct iphdr *)skb_network_header(skb);
>  }
>  #endif
>
>  /* <net/ip.h> */
>  #ifdef _IP_H
>  static inline unsigned int
>  ip_hdrlen(const struct sk_buff *skb)
>  {
>        return ip_hdr(skb)->ihl * 4;
>  }
>  #endif
>
> That way, if the original code did not include the headers, it will
> just not get the compat definition. The macro checked is the one used
> as re-inclusion guard from the parent header which should be stable,
> but even if it changes, supporting the new is trivial. With this
> solution, the compat headers no longer needs to import all the headers
> it is providing compatibililty to, but just let the original source
> tells it what it needs compatibility for.
>
> I will send as answer to mail a proof of concept for 2.6.22

I suspect we'll find a case where headers are required to be included
for some older kernel where it was not required for newer ones but we
can find out. Just please be sure to test compile your patches against
each supported kernel and I think we'll be good!

  Luis

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] compat/2.6.22: limit namespace pollution
  2010-11-22  1:30 ` [PATCH] compat/2.6.22: limit " Arnaud Lacombe
  2010-11-22  1:32   ` Arnaud Lacombe
@ 2010-11-22 18:01   ` Luis R. Rodriguez
  2010-11-22 18:14     ` Arnaud Lacombe
  1 sibling, 1 reply; 7+ messages in thread
From: Luis R. Rodriguez @ 2010-11-22 18:01 UTC (permalink / raw)
  To: Arnaud Lacombe; +Cc: linux-wireless

On Sun, Nov 21, 2010 at 5:30 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>

Was this test compiled on all supported kernels?

  Luis

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] compat/2.6.22: limit namespace pollution
  2010-11-22 18:01   ` Luis R. Rodriguez
@ 2010-11-22 18:14     ` Arnaud Lacombe
  2010-11-22 18:29       ` Luis R. Rodriguez
  0 siblings, 1 reply; 7+ messages in thread
From: Arnaud Lacombe @ 2010-11-22 18:14 UTC (permalink / raw)
  To: Luis R. Rodriguez; +Cc: linux-wireless

Hi,

On Mon, Nov 22, 2010 at 1:01 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> On Sun, Nov 21, 2010 at 5:30 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>> Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>
>
> Was this test compiled on all supported kernels?
>
I am using this internally for all kernels (.15 -> .36). Beside that,
I should still test-build the full compat module. Fixes might be
needed.

What is the "official" range of supported kernels ? AFAIR, at some
point, very old kernel are not included by <linux/compat.h>. Do you
know a module I can use to test regressions, other than my use-case ?

 - Arnaud

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] compat/2.6.22: limit namespace pollution
  2010-11-22 18:14     ` Arnaud Lacombe
@ 2010-11-22 18:29       ` Luis R. Rodriguez
  0 siblings, 0 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2010-11-22 18:29 UTC (permalink / raw)
  To: Arnaud Lacombe; +Cc: linux-wireless

On Mon, Nov 22, 2010 at 10:14 AM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> Hi,
>
> On Mon, Nov 22, 2010 at 1:01 PM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
>> On Sun, Nov 21, 2010 at 5:30 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>>> Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>
>>
>> Was this test compiled on all supported kernels?
>>
> I am using this internally for all kernels (.15 -> .36). Beside that,
> I should still test-build the full compat module. Fixes might be
> needed.

Yeah.. then the patch cannot be applied.

> What is the "official" range of supported kernels ?

You'd know better, we use compat down to 2.6.22 in compat-wireless but
last I tried I believe I was able to compile compat even on older
kernels.

> AFAIR, at some
> point, very old kernel are not included by <linux/compat.h>. Do you
> know a module I can use to test regressions, other than my use-case ?

Sure, you can try building compat-wireless as an example but
compat-wireless only goes down to 2.6.22 (IIRC) and compat goes down
to older kernels. Either way, if you take compat down to older than
2.6.22 we'll be good. So you can test anything you care about older
than 2.6.22 and anything above for sure.

  Luis

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2010-11-22 18:30 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-22  1:29 [COMPAT] Preventing namespace pollution Arnaud Lacombe
2010-11-22  1:30 ` [PATCH] compat/2.6.22: limit " Arnaud Lacombe
2010-11-22  1:32   ` Arnaud Lacombe
2010-11-22 18:01   ` Luis R. Rodriguez
2010-11-22 18:14     ` Arnaud Lacombe
2010-11-22 18:29       ` Luis R. Rodriguez
2010-11-22 17:59 ` [COMPAT] Preventing " Luis R. Rodriguez

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.