RE: kernel stack trace using conntrack

RE: kernel stack trace using conntrack

[Eric Dumazet]

Le mardi 16 février 2010 à 11:25 +0100, Ramblewski David a écrit :
> Here it is:
> 
> [root@obench03s ~]# uname -a
> Linux obench03s 2.6.32.8 #1 SMP PREEMPT Wed Feb 10 17:32:16 CET 2010 i686 i686 i386 GNU/Linux
> 
> [root@obench03s ~]# lsmod
> Module                  Size  Used by
> nf_conntrack_netlink     9810  0
> nf_conntrack           50283  1 nf_conntrack_netlink
> bonding                69153  0
> usbhid                 14557  0
> ipmi_si                29902  0
> rtc_cmos                7175  0
> bnx2                   53497  0
> hpwdt                   5548  0
> rtc_core               11461  1 rtc_cmos
> rtc_lib                 2022  1 rtc_core
> hpilo                   6051  0
> ipmi_msghandler        27670  1 ipmi_si
> ehci_hcd               27584  0
> uhci_hcd               16357  0
> usbcore               120714  4 usbhid,ehci_hcd,uhci_hcd
> dm_mod                 49320  0
> [root@obench03s ~]#
> 
> 
> [root@obench03s ~]# iptables --version
> iptables v1.4.0
> [root@obench03s ~]# iptables -L
> Chain INPUT (policy ACCEPT)
> target     prot opt source               destination
> 
> Chain FORWARD (policy ACCEPT)
> target     prot opt source               destination
> 
> Chain OUTPUT (policy ACCEPT)
> target     prot opt source               destination
> [root@obench03s ~]# iptables -L -t nat
> Chain PREROUTING (policy ACCEPT)
> target     prot opt source               destination
> 
> Chain POSTROUTING (policy ACCEPT)
> target     prot opt source               destination
> 
> Chain OUTPUT (policy ACCEPT)
> target     prot opt source               destination
> [root@obench03s ~]# conntrack -L
> udp      17 27 src=10.24.230.151 dst=10.26.103.28 sport=510 dport=516 packets=5 bytes=295 [UNREPLIED] src=10.26.103.28 dst=10.24.230.151 sport=516 dport=510 packets=0 bytes=0 mark=0 secmark=0 use=2
> tcp      6 299 ESTABLISHED src=10.26.103.28 dst=10.28.64.65 sport=22 dport=53497 packets=89 bytes=8628 src=10.28.64.65 dst=10.26.103.28 sport=53497 dport=22 packets=127 bytes=10396 [ASSURED] mark=0 secmark=0 use=2
> udp      17 21 src=127.0.0.1 dst=127.0.0.1 sport=32070 dport=161 packets=6 bytes=492 [UNREPLIED] src=127.0.0.1 dst=127.0.0.1 sport=161 dport=32070 packets=0 bytes=0 mark=0 secmark=0 use=2
> conntrack v0.9.14 (conntrack-tools): 3 flow entries have been shown.
> [root@obench03s ~]#
> 
> David

OK thanks David, I reproduced the problem on latest net-next-2.6 tree
too. I wonder why nobody hit this before.


