TCP proto info

TCP proto info

[rui.sousa]

Hi,

I have an application using libnetfilter_conntrack-0.100 that started 
reporting errors after the commit: 

1c450e1595afdc8d1bfabb4f640c9251808426eb.

I've checked that the error happens because the kernel sees an empty 
CTA_PROTOINFO attribute when parsing a conntrack update.
In the case of my application I was just trying to update the timeout of a 
TCP conntrack and so I didn't specify any of the TCP protoinfo attributes. 
With the previous library version no CTA_PROTOINFO attribute was set in 
the conntrack and no error was seen (this is valid from the point of view 
of the kernel). Now, an error is reported and the netlink command ignored.
What do you think would be the right fix? Should the commit be reverted? 
Or is the application obliged to specify attributes which it doesn't want 
to modify (and introduce possible race conditions since it's usually a 
read/write/modify)?

Br,
Rui

Re: TCP proto info

[Pablo Neira Ayuso]

Hi Rui,

On 01/09/10 15:45, rui.sousa@mindspeed.com wrote:
> Hi,
> 
> I have an application using libnetfilter_conntrack-0.100 that started 
> reporting errors after the commit: 
> 
> 1c450e1595afdc8d1bfabb4f640c9251808426eb.

Looking at the source code, this seems to be already fixed in
libnetfilter_conntrack 0.0.102, please upgrade to latest.

Re: TCP proto info

[rui.sousa]

Pablo Neira Ayuso <pablo@netfilter.org> wrote on 09/02/2010 10:57:39 AM:

> Hi Rui,

Hi Pablo,

> On 01/09/10 15:45, rui.sousa@mindspeed.com wrote:
> > Hi,
> > 
> > I have an application using libnetfilter_conntrack-0.100 that started 
> > reporting errors after the commit: 
> > 
> > 1c450e1595afdc8d1bfabb4f640c9251808426eb.
> 
> Looking at the source code, this seems to be already fixed in
> libnetfilter_conntrack 0.0.102, please upgrade to latest.

Hmm... looking at the git tree I see that the __build_conntrack() code is
still calling __build_protoinfo() unconditionally and inside the function
we always do:

nest = nfnl_nest(&req->nlh, size, CTA_PROTOINFO);
nest_proto = nfnl_nest(&req->nlh, size, CTA_PROTOINFO_TCP);
...
nfnl_nest_end(&req->nlh, nest_proto);
nfnl_nest_end(&req->nlh, nest);

even if none of the ATTR_TCP_xxx bits are set. This is what causes the 
kernel to return -EINVAL
and ignore the conntrack update. Or am I missing something?

Br,
Rui

Re: TCP proto info

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

On 02/09/10 11:21, rui.sousa@mindspeed.com wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote on 09/02/2010 10:57:39 AM:
> 
>> Hi Rui,
> 
> Hi Pablo,
> 
>> On 01/09/10 15:45, rui.sousa@mindspeed.com wrote:
>>> Hi,
>>>
>>> I have an application using libnetfilter_conntrack-0.100 that started 
>>> reporting errors after the commit: 
>>>
>>> 1c450e1595afdc8d1bfabb4f640c9251808426eb.
>>
>> Looking at the source code, this seems to be already fixed in
>> libnetfilter_conntrack 0.0.102, please upgrade to latest.
> 
> Hmm... looking at the git tree I see that the __build_conntrack() code is
> still calling __build_protoinfo() unconditionally and inside the function
> we always do:
> 
> nest = nfnl_nest(&req->nlh, size, CTA_PROTOINFO);
> nest_proto = nfnl_nest(&req->nlh, size, CTA_PROTOINFO_TCP);
> ...
> nfnl_nest_end(&req->nlh, nest_proto);
> nfnl_nest_end(&req->nlh, nest);
> 
> even if none of the ATTR_TCP_xxx bits are set. This is what causes the 
> kernel to return -EINVAL
> and ignore the conntrack update. Or am I missing something?

I see, I guess that you're using a Linux kernel <= 2.6.25 since I
couldn't reproduce it with recent kernels. Please, could you give a try
to the following patch?

