* [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len @ 2014-03-27 15:32 Andrey Vagin 2014-03-27 15:45 ` Patrick McHardy 2014-03-27 15:50 ` [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len Florian Westphal 0 siblings, 2 replies; 5+ messages in thread From: Andrey Vagin @ 2014-03-27 15:32 UTC (permalink / raw) To: linux-kernel Cc: netfilter-devel, netfilter, coreteam, netdev, vvs, Andrey Vagin, Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik, David S. Miller "len" contains sizeof(nf_ct_ext) and size of extensions. In a worst case it can contain all extensions. Bellow you can find sizes for all types of extensions. Their sum is definitely bigger than 256. nf_ct_ext_types[0]->len = 24 nf_ct_ext_types[1]->len = 32 nf_ct_ext_types[2]->len = 24 nf_ct_ext_types[3]->len = 32 nf_ct_ext_types[4]->len = 152 nf_ct_ext_types[5]->len = 2 nf_ct_ext_types[6]->len = 16 nf_ct_ext_types[7]->len = 8 I have seen "len" up to 280 and my host has crashes w/o this patch. Cc: Pablo Neira Ayuso <pablo@netfilter.org> Cc: Patrick McHardy <kaber@trash.net> Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> Cc: "David S. Miller" <davem@davemloft.net> Signed-off-by: Andrey Vagin <avagin@openvz.org> --- include/net/netfilter/nf_conntrack_extend.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h index 956b175..36fd6bf 100644 --- a/include/net/netfilter/nf_conntrack_extend.h +++ b/include/net/netfilter/nf_conntrack_extend.h @@ -48,7 +48,7 @@ enum nf_ct_ext_id { struct nf_ct_ext { struct rcu_head rcu; u8 offset[NF_CT_EXT_NUM]; - u8 len; + u16 len; char data[0]; }; -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len 2014-03-27 15:32 [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len Andrey Vagin @ 2014-03-27 15:45 ` Patrick McHardy 2014-03-27 20:06 ` [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len (v2) Andrey Vagin 2014-03-27 15:50 ` [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len Florian Westphal 1 sibling, 1 reply; 5+ messages in thread From: Patrick McHardy @ 2014-03-27 15:45 UTC (permalink / raw) To: Andrey Vagin Cc: linux-kernel, netfilter-devel, netfilter, coreteam, netdev, vvs, Pablo Neira Ayuso, Jozsef Kadlecsik, David S. Miller On Thu, Mar 27, 2014 at 07:32:34PM +0400, Andrey Vagin wrote: > "len" contains sizeof(nf_ct_ext) and size of extensions. In a worst > case it can contain all extensions. Bellow you can find sizes for all > types of extensions. Their sum is definitely bigger than 256. > > nf_ct_ext_types[0]->len = 24 > nf_ct_ext_types[1]->len = 32 > nf_ct_ext_types[2]->len = 24 > nf_ct_ext_types[3]->len = 32 > nf_ct_ext_types[4]->len = 152 > nf_ct_ext_types[5]->len = 2 > nf_ct_ext_types[6]->len = 16 > nf_ct_ext_types[7]->len = 8 > > I have seen "len" up to 280 and my host has crashes w/o this patch. Very nice catch. I suppose we also need to either increase the size of offset[] or rearrange the extension so 4 (ECACHE) comes last. > > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > Cc: Patrick McHardy <kaber@trash.net> > Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> > Cc: "David S. Miller" <davem@davemloft.net> > Signed-off-by: Andrey Vagin <avagin@openvz.org> > --- > include/net/netfilter/nf_conntrack_extend.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h > index 956b175..36fd6bf 100644 > --- a/include/net/netfilter/nf_conntrack_extend.h > +++ b/include/net/netfilter/nf_conntrack_extend.h > @@ -48,7 +48,7 @@ enum nf_ct_ext_id { > struct nf_ct_ext { > struct rcu_head rcu; > u8 offset[NF_CT_EXT_NUM]; > - u8 len; > + u16 len; > char data[0]; > }; > > -- > 1.8.5.3 > > -- > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len (v2) 2014-03-27 15:45 ` Patrick McHardy @ 2014-03-27 20:06 ` Andrey Vagin 0 siblings, 0 replies; 5+ messages in thread From: Andrey Vagin @ 2014-03-27 20:06 UTC (permalink / raw) To: linux-kernel Cc: netfilter-devel, netfilter, coreteam, netdev, vvs, Andrey Vagin, Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik, David S. Miller "len" contains sizeof(nf_ct_ext) and size of extensions. In a worst case it can contain all extensions. Bellow you can find sizes for all types of extensions. Their sum is definitely bigger than 256. nf_ct_ext_types[0]->len = 24 nf_ct_ext_types[1]->len = 32 nf_ct_ext_types[2]->len = 24 nf_ct_ext_types[3]->len = 32 nf_ct_ext_types[4]->len = 152 nf_ct_ext_types[5]->len = 2 nf_ct_ext_types[6]->len = 16 nf_ct_ext_types[7]->len = 8 I have seen "len" up to 280 and my host has crashes w/o this patch. The right way to fix this problem is reducing the size of the ecache extension (4) and Florian is going to do this, but these changes will be quite large to be appropriate for a stable tree. v2: rearrange the extension so ECACHE comes last. This is required to prevent overflow of nf_ct_ext->offset. Fixes: 5b423f6a40a0 (netfilter: nf_conntrack: fix racy timer handling with reliable) Cc: Pablo Neira Ayuso <pablo@netfilter.org> Cc: Patrick McHardy <kaber@trash.net> Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> Cc: "David S. Miller" <davem@davemloft.net> Signed-off-by: Andrey Vagin <avagin@openvz.org> --- include/net/netfilter/nf_conntrack_extend.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h index 956b175..9d41750 100644 --- a/include/net/netfilter/nf_conntrack_extend.h +++ b/include/net/netfilter/nf_conntrack_extend.h @@ -12,9 +12,6 @@ enum nf_ct_ext_id { #endif NF_CT_EXT_SEQADJ, NF_CT_EXT_ACCT, -#ifdef CONFIG_NF_CONNTRACK_EVENTS - NF_CT_EXT_ECACHE, -#endif #ifdef CONFIG_NF_CONNTRACK_ZONES NF_CT_EXT_ZONE, #endif @@ -30,6 +27,15 @@ enum nf_ct_ext_id { #if IS_ENABLED(CONFIG_NETFILTER_SYNPROXY) NF_CT_EXT_SYNPROXY, #endif + +/* + * The size of the nf_conntrack_ecache struct is about 150 bytes, so + * NF_CT_EXT_ECACHE must be the last one to prevent overflow of + * nf_ct_ext->offset[id] + */ +#ifdef CONFIG_NF_CONNTRACK_EVENTS + NF_CT_EXT_ECACHE, +#endif NF_CT_EXT_NUM, }; @@ -48,7 +54,7 @@ enum nf_ct_ext_id { struct nf_ct_ext { struct rcu_head rcu; u8 offset[NF_CT_EXT_NUM]; - u8 len; + u16 len; char data[0]; }; -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len 2014-03-27 15:32 [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len Andrey Vagin 2014-03-27 15:45 ` Patrick McHardy @ 2014-03-27 15:50 ` Florian Westphal 2014-03-27 16:01 ` Patrick McHardy 1 sibling, 1 reply; 5+ messages in thread From: Florian Westphal @ 2014-03-27 15:50 UTC (permalink / raw) To: Andrey Vagin Cc: linux-kernel, netfilter-devel, netfilter, coreteam, netdev, vvs, Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik, David S. Miller Andrey Vagin <avagin@openvz.org> wrote: > "len" contains sizeof(nf_ct_ext) and size of extensions. In a worst > case it can contain all extensions. Bellow you can find sizes for all > types of extensions. Their sum is definitely bigger than 256. > > nf_ct_ext_types[0]->len = 24 > nf_ct_ext_types[1]->len = 32 > nf_ct_ext_types[2]->len = 24 > nf_ct_ext_types[3]->len = 32 > nf_ct_ext_types[4]->len = 152 Guess its the timer in the ecache extension (with LOCKDEP on probably). I'll submit a patch to get rid of that shortly. I think your patch is fine because the 'no timer in ecache extension' change is quite large, needs review/stress testing etc and is inapropriate for stable tree anyway. > I have seen "len" up to 280 and my host has crashes w/o this patch. ecache is 24 bytes without that timer, should be < 256 in total for all extensions. I think BUILD_BUG_ON test would be nice. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len 2014-03-27 15:50 ` [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len Florian Westphal @ 2014-03-27 16:01 ` Patrick McHardy 0 siblings, 0 replies; 5+ messages in thread From: Patrick McHardy @ 2014-03-27 16:01 UTC (permalink / raw) To: Florian Westphal Cc: Andrey Vagin, linux-kernel, netfilter-devel, netfilter, coreteam, netdev, vvs, Pablo Neira Ayuso, Jozsef Kadlecsik, David S. Miller On Thu, Mar 27, 2014 at 04:50:33PM +0100, Florian Westphal wrote: > Andrey Vagin <avagin@openvz.org> wrote: > > "len" contains sizeof(nf_ct_ext) and size of extensions. In a worst > > case it can contain all extensions. Bellow you can find sizes for all > > types of extensions. Their sum is definitely bigger than 256. > > > > nf_ct_ext_types[0]->len = 24 > > nf_ct_ext_types[1]->len = 32 > > nf_ct_ext_types[2]->len = 24 > > nf_ct_ext_types[3]->len = 32 > > nf_ct_ext_types[4]->len = 152 > > Guess its the timer in the ecache extension (with LOCKDEP on probably). > > I'll submit a patch to get rid of that shortly. > > I think your patch is fine because the 'no timer > in ecache extension' change is quite large, needs review/stress testing > etc and is inapropriate for stable tree anyway. > > > I have seen "len" up to 280 and my host has crashes w/o this patch. > > ecache is 24 bytes without that timer, should be < 256 in total for > all extensions. > > I think BUILD_BUG_ON test would be nice. We also have variable length extensions, but we definitely should catch this *somewhere*. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-03-27 20:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-27 15:32 [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len Andrey Vagin 2014-03-27 15:45 ` Patrick McHardy 2014-03-27 20:06 ` [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len (v2) Andrey Vagin 2014-03-27 15:50 ` [PATCH] netfilter: nf_conntrack: reserve two bytes for nf_ct_ext->len Florian Westphal 2014-03-27 16:01 ` Patrick McHardy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).