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
On Mon, Nov 22, 2010 at 10:14 AM, Arnaud Lacombe <[email protected]> wrote:
> Hi,
>
> On Mon, Nov 22, 2010 at 1:01 PM, Luis R. Rodriguez
> <[email protected]> wrote:
>> On Sun, Nov 21, 2010 at 5:30 PM, Arnaud Lacombe <[email protected]> wrote:
>>> Signed-off-by: Arnaud Lacombe <[email protected]>
>>
>> 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
Hi,
On Sun, Nov 21, 2010 at 8:30 PM, Arnaud Lacombe <[email protected]> wrote:
> Signed-off-by: Arnaud Lacombe <[email protected]>
> ---
> ?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.
On Sun, Nov 21, 2010 at 5:30 PM, Arnaud Lacombe <[email protected]> wrote:
> Signed-off-by: Arnaud Lacombe <[email protected]>
Was this test compiled on all supported kernels?
Luis
Signed-off-by: Arnaud Lacombe <[email protected]>
---
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
On Sun, Nov 21, 2010 at 5:29 PM, Arnaud Lacombe <[email protected]> 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
Hi,
On Mon, Nov 22, 2010 at 1:01 PM, Luis R. Rodriguez
<[email protected]> wrote:
> On Sun, Nov 21, 2010 at 5:30 PM, Arnaud Lacombe <[email protected]> wrote:
>> Signed-off-by: Arnaud Lacombe <[email protected]>
>
> 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