[-- Attachment #2: fix.patch --]
[-- Type: text/x-patch, Size: 2516 bytes --]

ct: fix EINVAL if not TCP attributes are set for Linux kernel <= 2.6.25

This patch fixes an EINVAL error that we hit in Linux kernel <= 2.6.25.
Basically, if we send an empty CTA_PROTOINFO_TCP attribute nest, the
kernel returns EINVAL. To fix this, we previously check if there is
any TCP attribute set.

Reported-by: Rui Sousa <rui.sousa@mindspeed.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/conntrack/build.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/src/conntrack/build.c b/src/conntrack/build.c
index b878ddd..ec7623d 100644
--- a/src/conntrack/build.c
+++ b/src/conntrack/build.c
@@ -106,6 +106,18 @@ static void __build_protoinfo(struct nfnlhdr *req, size_t size,
 
 	switch(ct->tuple[__DIR_ORIG].protonum) {
 	case IPPROTO_TCP:
+		/* Preliminary attribute check to avoid sending an empty
+		 * CTA_PROTOINFO_TCP nest, which results in EINVAL in
+		 * Linux kernel <= 2.6.25. */
+		if (!(test_bit(ATTR_TCP_STATE, ct->set) ||
+		      test_bit(ATTR_TCP_FLAGS_ORIG, ct->set) ||
+		      test_bit(ATTR_TCP_FLAGS_REPL, ct->set) ||
+		      test_bit(ATTR_TCP_MASK_ORIG, ct->set) ||
+		      test_bit(ATTR_TCP_MASK_REPL, ct->set) ||
+		      test_bit(ATTR_TCP_WSCALE_ORIG, ct->set) ||
+		      test_bit(ATTR_TCP_WSCALE_REPL, ct->set))) {
+			break;
+		}
 		nest = nfnl_nest(&req->nlh, size, CTA_PROTOINFO);
 		nest_proto = nfnl_nest(&req->nlh, size, CTA_PROTOINFO_TCP);
 		if (test_bit(ATTR_TCP_STATE, ct->set))
@@ -139,6 +151,12 @@ static void __build_protoinfo(struct nfnlhdr *req, size_t size,
 		nfnl_nest_end(&req->nlh, nest);
 		break;
 	case IPPROTO_SCTP:
+		/* See comment above on TCP. */
+		if (!(test_bit(ATTR_SCTP_STATE, ct->set) ||
+		      test_bit(ATTR_SCTP_VTAG_ORIG, ct->set) ||
+		      test_bit(ATTR_SCTP_VTAG_REPL, ct->set))) {
+			break;
+		}
 		nest = nfnl_nest(&req->nlh, size, CTA_PROTOINFO);
 		nest_proto = nfnl_nest(&req->nlh, size, CTA_PROTOINFO_SCTP);
 		if (test_bit(ATTR_SCTP_STATE, ct->set))
@@ -158,6 +176,12 @@ static void __build_protoinfo(struct nfnlhdr *req, size_t size,
 		nfnl_nest_end(&req->nlh, nest);
 		break;
 	case IPPROTO_DCCP:
+		/* See comment above on TCP. */
+		if (!(test_bit(ATTR_DCCP_STATE, ct->set) ||
+		      test_bit(ATTR_DCCP_ROLE, ct->set) ||
+		      test_bit(ATTR_DCCP_HANDSHAKE_SEQ, ct->set))) {
+			break;
+		}
 		nest = nfnl_nest(&req->nlh, size, CTA_PROTOINFO);
 		nest_proto = nfnl_nest(&req->nlh, size, CTA_PROTOINFO_DCCP);
 		if (test_bit(ATTR_DCCP_STATE, ct->set))

Re: TCP proto info

[rui.sousa]

-----Pablo Neira Ayuso <pablo@netfilter.org> wrote: -----

>On 02/09/10 11:21, rui.sousa@mindspeed.com wrote:
>> Pablo Neira Ayuso  wrote on 09/02/2010
>10:57:39 AM:
>>
>>> Hi Rui,
>>
>> Hi Pablo,
>>
>>> On 01/09/10 15:45, rui.sousa@mindspeed.com wrote:
>>>> Hi,
>>>>
>>>> I have an application using libnetfilter_conntrack-0.100 that
>started
>>>> reporting errors after the commit:
>>>>
>>>> 1c450e1595afdc8d1bfabb4f640c9251808426eb.
>>>
>>> Looking at the source code, this seems to be already fixed in
>>> libnetfilter_conntrack 0.0.102, please upgrade to latest.
>>
>> Hmm... looking at the git tree I see that the __build_conntrack()
>code is
>> still calling __build_protoinfo() unconditionally and inside the
>function
>> we always do:
>>
>> nest = nfnl_nest(&req->nlh, size, CTA_PROTOINFO);
>> nest_proto = nfnl_nest(&req->nlh, size, CTA_PROTOINFO_TCP);
>> ...
>> nfnl_nest_end(&req->nlh, nest_proto);
>> nfnl_nest_end(&req->nlh, nest);
>>
>> even if none of the ATTR_TCP_xxx bits are set. This is what causes
>the
>> kernel to return -EINVAL
>> and ignore the conntrack update. Or am I missing something?
>
>I see, I guess that you're using a Linux kernel <= 2.6.25

Correct. It's an ARM based embedded linux system still running 2.6.21.

> since I
>couldn't reproduce it with recent kernels. Please, could you give a
>try
>to the following patch?

This patch fixed my problem.

Thanks,
Rui

--
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