A couple of netfilter fixes

PATCH Applied Pablo Neira Ayuso 2 participants 9 messages Jul 12, 2010 – Jul 15, 2010

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

Pablo Neira Ayuso

author maintainer

2010-07-12 16:59 UTC #0 lore raw

cover A couple of netfilter fixes

Hi Patrick,

You can find two patches here. The former fixes flow recovery with TCP window tracking enabled, it's been tested with the current snapshot of conntrackd and the libnetfilter_* libraries. The latter defines aligned_be64 to allow to compile user-space Netlink code without adding an ad-hoc definition of that type.

Let me know if you consider that they can be qualified as fixes for the -stable branch.

Thanks!

---

Pablo Neira Ayuso (2):
netfilter: nf_ct_tcp: fix flow recovery with TCP window tracking enabled
netfilter: nfnetlink_queue: add definition of aligned_be64 for user-space apps

 include/linux/netfilter/nfnetlink_queue.h |    6 ++++++
 net/netfilter/nf_conntrack_proto_tcp.c    |   10 +++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

--
Under the asphalt, there's the orchard!

Pablo Neira Ayuso

author maintainer

2010-07-12 17:00 UTC #1 lore raw

PATCH 1/2 netfilter: nf_ct_tcp: fix flow recovery with TCP window tracking enabled

This patch adds the missing bits to support the recovery of TCP flows without disabling window tracking (aka be_liberal). To ensure a successful recovery, we have to inject the window scale factor via ctnetlink.

This patch has been tested with a development snapshot of conntrackd and the new clause `TCPWindowTracking' that allows to perform strict TCP window tracking recovery across fail-overs.

With this patch, we don't update the receiver's window until it's not initiated. We require this to perform a successful recovery. Jozsef confirmed in a private email that this spotted a real issue since that should not happen.

Signed-off-by Pablo Neira Ayuso <[email protected]>

Acked-by Jozsef Kadlecsik <[email protected]>

---
 net/netfilter/nf_conntrack_proto_tcp.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

net/netfilter/nf_conntrack_proto_tcp.c +9 -1

index 802dbff..c4c885d 100644
@@ -585,8 +585,16 @@ static bool tcp_in_window(const struct nf_conn *ct,
 			 * Let's try to use the data from the packet.
 			 */
 			sender->td_end = end;
+			win <<= sender->td_scale;
 			sender->td_maxwin = (win == 0 ? 1 : win);
 			sender->td_maxend = end + sender->td_maxwin;
+			/*
+			 * We haven't seen traffic in the other direction yet
+			 * but we have to tweak window tracking to pass III
+			 * and IV until that happens.
+			 */
+			if (receiver->td_maxwin == 0)
+				receiver->td_end = receiver->td_maxend = sack;
 		}
 	} else if (((state->state == TCP_CONNTRACK_SYN_SENT
 		     && dir == IP_CT_DIR_ORIGINAL)

@@ -680,7 +688,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
 		/*
 		 * Update receiver data.
 		 */
-		if (after(end, sender->td_maxend))
+		if (receiver->td_maxwin != 0 && after(end, sender->td_maxend))
 			receiver->td_maxwin += end - sender->td_maxend;
 		if (after(sack + win, receiver->td_maxend - 1)) {
 			receiver->td_maxend = sack + win;

Patrick McHardy

2010-07-15 09:16 UTC #2 lore raw

Re: [PATCH 1/2] netfilter: nf_ct_tcp: fix flow recovery with TCP window tracking enabled

Am 12.07.2010 18:59, schrieb Pablo Neira Ayuso:

Quoted 5 lines

This patch adds the missing bits to support the recovery of TCP flows without disabling window tracking (aka be_liberal). To ensure a successful recovery, we have to inject the window scale factor via ctnetlink.

Quoted 8 lines

This patch has been tested with a development snapshot of conntrackd and the new clause `TCPWindowTracking' that allows to perform strict TCP window tracking recovery across fail-overs.
With this patch, we don't update the receiver's window until it's not initiated. We require this to perform a successful recovery. Jozsef confirmed in a private email that this spotted a real issue since that should not happen.

Regarding the question whether to put this into nf-2.6.git or nf-next.git, does this fix any bugs besides adding support for using window tracking with synchronization? I'm not quite sure what "a real issue" is supposed to mean in this case.

Pablo Neira Ayuso

author maintainer