[352468.556484] ------------[ cut here ]------------
[352468.556511] WARNING: at net/netfilter/nf_conntrack_extend.c:82
__nf_ct_ext_add+0x1c2/0x1e0 [nf_conntrack]()
[352468.556559] Hardware name: ProLiant BL460c G1
[352468.556582] Modules linked in: nf_defrag_ipv4 nf_conntrack_netlink
nf_conntrack sch_hfsc sch_sfq ipmi_devintf ipmi_si ipmi_msghandler hpilo
bonding [last unloaded: nf_conntrack_ipv4]
[352468.556675] Pid: 18852, comm: conntrack Tainted: G        W
2.6.33-rc5-02754-g0ea034c-dirty #545
[352468.556721] Call Trace:
[352468.556742]  [<c054d45f>] ? printk+0x1d/0x26
[352468.556767]  [<c023bbc2>] warn_slowpath_common+0x72/0xa0
[352468.556795]  [<fee75e42>] ? __nf_ct_ext_add+0x1c2/0x1e0
[nf_conntrack]
[352468.556825]  [<fee75e42>] ? __nf_ct_ext_add+0x1c2/0x1e0
[nf_conntrack]
[352468.556854]  [<c023bc0a>] warn_slowpath_null+0x1a/0x20
[352468.556882]  [<fee75e42>] __nf_ct_ext_add+0x1c2/0x1e0 [nf_conntrack]
[352468.556911]  [<fee70dcc>] ? nf_conntrack_alloc+0x10c/0x1a0
[nf_conntrack]
[352468.556940]  [<feecaf59>] ctnetlink_create_conntrack+0x339/0x360
[nf_conntrack_netlink]
[352468.556987]  [<feeca26b>] ? ctnetlink_parse_tuple+0x14b/0x1c0
[nf_conntrack_netlink]
[352468.557039]  [<fee6fd60>] ? __nf_conntrack_find+0x70/0x100
[nf_conntrack]
[352468.557068]  [<feecb090>] ctnetlink_new_conntrack+0x110/0x680
[nf_conntrack_netlink]
[352468.557113]  [<c04d93b5>] nfnetlink_rcv_msg+0x125/0x180
[352468.557140]  [<c054ec57>] ? __mutex_lock_slowpath+0x197/0x230
[352468.557167]  [<c04d9290>] ? nfnetlink_rcv_msg+0x0/0x180
[352468.557194]  [<c04d5896>] netlink_rcv_skb+0x96/0xc0
[352468.557219]  [<c04d927c>] nfnetlink_rcv+0x1c/0x30
[352468.557245]  [<c04d5545>] netlink_unicast+0x255/0x2a0
[352468.557274]  [<c04d5d3f>] netlink_sendmsg+0x1af/0x2b0
[352468.557300]  [<c04a86ec>] sock_sendmsg+0xac/0xe0
[352468.559358]  [<c029d042>] ? find_get_page+0x22/0xd0
[352468.559385]  [<c029d9dc>] ? filemap_fault+0x8c/0x3c0
[352468.559410]  [<c04a905a>] sys_sendto+0xaa/0xd0
[352468.559436]  [<c02b3780>] ? __do_fault+0x370/0x470
[352468.559462]  [<c02b54d9>] ? handle_mm_fault+0x1d9/0x7d0
[352468.559488]  [<c04aa245>] sys_socketcall+0x195/0x280
[352468.559514]  [<c0202c50>] sysenter_do_call+0x12/0x26
[352468.559539] ---[ end trace 6ecb842e4e35a653 ]---

Could you try following patch ?

Thanks

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 0ffe689..d2657aa 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -923,7 +923,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
 	unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
 	d = ct->status ^ status;
 
-	if (d & (IPS_EXPECTED|IPS_CONFIRMED|IPS_DYING))
+	if (d & (IPS_EXPECTED|IPS_DYING))
 		/* unchangeable */
 		return -EBUSY;
 
@@ -938,7 +938,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
 	/* Be careful here, modifying NAT bits can screw up things,
 	 * so don't let users modify them directly if they don't pass
 	 * nf_nat_range. */
-	ct->status |= status & ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK);
+	ct->status |= status & ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK | IPS_CONFIRMED);
 	return 0;
 }
 
