On Tue, Aug 05, 2014 at 06:07:11PM +0200, Eric Dumazet wrote:
> On Tue, 2014-08-05 at 17:30 +0200, Pablo Neira Ayuso wrote:
> > Fix possible replacemen of the per-cpu chain counters by null
> > pointer when updating an existing chain in the commit path.
> >
> > Reported-by: Matteo Croce <technoboy85@xxxxxxxxx>
> > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> > ---
> > net/netfilter/nf_tables_api.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index f95dc95..f7dce2b 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -899,6 +899,9 @@ static struct nft_stats __percpu *nft_stats_alloc(const struct nlattr *attr)
> > static void nft_chain_stats_replace(struct nft_base_chain *chain,
> > struct nft_stats __percpu *newstats)
> > {
> > + if (newstats == NULL)
> > + return;
> > +
> > if (chain->stats) {
> > struct nft_stats __percpu *oldstats =
> > nft_dereference(chain->stats);
>
> This looks strange. Real bug is that nft_dump_stats() should not try to
> fold percpu stats and output something if 'stats' is NULL ?
>
> Otherwise, why nft_chain_stats_replace() checks if previous pointer is
> NULL ?
Currently, we always allocate the per-cpu chain stats. The problem
occurs here:
static int nf_tables_newchain(struct sock *nlsk, ...
...
if (chain != NULL) {
struct nft_stats *stats = NULL;
...
if (nla[NFTA_CHAIN_COUNTERS]) {
...
}
...
nft_trans_chain_stats(trans) = stats;
Then, in the nf_tables_commit() path, this calls
nft_chain_commit_update() which sets the per-chain stats to NULL,
which is bad.
The idea is that, if no chain stats are passed when updating the
chain, this means: "please, leave the chain counter as is in this
update".
This bug slipped through with the transaction infrastructure.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