2010-07-15 11:55 UTC #3 lore raw

Re: [PATCH 1/2] netfilter: nf_ct_tcp: fix flow recovery with TCP window tracking enabled

Quoted 15 lines · Patrick McHardy

Am 12.07.2010 18:59, schrieb Pablo Neira Ayuso:
Quoted 5 lines
This patch adds the missing bits to support the recovery of TCP flows without disabling window tracking (aka be_liberal). To ensure a successful recovery, we have to inject the window scale factor via ctnetlink.
Quoted 8 lines
This patch has been tested with a development snapshot of conntrackd and the new clause `TCPWindowTracking' that allows to perform strict TCP window tracking recovery across fail-overs.
With this patch, we don't update the receiver's window until it's not initiated. We require this to perform a successful recovery. Jozsef confirmed in a private email that this spotted a real issue since that should not happen.

Quoted 4 lines

Regarding the question whether to put this into nf-2.6.git or nf-next.git, does this fix any bugs besides adding support for using window tracking with synchronization? I'm not quite sure what "a real issue" is supposed to mean in this case.

Hm, no. This is only useful for window tracking with synchronization (or any similar application using ctnetlink). Better push it into nf-next-2.6.git.

Patrick McHardy

Applied

2010-07-15 15:10 UTC #4 lore raw

Re: [PATCH 1/2] netfilter: nf_ct_tcp: fix flow recovery with TCP window tracking enabled

Am 15.07.2010 13:55, schrieb Pablo Neira Ayuso:

Quoted 16 lines · Patrick McHardy

Quoted 15 lines
Am 12.07.2010 18:59, schrieb Pablo Neira Ayuso:
Quoted 13 lines
This patch adds the missing bits to support the recovery of TCP flows without disabling window tracking (aka be_liberal). To ensure a successful recovery, we have to inject the window scale factor via ctnetlink.
This patch has been tested with a development snapshot of conntrackd and the new clause `TCPWindowTracking' that allows to perform strict TCP window tracking recovery across fail-overs.
With this patch, we don't update the receiver's window until it's not initiated. We require this to perform a successful recovery. Jozsef confirmed in a private email that this spotted a real issue since that should not happen.

Quoted 8 lines

Quoted 4 lines
Regarding the question whether to put this into nf-2.6.git or nf-next.git, does this fix any bugs besides adding support for using window tracking with synchronization? I'm not quite sure what "a real issue" is supposed to mean in this case.
Hm, no. This is only useful for window tracking with synchronization (or any similar application using ctnetlink). Better push it into nf-next-2.6.git.

Applied to nf-next, thanks.

Pablo Neira Ayuso

author maintainer

2010-07-12 17:00 UTC #5 lore raw

PATCH 2/2 netfilter: nfnetlink_queue: add definition of aligned_be64 for user-space apps

In reply to Pablo Neira Ayuso

Currently, libnl and libnetfilter_queue include in one of their user-space header files an ad-hoc definition of aligned_be64. However, applications that use the BSD socket API to communicate via Netlink sockets (ie. those that do not use these libraries) would need to define this type by hand if they include the kernel-space header nfnetlink_queue.h.

This patch adds the definition of aligned_bed64 for user-space applications in the kernel header. Otherwise, they have to define it to avoid the following compilation problem:

/usr/include/linux/netfilter/nfnetlink_queue.h:28: error: expected specifier-qualifier-list before ‘aligned_be64’

I know, this is ugly but I think that user-space Netlink applications should compile with the only need of including the kernel-space header that contains the protocol definitions.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 include/linux/netfilter/nfnetlink_queue.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

include/linux/netfilter/nfnetlink_queue.h +6 -0

index 2455fe5..5512a35 100644
@@ -24,6 +24,12 @@ struct nfqnl_msg_packet_hw {
 	__u8	hw_addr[8];
 };
 
