Re: [PATCH 1/3] netfilter: nf_tables: fix transaction race condition

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Mar 03, 2015 at 08:04:18PM +0000, Patrick McHardy wrote:
> A race condition exists in the rule transaction code for rules that
> get added and removed within the same transaction.
> 
> The new rule starts out as inactive in the current and active in the
> next generation and is inserted into the ruleset. When it is deleted,
> it is additionally set to inactive in the next generation as well.
> 
> On commit the next generation is begun, then the actions are finalized.
> For the new rule this would mean clearing out the inactive bit for
> the previously current, now next generation.
> 
> However nft_rule_clear() clears out the bits for *both* generations,
> activating the rule in the current generation, where it should be
> deactivated due to being deleted. The rule will thus be active until
> the deletion is finalized, removing the rule from the ruleset.
> 
> Similarly, when aborting a transaction for the same case, the undo
> of insertion will remove it from the RCU protected rule list, the
> deletion will clear out all bits. However until the next RCU
> synchronization after all operations have been undone, the rule is
> active on CPUs which can still see the rule on the list.
> 
> Generally, there may never be any modifications of the current
> generations' inactive bit since this defeats the entire purpose of
> atomicity. Change nft_rule_clear() to only touch the next generations
> bit to fix this.

I think we can get rid of the nft_rule_clear() call from the error
path of nf_tables_newrule() too.

> Signed-off-by: Patrick McHardy <kaber@trash.net>
> ---
>  net/netfilter/nf_tables_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index a8c9462..6fb532b 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -227,7 +227,7 @@ nft_rule_deactivate_next(struct net *net, struct nft_rule *rule)
>  
>  static inline void nft_rule_clear(struct net *net, struct nft_rule *rule)
>  {
> -	rule->genmask = 0;
> +	rule->genmask &= ~(1 << gencursor_next(net));
>  }
>  
>  static int
> -- 
> 2.1.0
>