@@ -1193,7 +1193,6 @@ ctnetlink_create_conntrack(const struct nlattr * const cda[],
 	ct->timeout.expires = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
 
 	ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
-	ct->status |= IPS_CONFIRMED;
 
 	rcu_read_lock();
  	if (cda[CTA_HELP]) {
@@ -1296,6 +1295,7 @@ ctnetlink_create_conntrack(const struct nlattr * const cda[],
 	}
 
 	add_timer(&ct->timeout);
+	ct->status |= IPS_CONFIRMED;
 	nf_conntrack_hash_insert(ct);
 	rcu_read_unlock();
 


--
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: kernel stack trace using conntrack

[Pablo Neira Ayuso]

Eric Dumazet wrote:
> OK thanks David, I reproduced the problem on latest net-next-2.6 tree
> too. I wonder why nobody hit this before.

Hmm, my config had not NETFILTER_DEBUG enabled, that's why I didn't hit
that assertion.

> [352468.556484] ------------[ cut here ]------------
> [352468.556511] WARNING: at net/netfilter/nf_conntrack_extend.c:82
> __nf_ct_ext_add+0x1c2/0x1e0 [nf_conntrack]()
> [352468.556559] Hardware name: ProLiant BL460c G1
> [352468.556582] Modules linked in: nf_defrag_ipv4 nf_conntrack_netlink
> nf_conntrack sch_hfsc sch_sfq ipmi_devintf ipmi_si ipmi_msghandler hpilo
> bonding [last unloaded: nf_conntrack_ipv4]
> [352468.556675] Pid: 18852, comm: conntrack Tainted: G        W
> 2.6.33-rc5-02754-g0ea034c-dirty #545
> [352468.556721] Call Trace:
> [352468.556742]  [<c054d45f>] ? printk+0x1d/0x26
> [352468.556767]  [<c023bbc2>] warn_slowpath_common+0x72/0xa0
> [352468.556795]  [<fee75e42>] ? __nf_ct_ext_add+0x1c2/0x1e0
> [nf_conntrack]
> [352468.556825]  [<fee75e42>] ? __nf_ct_ext_add+0x1c2/0x1e0
> [nf_conntrack]
> [352468.556854]  [<c023bc0a>] warn_slowpath_null+0x1a/0x20
> [352468.556882]  [<fee75e42>] __nf_ct_ext_add+0x1c2/0x1e0 [nf_conntrack]
> [352468.556911]  [<fee70dcc>] ? nf_conntrack_alloc+0x10c/0x1a0
> [nf_conntrack]
> [352468.556940]  [<feecaf59>] ctnetlink_create_conntrack+0x339/0x360
> [nf_conntrack_netlink]
> [352468.556987]  [<feeca26b>] ? ctnetlink_parse_tuple+0x14b/0x1c0
> [nf_conntrack_netlink]
> [352468.557039]  [<fee6fd60>] ? __nf_conntrack_find+0x70/0x100
> [nf_conntrack]
> [352468.557068]  [<feecb090>] ctnetlink_new_conntrack+0x110/0x680
> [nf_conntrack_netlink]
> [352468.557113]  [<c04d93b5>] nfnetlink_rcv_msg+0x125/0x180
> [352468.557140]  [<c054ec57>] ? __mutex_lock_slowpath+0x197/0x230
> [352468.557167]  [<c04d9290>] ? nfnetlink_rcv_msg+0x0/0x180
> [352468.557194]  [<c04d5896>] netlink_rcv_skb+0x96/0xc0
> [352468.557219]  [<c04d927c>] nfnetlink_rcv+0x1c/0x30
> [352468.557245]  [<c04d5545>] netlink_unicast+0x255/0x2a0
> [352468.557274]  [<c04d5d3f>] netlink_sendmsg+0x1af/0x2b0
> [352468.557300]  [<c04a86ec>] sock_sendmsg+0xac/0xe0
> [352468.559358]  [<c029d042>] ? find_get_page+0x22/0xd0
> [352468.559385]  [<c029d9dc>] ? filemap_fault+0x8c/0x3c0
> [352468.559410]  [<c04a905a>] sys_sendto+0xaa/0xd0
> [352468.559436]  [<c02b3780>] ? __do_fault+0x370/0x470
> [352468.559462]  [<c02b54d9>] ? handle_mm_fault+0x1d9/0x7d0
> [352468.559488]  [<c04aa245>] sys_socketcall+0x195/0x280
> [352468.559514]  [<c0202c50>] sysenter_do_call+0x12/0x26
> [352468.559539] ---[ end trace 6ecb842e4e35a653 ]---
> 
> Could you try following patch ?
> 
> Thanks
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 0ffe689..d2657aa 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -923,7 +923,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
>  	unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
>  	d = ct->status ^ status;
>  
> -	if (d & (IPS_EXPECTED|IPS_CONFIRMED|IPS_DYING))
> +	if (d & (IPS_EXPECTED|IPS_DYING))
>  		/* unchangeable */
>  		return -EBUSY;

I think that we should explicitly report if the user unsets
IPS_CONFIRMED. Please, don't change this.

Apart from that, the patch seems fine to me. Thanks!

Re: kernel stack trace using conntrack

[Eric Dumazet]

Le mardi 16 février 2010 à 14:33 +0100, Pablo Neira Ayuso a écrit :
> Eric Dumazet wrote:
> > OK thanks David, I reproduced the problem on latest net-next-2.6 tree
> > too. I wonder why nobody hit this before.
> 
> Hmm, my config had not NETFILTER_DEBUG enabled, that's why I didn't hit
> that assertion.
> 
> > [352468.556484] ------------[ cut here ]------------
> > [352468.556511] WARNING: at net/netfilter/nf_conntrack_extend.c:82
> > __nf_ct_ext_add+0x1c2/0x1e0 [nf_conntrack]()
> > [352468.556559] Hardware name: ProLiant BL460c G1
> > [352468.556582] Modules linked in: nf_defrag_ipv4 nf_conntrack_netlink
> > nf_conntrack sch_hfsc sch_sfq ipmi_devintf ipmi_si ipmi_msghandler hpilo
> > bonding [last unloaded: nf_conntrack_ipv4]
> > [352468.556675] Pid: 18852, comm: conntrack Tainted: G        W
> > 2.6.33-rc5-02754-g0ea034c-dirty #545
> > [352468.556721] Call Trace:
> > [352468.556742]  [<c054d45f>] ? printk+0x1d/0x26
> > [352468.556767]  [<c023bbc2>] warn_slowpath_common+0x72/0xa0
> > [352468.556795]  [<fee75e42>] ? __nf_ct_ext_add+0x1c2/0x1e0
> > [nf_conntrack]
> > [352468.556825]  [<fee75e42>] ? __nf_ct_ext_add+0x1c2/0x1e0
> > [nf_conntrack]
> > [352468.556854]  [<c023bc0a>] warn_slowpath_null+0x1a/0x20
> > [352468.556882]  [<fee75e42>] __nf_ct_ext_add+0x1c2/0x1e0 [nf_conntrack]
> > [352468.556911]  [<fee70dcc>] ? nf_conntrack_alloc+0x10c/0x1a0
> > [nf_conntrack]
> > [352468.556940]  [<feecaf59>] ctnetlink_create_conntrack+0x339/0x360
> > [nf_conntrack_netlink]
> > [352468.556987]  [<feeca26b>] ? ctnetlink_parse_tuple+0x14b/0x1c0
> > [nf_conntrack_netlink]
> > [352468.557039]  [<fee6fd60>] ? __nf_conntrack_find+0x70/0x100
> > [nf_conntrack]
> > [352468.557068]  [<feecb090>] ctnetlink_new_conntrack+0x110/0x680
> > [nf_conntrack_netlink]
> > [352468.557113]  [<c04d93b5>] nfnetlink_rcv_msg+0x125/0x180
> > [352468.557140]  [<c054ec57>] ? __mutex_lock_slowpath+0x197/0x230
> > [352468.557167]  [<c04d9290>] ? nfnetlink_rcv_msg+0x0/0x180
> > [352468.557194]  [<c04d5896>] netlink_rcv_skb+0x96/0xc0
> > [352468.557219]  [<c04d927c>] nfnetlink_rcv+0x1c/0x30
> > [352468.557245]  [<c04d5545>] netlink_unicast+0x255/0x2a0
> > [352468.557274]  [<c04d5d3f>] netlink_sendmsg+0x1af/0x2b0
> > [352468.557300]  [<c04a86ec>] sock_sendmsg+0xac/0xe0
> > [352468.559358]  [<c029d042>] ? find_get_page+0x22/0xd0
> > [352468.559385]  [<c029d9dc>] ? filemap_fault+0x8c/0x3c0
> > [352468.559410]  [<c04a905a>] sys_sendto+0xaa/0xd0
> > [352468.559436]  [<c02b3780>] ? __do_fault+0x370/0x470
> > [352468.559462]  [<c02b54d9>] ? handle_mm_fault+0x1d9/0x7d0
> > [352468.559488]  [<c04aa245>] sys_socketcall+0x195/0x280
> > [352468.559514]  [<c0202c50>] sysenter_do_call+0x12/0x26
> > [352468.559539] ---[ end trace 6ecb842e4e35a653 ]---
> > 
> > Could you try following patch ?
> > 
> > Thanks
> > 
> > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > index 0ffe689..d2657aa 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -923,7 +923,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
> >  	unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
> >  	d = ct->status ^ status;
> >  
> > -	if (d & (IPS_EXPECTED|IPS_CONFIRMED|IPS_DYING))
> > +	if (d & (IPS_EXPECTED|IPS_DYING))
> >  		/* unchangeable */
> >  		return -EBUSY;
> 
> I think that we should explicitly report if the user unsets
> IPS_CONFIRMED. Please, don't change this.
> 
> Apart from that, the patch seems fine to me. Thanks!

Problem is we now (I mean after my patch) enter
ctnetlink_change_status() with ct->status being null (or at least,
IPS_CONFIRMED not set)



--
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: kernel stack trace using conntrack

[Ramblewski David]

Hi Eric,

The conntrack patch works successfully.

Thanks for your help!

David

-----Message d'origine-----
De : Eric Dumazet [mailto:eric.dumazet@gmail.com]
Envoyé : mardi 16 février 2010 14:45
À : Pablo Neira Ayuso
Cc : Ramblewski David; netfilter-devel@vger.kernel.org; netdev
Objet : Re: kernel stack trace using conntrack

Le mardi 16 février 2010 à 14:33 +0100, Pablo Neira Ayuso a écrit :
> Eric Dumazet wrote:
> > OK thanks David, I reproduced the problem on latest net-next-2.6 tree
> > too. I wonder why nobody hit this before.
>
> Hmm, my config had not NETFILTER_DEBUG enabled, that's why I didn't hit
> that assertion.
>
> > [352468.556484] ------------[ cut here ]------------
> > [352468.556511] WARNING: at net/netfilter/nf_conntrack_extend.c:82
> > __nf_ct_ext_add+0x1c2/0x1e0 [nf_conntrack]()
> > [352468.556559] Hardware name: ProLiant BL460c G1
> > [352468.556582] Modules linked in: nf_defrag_ipv4 nf_conntrack_netlink
> > nf_conntrack sch_hfsc sch_sfq ipmi_devintf ipmi_si ipmi_msghandler hpilo
> > bonding [last unloaded: nf_conntrack_ipv4]
> > [352468.556675] Pid: 18852, comm: conntrack Tainted: G        W
> > 2.6.33-rc5-02754-g0ea034c-dirty #545
> > [352468.556721] Call Trace:
> > [352468.556742]  [<c054d45f>] ? printk+0x1d/0x26
> > [352468.556767]  [<c023bbc2>] warn_slowpath_common+0x72/0xa0
> > [352468.556795]  [<fee75e42>] ? __nf_ct_ext_add+0x1c2/0x1e0
> > [nf_conntrack]
> > [352468.556825]  [<fee75e42>] ? __nf_ct_ext_add+0x1c2/0x1e0
> > [nf_conntrack]
> > [352468.556854]  [<c023bc0a>] warn_slowpath_null+0x1a/0x20
> > [352468.556882]  [<fee75e42>] __nf_ct_ext_add+0x1c2/0x1e0 [nf_conntrack]
> > [352468.556911]  [<fee70dcc>] ? nf_conntrack_alloc+0x10c/0x1a0
> > [nf_conntrack]
> > [352468.556940]  [<feecaf59>] ctnetlink_create_conntrack+0x339/0x360
> > [nf_conntrack_netlink]
> > [352468.556987]  [<feeca26b>] ? ctnetlink_parse_tuple+0x14b/0x1c0
> > [nf_conntrack_netlink]
> > [352468.557039]  [<fee6fd60>] ? __nf_conntrack_find+0x70/0x100
> > [nf_conntrack]
> > [352468.557068]  [<feecb090>] ctnetlink_new_conntrack+0x110/0x680
> > [nf_conntrack_netlink]
> > [352468.557113]  [<c04d93b5>] nfnetlink_rcv_msg+0x125/0x180
> > [352468.557140]  [<c054ec57>] ? __mutex_lock_slowpath+0x197/0x230
> > [352468.557167]  [<c04d9290>] ? nfnetlink_rcv_msg+0x0/0x180
> > [352468.557194]  [<c04d5896>] netlink_rcv_skb+0x96/0xc0
> > [352468.557219]  [<c04d927c>] nfnetlink_rcv+0x1c/0x30
> > [352468.557245]  [<c04d5545>] netlink_unicast+0x255/0x2a0
> > [352468.557274]  [<c04d5d3f>] netlink_sendmsg+0x1af/0x2b0
> > [352468.557300]  [<c04a86ec>] sock_sendmsg+0xac/0xe0
> > [352468.559358]  [<c029d042>] ? find_get_page+0x22/0xd0
> > [352468.559385]  [<c029d9dc>] ? filemap_fault+0x8c/0x3c0
> > [352468.559410]  [<c04a905a>] sys_sendto+0xaa/0xd0
> > [352468.559436]  [<c02b3780>] ? __do_fault+0x370/0x470
> > [352468.559462]  [<c02b54d9>] ? handle_mm_fault+0x1d9/0x7d0
> > [352468.559488]  [<c04aa245>] sys_socketcall+0x195/0x280
> > [352468.559514]  [<c0202c50>] sysenter_do_call+0x12/0x26
> > [352468.559539] ---[ end trace 6ecb842e4e35a653 ]---
> >
> > Could you try following patch ?
> >
> > Thanks
> >
> > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > index 0ffe689..d2657aa 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -923,7 +923,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
> >     unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
> >     d = ct->status ^ status;
> >
> > -   if (d & (IPS_EXPECTED|IPS_CONFIRMED|IPS_DYING))
> > +   if (d & (IPS_EXPECTED|IPS_DYING))
> >             /* unchangeable */
> >             return -EBUSY;
>
> I think that we should explicitly report if the user unsets
> IPS_CONFIRMED. Please, don't change this.
>
> Apart from that, the patch seems fine to me. Thanks!

Problem is we now (I mean after my patch) enter
ctnetlink_change_status() with ct->status being null (or at least,
IPS_CONFIRMED not set)






Ce message et les pièces jointes sont confidentiels et réservés à l'usage exclusif de ses destinataires. Il peut également être protégé par le secret professionnel. Si vous recevez ce message par erreur, merci d'en avertir immédiatement l'expéditeur et de le détruire. L'intégrité du message ne pouvant être assurée sur Internet, la responsabilité du groupe Atos Origin ne pourra être recherchée quant au contenu de ce message. Bien que les meilleurs efforts soient faits pour maintenir cette transmission exempte de tout virus, l'expéditeur ne donne aucune garantie à cet égard et sa responsabilité ne saurait être recherchée pour tout dommage résultant d'un virus transmis.

This e-mail and the documents attached are confidential and intended solely for the addressee; it may also be privileged. If you receive this e-mail in error, please notify the sender immediately and destroy it. As its integrity cannot be secured on the Internet, the Atos Origin group liability cannot be triggered for the message content. Although the sender endeavours to maintain a computer virus-free network, the sender does not warrant that this transmission is virus-free and will not be liable for any damages resulting from any virus transmitted.

Re: kernel stack trace using conntrack

[Patrick McHardy]

Ramblewski David wrote:
> Hi Eric,
> 
> The conntrack patch works successfully.
> 
>>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>>> index 0ffe689..d2657aa 100644
>>> --- a/net/netfilter/nf_conntrack_netlink.c
>>> +++ b/net/netfilter/nf_conntrack_netlink.c
>>> @@ -923,7 +923,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
>>>     unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
>>>     d = ct->status ^ status;
>>>
>>> -   if (d & (IPS_EXPECTED|IPS_CONFIRMED|IPS_DYING))
>>> +   if (d & (IPS_EXPECTED|IPS_DYING))
>>>             /* unchangeable */
>>>             return -EBUSY;
>> I think that we should explicitly report if the user unsets
>> IPS_CONFIRMED. Please, don't change this.
>>
>> Apart from that, the patch seems fine to me. Thanks!
> 
> Problem is we now (I mean after my patch) enter
> ctnetlink_change_status() with ct->status being null (or at least,
> IPS_CONFIRMED not set)

Pablo, please let me know whether you want me to apply this.

Re: kernel stack trace using conntrack

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Ramblewski David wrote:
>> Hi Eric,
>>
>> The conntrack patch works successfully.
>>
>>>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>>>> index 0ffe689..d2657aa 100644
>>>> --- a/net/netfilter/nf_conntrack_netlink.c
>>>> +++ b/net/netfilter/nf_conntrack_netlink.c
>>>> @@ -923,7 +923,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
>>>>     unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
>>>>     d = ct->status ^ status;
>>>>
>>>> -   if (d & (IPS_EXPECTED|IPS_CONFIRMED|IPS_DYING))
>>>> +   if (d & (IPS_EXPECTED|IPS_DYING))
>>>>             /* unchangeable */
>>>>             return -EBUSY;
>>> I think that we should explicitly report if the user unsets
>>> IPS_CONFIRMED. Please, don't change this.
>>>
>>> Apart from that, the patch seems fine to me. Thanks!
>> Problem is we now (I mean after my patch) enter
>> ctnetlink_change_status() with ct->status being null (or at least,
>> IPS_CONFIRMED not set)
> 
> Pablo, please let me know whether you want me to apply this.

ctnetlink_change_helper() also calls nf_ct_ext_add() for conntracks that
are confirmed (in case of a helper update for an existing conntrack).
That would also trigger the assertion. If we want to support helper
assignation via ctnetlink for existing conntracks, we will need to add
locking to the conntrack extension infrastructure to avoid races.

I don't see a clear solution for this yet.

Re: kernel stack trace using conntrack

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> Ramblewski David wrote:
>>> Hi Eric,
>>>
>>> The conntrack patch works successfully.
>>>
>>>>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>>>>> index 0ffe689..d2657aa 100644
>>>>> --- a/net/netfilter/nf_conntrack_netlink.c
>>>>> +++ b/net/netfilter/nf_conntrack_netlink.c
>>>>> @@ -923,7 +923,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
>>>>>     unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
>>>>>     d = ct->status ^ status;
>>>>>
>>>>> -   if (d & (IPS_EXPECTED|IPS_CONFIRMED|IPS_DYING))
>>>>> +   if (d & (IPS_EXPECTED|IPS_DYING))
>>>>>             /* unchangeable */
>>>>>             return -EBUSY;
>>>> I think that we should explicitly report if the user unsets
>>>> IPS_CONFIRMED. Please, don't change this.
>>>>
>>>> Apart from that, the patch seems fine to me. Thanks!
>>> Problem is we now (I mean after my patch) enter
>>> ctnetlink_change_status() with ct->status being null (or at least,
>>> IPS_CONFIRMED not set)
>> Pablo, please let me know whether you want me to apply this.
> 
> ctnetlink_change_helper() also calls nf_ct_ext_add() for conntracks that
> are confirmed (in case of a helper update for an existing conntrack).
> That would also trigger the assertion. If we want to support helper
> assignation via ctnetlink for existing conntracks, we will need to add
> locking to the conntrack extension infrastructure to avoid races.
> 
> I don't see a clear solution for this yet.

I see, this is indeed a problem. Since the helper is known at the
first event, we could restrict this to only allow manual assignment
for newly created conntracks. Most helpers probably can't properly
cope with connections not seen from the beginning anyways.

Re: kernel stack trace using conntrack

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Patrick McHardy wrote:
>>> Ramblewski David wrote:
>>>> Hi Eric,
>>>>
>>>> The conntrack patch works successfully.
>>>>
>>>>>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>>>>>> index 0ffe689..d2657aa 100644
>>>>>> --- a/net/netfilter/nf_conntrack_netlink.c
>>>>>> +++ b/net/netfilter/nf_conntrack_netlink.c
>>>>>> @@ -923,7 +923,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
>>>>>>     unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
>>>>>>     d = ct->status ^ status;
>>>>>>
>>>>>> -   if (d & (IPS_EXPECTED|IPS_CONFIRMED|IPS_DYING))
>>>>>> +   if (d & (IPS_EXPECTED|IPS_DYING))
>>>>>>             /* unchangeable */
>>>>>>             return -EBUSY;
>>>>> I think that we should explicitly report if the user unsets
>>>>> IPS_CONFIRMED. Please, don't change this.
>>>>>
>>>>> Apart from that, the patch seems fine to me. Thanks!
>>>> Problem is we now (I mean after my patch) enter
>>>> ctnetlink_change_status() with ct->status being null (or at least,
>>>> IPS_CONFIRMED not set)
>>> Pablo, please let me know whether you want me to apply this.
>> ctnetlink_change_helper() also calls nf_ct_ext_add() for conntracks that
>> are confirmed (in case of a helper update for an existing conntrack).
>> That would also trigger the assertion. If we want to support helper
>> assignation via ctnetlink for existing conntracks, we will need to add
>> locking to the conntrack extension infrastructure to avoid races.
>>
>> I don't see a clear solution for this yet.
> 
> I see, this is indeed a problem. Since the helper is known at the
> first event, we could restrict this to only allow manual assignment
> for newly created conntracks. Most helpers probably can't properly
> cope with connections not seen from the beginning anyways.

Indeed, changing the helper in the middle of the road doesn't make too
much sense to me either. I can send you a patch for this along today,
I'll find some spare time to do it.

Re: kernel stack trace using conntrack

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>>>> Pablo, please let me know whether you want me to apply this.
>>> ctnetlink_change_helper() also calls nf_ct_ext_add() for conntracks that
>>> are confirmed (in case of a helper update for an existing conntrack).
>>> That would also trigger the assertion. If we want to support helper
>>> assignation via ctnetlink for existing conntracks, we will need to add
>>> locking to the conntrack extension infrastructure to avoid races.
>>>
>>> I don't see a clear solution for this yet.
>> I see, this is indeed a problem. Since the helper is known at the
>> first event, we could restrict this to only allow manual assignment
>> for newly created conntracks. Most helpers probably can't properly
>> cope with connections not seen from the beginning anyways.
> 
> Indeed, changing the helper in the middle of the road doesn't make too
> much sense to me either. I can send you a patch for this along today,
> I'll find some spare time to do it.

Great, thanks Pablo.

Re: kernel stack trace using conntrack

[Pablo Neira Ayuso]

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

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Patrick McHardy wrote:
>>>>> Pablo, please let me know whether you want me to apply this.
>>>> ctnetlink_change_helper() also calls nf_ct_ext_add() for conntracks that
>>>> are confirmed (in case of a helper update for an existing conntrack).
>>>> That would also trigger the assertion. If we want to support helper
>>>> assignation via ctnetlink for existing conntracks, we will need to add
>>>> locking to the conntrack extension infrastructure to avoid races.
>>>>
>>>> I don't see a clear solution for this yet.
>>> I see, this is indeed a problem. Since the helper is known at the
>>> first event, we could restrict this to only allow manual assignment
>>> for newly created conntracks. Most helpers probably can't properly
>>> cope with connections not seen from the beginning anyways.
>> Indeed, changing the helper in the middle of the road doesn't make too
>> much sense to me either. I can send you a patch for this along today,
>> I'll find some spare time to do it.
> 
> Great, thanks Pablo.

I have slightly tested the following patch here. I think it should fix
the problem.

We can revisit ctnetlink_change_helper() later, I think there's some
code there that can be refactorized.

Let me know if you're OK with it.

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

netfilter: ctnetlink: fix creation of conntrack with helpers

This patch fixes a bug that triggers an assertion if you create
a conntrack entry with a helper and netfilter debugging is enabled.
Basically, we hit the assertion because the confirmation flag is
set before the conntrack extensions are added. To fix this, we
move the extension addition before the aforementioned flag is
set.

This patch also removes the possibility of setting a helper for
existing conntracks. This operation would also trigger the
assertion since we are not allowed to add new extensions for
existing conntracks. We know noone that could benefit from
this operation sanely.

Thanks to Eric Dumazet for initial posting a preliminary patch
to address this issue.

Reported-by: David Ramblewski <David.Ramblewski@atosorigin.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_netlink.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 8b05f36..2b2af63 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1077,9 +1077,8 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
 		/* need to zero data of old helper */
 		memset(&help->help, 0, sizeof(help->help));
 	} else {
-		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-		if (help == NULL)
-			return -ENOMEM;
+		/* we cannot set a helper for an existing conntrack */
+		return -EOPNOTSUPP;
 	}
 
 	rcu_assign_pointer(help->helper, helper);
@@ -1263,7 +1262,6 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
 	ct->timeout.expires = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
 
 	ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
-	ct->status |= IPS_CONFIRMED;
 
 	rcu_read_lock();
  	if (cda[CTA_HELP]) {
@@ -1314,14 +1312,19 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
 			goto err2;
 	}
 
-	if (cda[CTA_STATUS]) {
-		err = ctnetlink_change_status(ct, cda);
+	if (cda[CTA_NAT_SRC] || cda[CTA_NAT_DST]) {
+		err = ctnetlink_change_nat(ct, cda);
 		if (err < 0)
 			goto err2;
 	}
 
-	if (cda[CTA_NAT_SRC] || cda[CTA_NAT_DST]) {
-		err = ctnetlink_change_nat(ct, cda);
+	nf_ct_acct_ext_add(ct, GFP_ATOMIC);
+	nf_ct_ecache_ext_add(ct, 0, 0, GFP_ATOMIC);
+	/* we must add conntrack extensions before confirmation. */
+	ct->status |= IPS_CONFIRMED;
+
+	if (cda[CTA_STATUS]) {
+		err = ctnetlink_change_status(ct, cda);
 		if (err < 0)
 			goto err2;
 	}
@@ -1340,9 +1343,6 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
 			goto err2;
 	}
 
-	nf_ct_acct_ext_add(ct, GFP_ATOMIC);
-	nf_ct_ecache_ext_add(ct, 0, 0, GFP_ATOMIC);
-
 #if defined(CONFIG_NF_CONNTRACK_MARK)
 	if (cda[CTA_MARK])
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));