+#ifndef __KERNEL__
+#ifndef aligned_be64
+#define aligned_be64 u_int64_t __attribute__((aligned(8)))
+#endif
+#endif /* !__KERNEL__ */
+
 struct nfqnl_msg_packet_timestamp {
 	aligned_be64	sec;
 	aligned_be64	usec;

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

2010-07-15 09:19 UTC #6 lore raw

Re: [PATCH 2/2] netfilter: nfnetlink_queue: add definition of aligned_be64 for user-space apps

Am 12.07.2010 19:00, schrieb Pablo Neira Ayuso:

Quoted 7 lines

Currently, libnl and libnetfilter_queue include in one of their user-space header files an ad-hoc definition of aligned_be64. However, applications that use the BSD socket API to communicate via Netlink sockets (ie. those that do not use these libraries) would need to define this type by hand if they include the kernel-space header nfnetlink_queue.h.

Quoted 5 lines

This patch adds the definition of aligned_bed64 for user-space applications in the kernel header. Otherwise, they have to define it to avoid the following compilation problem:
/usr/include/linux/netfilter/nfnetlink_queue.h:28: error: expected specifier-qualifier-list before ‘aligned_be64’

Why can't these applications simply include linux/types.h?

Quoted 3 lines

I know, this is ugly but I think that user-space Netlink applications should compile with the only need of including the kernel-space header that contains the protocol definitions.

I disagree, it makes no sense to re-define this for every header file. They can either include linux/types.h, or we could automatically include it in the files requiring it.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

author maintainer

2010-07-15 11:59 UTC #7 lore raw

Re: [PATCH 2/2] netfilter: nfnetlink_queue: add definition of aligned_be64 for user-space apps

Quoted 8 lines · Patrick McHardy

Am 12.07.2010 19:00, schrieb Pablo Neira Ayuso:
Quoted 7 lines
Currently, libnl and libnetfilter_queue include in one of their user-space header files an ad-hoc definition of aligned_be64. However, applications that use the BSD socket API to communicate via Netlink sockets (ie. those that do not use these libraries) would need to define this type by hand if they include the kernel-space header nfnetlink_queue.h.

Quoted 7 lines

Quoted 5 lines
This patch adds the definition of aligned_bed64 for user-space applications in the kernel header. Otherwise, they have to define it to avoid the following compilation problem:
/usr/include/linux/netfilter/nfnetlink_queue.h:28: error: expected specifier-qualifier-list before ‘aligned_be64’
Why can't these applications simply include linux/types.h?

Including it doesn't fix the problem here:

#include <linux/types.h>
#include <linux/netfilter/nfnetlink_queue.h>

I still get:

/usr/include/linux/netfilter/nfnetlink_queue.h:28: error: expected specifier-qualifier-list before ‘aligned_be64’

aligned_be64 is only define in the kernel (it's included under the __KERNEL__ definition).

Quoted 7 lines

Quoted 3 lines
I know, this is ugly but I think that user-space Netlink applications should compile with the only need of including the kernel-space header that contains the protocol definitions.
I disagree, it makes no sense to re-define this for every header file. They can either include linux/types.h, or we could automatically include it in the files requiring it.

This is how it looks now in user-space:

#include <...>
#ifndef aligned_be64
#define aligned_be64 u_int64_t __attribute__((aligned(8)))
#endif
#include <linux/netfilter/nfnetlink_queue.h>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

2010-07-15 15:07 UTC #8 lore raw

Re: [PATCH 2/2] netfilter: nfnetlink_queue: add definition of aligned_be64 for user-space apps

Am 15.07.2010 13:59, schrieb Pablo Neira Ayuso:

Quoted 22 lines · Patrick McHardy

Quoted 8 lines
Am 12.07.2010 19:00, schrieb Pablo Neira Ayuso:
Quoted 7 lines
Currently, libnl and libnetfilter_queue include in one of their user-space header files an ad-hoc definition of aligned_be64. However, applications that use the BSD socket API to communicate via Netlink sockets (ie. those that do not use these libraries) would need to define this type by hand if they include the kernel-space header nfnetlink_queue.h.
Quoted 7 lines
Quoted 5 lines
This patch adds the definition of aligned_bed64 for user-space applications in the kernel header. Otherwise, they have to define it to avoid the following compilation problem:
/usr/include/linux/netfilter/nfnetlink_queue.h:28: error: expected specifier-qualifier-list before ‘aligned_be64’
Why can't these applications simply include linux/types.h?
Including it doesn't fix the problem here:
#include <linux/types.h>
#include <linux/netfilter/nfnetlink_queue.h>

Quoted 8 lines

I still get:
/usr/include/linux/netfilter/nfnetlink_queue.h:28: error: expected specifier-qualifier-list before ‘aligned_be64’
aligned_be64 is only define in the kernel (it's included under the __KERNEL__ definition).

In that case I think we should export a __aligned_be64 in types.h and use that instead of aligned_be64.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html