On Sat, Jan 25, 2014 at 02:03:51PM +0100, Pablo Neira Ayuso wrote:
> As a side effect, we save memory as we don't need rcu_head per rule
> anymore.
We can also save some memory for now unnecessary families in the private
structs since we have the context available during destruction again.
> @@ -1809,9 +1803,6 @@ static int nf_tables_commit(struct sk_buff *skb)
> synchronize_rcu();
>
> list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) {
> - /* Delete this rule from the dirty list */
> - list_del(&rupd->list);
> -
> /* This rule was inactive in the past and just became active.
> * Clear the next bit of the genmask since its meaning has
> * changed, now it is the future.
> @@ -1822,6 +1813,7 @@ static int nf_tables_commit(struct sk_buff *skb)
> rupd->chain, rupd->rule,
> NFT_MSG_NEWRULE, 0,
> rupd->family);
> + list_del(&rupd->list);
> kfree(rupd);
> continue;
> }
> @@ -1831,7 +1823,15 @@ static int nf_tables_commit(struct sk_buff *skb)
> nf_tables_rule_notify(skb, rupd->nlh, rupd->table, rupd->chain,
> rupd->rule, NFT_MSG_DELRULE, 0,
> rupd->family);
> + }
> +
> + /* Make sure we don't see any packet traversing old rules */
> + synchronize_rcu();
> +
> + /* Now we can safely release unused old rules */
> + list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) {
> nf_tables_rule_destroy(rupd->rule);
> + list_del(&rupd->list);
> kfree(rupd);
> }
>
> @@ -1844,20 +1844,26 @@ static int nf_tables_abort(struct sk_buff *skb)
> struct nft_rule_trans *rupd, *tmp;
>
> list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) {
> - /* Delete all rules from the dirty list */
> - list_del(&rupd->list);
> -
> if (!nft_rule_is_active_next(net, rupd->rule)) {
> nft_rule_clear(net, rupd->rule);
> + list_del(&rupd->list);
> kfree(rupd);
> continue;
> }
>
> /* This rule is inactive, get rid of it */
> list_del_rcu(&rupd->rule->list);
> + }
> +
> + /* Make sure we don't see any packet accessing aborted rules */
> + synchronize_rcu();
> +
> + list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) {
> nf_tables_rule_destroy(rupd->rule);
> + list_del(&rupd->list);
> kfree(rupd);
> }
I have to admit this all seems slightly confusing to me, we now have three
synhronize_rcu()s in this function, are all those really needed?
--
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
