Message ID | 1282320404-2948-1-git-send-email-xiaosuo@gmail.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
Le samedi 21 août 2010 à 00:06 +0800, Changli Gao a écrit : > nf_conntrack_alloc() isn't called with nf_conntrack_lock locked, so hash > random initializing code maybe executed more than once on different CPUs. > > Signed-off-by: Changli Gao <xiaosuo@gmail.com> > --- > net/netfilter/nf_conntrack_core.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c > index df3eedb..7ae7f71 100644 > --- a/net/netfilter/nf_conntrack_core.c > +++ b/net/netfilter/nf_conntrack_core.c > @@ -65,8 +65,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_max); > DEFINE_PER_CPU(struct nf_conn, nf_conntrack_untracked); > EXPORT_PER_CPU_SYMBOL(nf_conntrack_untracked); > > -static int nf_conntrack_hash_rnd_initted; > -static unsigned int nf_conntrack_hash_rnd; > +static unsigned int nf_conntrack_hash_rnd __read_mostly; > > static u_int32_t __hash_conntrack(const struct nf_conntrack_tuple *tuple, > u16 zone, unsigned int size, unsigned int rnd) > @@ -574,10 +573,16 @@ struct nf_conn *nf_conntrack_alloc(struct net *net, u16 zone, > { > struct nf_conn *ct; > > - if (unlikely(!nf_conntrack_hash_rnd_initted)) { > - get_random_bytes(&nf_conntrack_hash_rnd, > - sizeof(nf_conntrack_hash_rnd)); > - nf_conntrack_hash_rnd_initted = 1; > + if (unlikely(!nf_conntrack_hash_rnd)) { > + unsigned int rand; > + > + /* Why not initialize nf_conntrack_rnd in a "init()" function ? > + * Because there isn't enough entropy when system initializing, > + * and we initialize it as late as possible. */ This patch is fine but a fine multi line comment is : /* Some * fine * comment */ > + do { > + get_random_bytes(&rand, sizeof(rand)); > + } while (!rand); > + cmpxchg(&nf_conntrack_hash_rnd, 0, rand); > } > > /* We don't want any race condition at early drop stage */ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 2010-08-20 18:17, Eric Dumazet wrote: >> + if (unlikely(!nf_conntrack_hash_rnd)) { >> + unsigned int rand; >> + >> + /* Why not initialize nf_conntrack_rnd in a "init()" function ? >> + * Because there isn't enough entropy when system initializing, >> + * and we initialize it as late as possible. */ > >This patch is fine but a fine multi line comment is : > /* Some > * fine > * comment > */ Actually, /* * Some */ But that's in CodingStyle already. Always a good read, even if reread. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le vendredi 20 août 2010 à 18:20 +0200, Jan Engelhardt a écrit : > On Friday 2010-08-20 18:17, Eric Dumazet wrote: > >> + if (unlikely(!nf_conntrack_hash_rnd)) { > >> + unsigned int rand; > >> + > >> + /* Why not initialize nf_conntrack_rnd in a "init()" function ? > >> + * Because there isn't enough entropy when system initializing, > >> + * and we initialize it as late as possible. */ > > > >This patch is fine but a fine multi line comment is : > > /* Some > > * fine > > * comment > > */ > > Actually, > > /* > * Some > */ > > But that's in CodingStyle already. Always a good read, even if reread. Sorry, I documented David S. Miller choice, or else I would have mentioned CodingStyle :) /* Some * fine * comment */ And it really is better ;) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 2010-08-20 18:26, Eric Dumazet wrote: >> Actually, >> /* >> * Some >> */ >> But that's in CodingStyle already. Always a good read, even if reread. > >Sorry, I documented David S. Miller choice, or else I would have >mentioned CodingStyle :) > > /* Some > * fine > * comment > */ > >And it really is better ;) Looks unsymmetric, which is probably the reason CodingStyle was conceived differently in the first place. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le vendredi 20 août 2010 à 18:34 +0200, Jan Engelhardt a écrit : > On Friday 2010-08-20 18:26, Eric Dumazet wrote: > > >> Actually, > >> /* > >> * Some > >> */ > >> But that's in CodingStyle already. Always a good read, even if reread. > > > >Sorry, I documented David S. Miller choice, or else I would have > >mentioned CodingStyle :) > > > > /* Some > > * fine > > * comment > > */ > > > >And it really is better ;) > > Looks unsymmetric, which is probably the reason CodingStyle was > conceived differently in the first place. Who said comments must be symmetric ? :) http://www.spinics.net/lists/netdev/msg134727.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 2010-08-20 18:35, Eric Dumazet wrote: >>>/* Some >>> * fine >>> * comment >>> */ >>>And it really is better >>Looks unsymmetric >Who said comments must be symmetric ? I can live with sub-level maintainers' free interpretation of areas not covered by CodingStyle, but top-level maintainers directly going against the law of Linux code is outrageous! The system is undeniable :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index df3eedb..7ae7f71 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -65,8 +65,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_max); DEFINE_PER_CPU(struct nf_conn, nf_conntrack_untracked); EXPORT_PER_CPU_SYMBOL(nf_conntrack_untracked); -static int nf_conntrack_hash_rnd_initted; -static unsigned int nf_conntrack_hash_rnd; +static unsigned int nf_conntrack_hash_rnd __read_mostly; static u_int32_t __hash_conntrack(const struct nf_conntrack_tuple *tuple, u16 zone, unsigned int size, unsigned int rnd) @@ -574,10 +573,16 @@ struct nf_conn *nf_conntrack_alloc(struct net *net, u16 zone, { struct nf_conn *ct; - if (unlikely(!nf_conntrack_hash_rnd_initted)) { - get_random_bytes(&nf_conntrack_hash_rnd, - sizeof(nf_conntrack_hash_rnd)); - nf_conntrack_hash_rnd_initted = 1; + if (unlikely(!nf_conntrack_hash_rnd)) { + unsigned int rand; + + /* Why not initialize nf_conntrack_rnd in a "init()" function ? + * Because there isn't enough entropy when system initializing, + * and we initialize it as late as possible. */ + do { + get_random_bytes(&rand, sizeof(rand)); + } while (!rand); + cmpxchg(&nf_conntrack_hash_rnd, 0, rand); } /* We don't want any race condition at early drop stage */
nf_conntrack_alloc() isn't called with nf_conntrack_lock locked, so hash random initializing code maybe executed more than once on different CPUs. Signed-off-by: Changli Gao <xiaosuo@gmail.com> --- net/netfilter/nf_conntrack_core.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html