[PATCH 0/2] A couple of netfilter fixes

[PATCH 0/2] A couple of netfilter fixes

[Pablo Neira Ayuso]

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!

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

[Pablo Neira Ayuso]

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 <pablo@netfilter.org>
Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/netfilter/nf_conntrack_proto_tcp.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 802dbff..c4c885d 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -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;

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

[Patrick McHardy]

Am 12.07.2010 18:59, schrieb Pablo Neira Ayuso:
> 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.

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.

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

[Pablo Neira Ayuso]

On 15/07/10 11:16, Patrick McHardy wrote:
> Am 12.07.2010 18:59, schrieb Pablo Neira Ayuso:
>> 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.
> 
> 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.

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

[Patrick McHardy]

Am 15.07.2010 13:55, schrieb Pablo Neira Ayuso:
> On 15/07/10 11:16, Patrick McHardy wrote:
>> Am 12.07.2010 18:59, schrieb Pablo Neira Ayuso:
>>> 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.
>>
>> 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.

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

[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 <pablo@netfilter.org>
---
 include/linux/netfilter/nfnetlink_queue.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/netfilter/nfnetlink_queue.h b/include/linux/netfilter/nfnetlink_queue.h
index 2455fe5..5512a35 100644
--- a/include/linux/netfilter/nfnetlink_queue.h
+++ b/include/linux/netfilter/nfnetlink_queue.h
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Patrick McHardy]

Am 12.07.2010 19:00, schrieb 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’

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

> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On 15/07/10 11:19, Patrick McHardy wrote:
> Am 12.07.2010 19:00, schrieb 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’
> 
> 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).

>> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Patrick McHardy]

Am 15.07.2010 13:59, schrieb Pablo Neira Ayuso:
> On 15/07/10 11:19, Patrick McHardy wrote:
>> Am 12.07.2010 19:00, schrieb 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’
>>
>> 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).
>

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html