Re: kernel stack trace using conntrack

[Eric Dumazet]

Le vendredi 19 février 2010 à 03:18 +0100, Pablo Neira Ayuso a écrit :
> Patrick McHardy wrote:
> > Pablo Neira Ayuso wrote:
> >> Patrick McHardy wrote:
> >>>>> Pablo, please let me know whether you want me to apply this.
> >>>> ctnetlink_change_helper() also calls nf_ct_ext_add() for conntracks that
> >>>> are confirmed (in case of a helper update for an existing conntrack).
> >>>> That would also trigger the assertion. If we want to support helper
> >>>> assignation via ctnetlink for existing conntracks, we will need to add
> >>>> locking to the conntrack extension infrastructure to avoid races.
> >>>>
> >>>> I don't see a clear solution for this yet.
> >>> I see, this is indeed a problem. Since the helper is known at the
> >>> first event, we could restrict this to only allow manual assignment
> >>> for newly created conntracks. Most helpers probably can't properly
> >>> cope with connections not seen from the beginning anyways.
> >> Indeed, changing the helper in the middle of the road doesn't make too
> >> much sense to me either. I can send you a patch for this along today,
> >> I'll find some spare time to do it.
> > 
> > Great, thanks Pablo.
> 
> I have slightly tested the following patch here. I think it should fix
> the problem.
> 
> We can revisit ctnetlink_change_helper() later, I think there's some
> code there that can be refactorized.
> 
> Let me know if you're OK with it.

I sucessfuly tested your patch Pablo, thanks.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: kernel stack trace using conntrack

[Patrick McHardy]

Eric Dumazet wrote:
> Le vendredi 19 février 2010 à 03:18 +0100, Pablo Neira Ayuso a écrit :
>> I have slightly tested the following patch here. I think it should fix
>> the problem.
>>
>> We can revisit ctnetlink_change_helper() later, I think there's some
>> code there that can be refactorized.
>>
>> Let me know if you're OK with it.
> 
> I sucessfuly tested your patch Pablo, thanks.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks everyone.