diff mbox

[nf_tables] netfilter: nf_tables: invert chain deletion abort path

Message ID 146011296985.3580.3314850969369156279.stgit@nfdev2.cica.es
State Not Applicable
Delegated to: Pablo Neira
Headers show

Commit Message

Arturo Borrero April 8, 2016, 10:56 a.m. UTC 
Before this patch, chain deletetion abort path re-add chains in reverse
order of what was originally in the ruleset.
Invert the order, so the ruleset is exactly the same after abort.

Example, using 2 config files:

ruleset_good.nft:

--
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

Comments

Pablo Neira Ayuso April 14, 2016, 10:24 a.m. UTC | #1 
On Fri, Apr 08, 2016 at 12:56:10PM +0200, Arturo Borrero Gonzalez wrote:
> Before this patch, chain deletetion abort path re-add chains in reverse
> order of what was originally in the ruleset.
> Invert the order, so the ruleset is exactly the same after abort.
> 
> Example, using 2 config files:
> 
> ruleset_good.nft:
> ==== 8< ====
> flush ruleset
> table ip t {
> 	chain c1 {
> 	}
> 	chain c2 {
> 	}
> 	chain c3 {
> 	}
> }
> ==== 8< ====
> 
> ruleset_bad.nft:
> ==== 8< ====
> flush ruleset
> table ip t {
> 	chain c1 {
> 	}
> 	chain c2 {
> 		jump c6
> 	}
> 	chain c3 {
> 	}
> }
> ==== 8< ====
> 
> 
> before this patch:
> 
> % nft -f ruleset_good.nft
> % nft -f ruleset_bad.nft
> % nft list ruleset
> table ip t {
> 	chain c3 {
> 	}
> 
> 	chain c2 {
> 	}
> 
> 	chain c1 {
> 	}
> }
> 
> [ note, inverse order of chain listing ]
> 
> after this patch:
> 
> % nft -f ruleset_good.nft
> % nft -f ruleset_bad.nft
> % nft list ruleset
> table ip t {
> 	chain c1 {
> 	}
> 
> 	chain c2 {
> 	}
> 
> 	chain c3 {
> 	}
> }
> 
> [ note, same order of chain listing ]
> 
> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
>  net/netfilter/nf_tables_api.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 2011977..8578cc6 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -4060,8 +4060,8 @@ static int nf_tables_abort(struct net *net, struct sk_buff *skb)
>  			break;
>  		case NFT_MSG_DELCHAIN:
>  			trans->ctx.table->use++;
> -			list_add_tail_rcu(&trans->ctx.chain->list,
> -					  &trans->ctx.table->chains);
> +			list_add_rcu(&trans->ctx.chain->list,
> +				     &trans->ctx.table->chains);

Thanks for coming up with this Arturo.

I have a better way to fix this by not adding/removing the objects to
the lists.

Ping me back if I don't come up with the fix anytime soon.
--
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
Arturo Borrero April 28, 2016, 8:55 a.m. UTC | #2 
On 14 April 2016 at 12:24, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Thanks for coming up with this Arturo.
>
> I have a better way to fix this by not adding/removing the objects to
> the lists.
>
> Ping me back if I don't come up with the fix anytime soon.

ping :-)
Pablo Neira Ayuso June 22, 2016, 12:45 p.m. UTC | #3 
On Thu, Apr 14, 2016 at 12:24:34PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 08, 2016 at 12:56:10PM +0200, Arturo Borrero Gonzalez wrote:
> > Before this patch, chain deletetion abort path re-add chains in reverse
> > order of what was originally in the ruleset.
> > Invert the order, so the ruleset is exactly the same after abort.
> > 
> > Example, using 2 config files:
> > 
> > ruleset_good.nft:
> > ==== 8< ====
> > flush ruleset
> > table ip t {
> > 	chain c1 {
> > 	}
> > 	chain c2 {
> > 	}
> > 	chain c3 {
> > 	}
> > }
> > ==== 8< ====
> > 
> > ruleset_bad.nft:
> > ==== 8< ====
> > flush ruleset
> > table ip t {
> > 	chain c1 {
> > 	}
> > 	chain c2 {
> > 		jump c6
> > 	}
> > 	chain c3 {
> > 	}
> > }
> > ==== 8< ====
> > 
> > 
> > before this patch:
> > 
> > % nft -f ruleset_good.nft
> > % nft -f ruleset_bad.nft
> > % nft list ruleset
> > table ip t {
> > 	chain c3 {
> > 	}
> > 
> > 	chain c2 {
> > 	}
> > 
> > 	chain c1 {
> > 	}
> > }
> > 
> > [ note, inverse order of chain listing ]
> > 
> > after this patch:
> > 
> > % nft -f ruleset_good.nft
> > % nft -f ruleset_bad.nft
> > % nft list ruleset
> > table ip t {
> > 	chain c1 {
> > 	}
> > 
> > 	chain c2 {
> > 	}
> > 
> > 	chain c3 {
> > 	}
> > }
> > 
> > [ note, same order of chain listing ]
> > 
> > Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> > ---
> >  net/netfilter/nf_tables_api.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 2011977..8578cc6 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -4060,8 +4060,8 @@ static int nf_tables_abort(struct net *net, struct sk_buff *skb)
> >  			break;
> >  		case NFT_MSG_DELCHAIN:
> >  			trans->ctx.table->use++;
> > -			list_add_tail_rcu(&trans->ctx.chain->list,
> > -					  &trans->ctx.table->chains);
> > +			list_add_rcu(&trans->ctx.chain->list,
> > +				     &trans->ctx.table->chains);
> 
> Thanks for coming up with this Arturo.
> 
> I have a better way to fix this by not adding/removing the objects to
> the lists.

Just for the record, this longer patch:

        http://patchwork.ozlabs.org/patch/639103/

fixes this, as it is described here:

        http://patchwork.ozlabs.org/patch/639102/

Thanks for your patience.
--
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
diff mbox

Patch

==== 8< ====
flush ruleset
table ip t {
	chain c1 {
	}
	chain c2 {
	}
	chain c3 {
	}
}
==== 8< ====

ruleset_bad.nft:
==== 8< ====
flush ruleset
table ip t {
	chain c1 {
	}
	chain c2 {
		jump c6
	}
	chain c3 {
	}
}
==== 8< ====


before this patch:

% nft -f ruleset_good.nft
% nft -f ruleset_bad.nft
% nft list ruleset
table ip t {
	chain c3 {
	}

	chain c2 {
	}

	chain c1 {
	}
}

[ note, inverse order of chain listing ]

after this patch:

% nft -f ruleset_good.nft
% nft -f ruleset_bad.nft
% nft list ruleset
table ip t {
	chain c1 {
	}

	chain c2 {
	}

	chain c3 {
	}
}

[ note, same order of chain listing ]

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 net/netfilter/nf_tables_api.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 2011977..8578cc6 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4060,8 +4060,8 @@  static int nf_tables_abort(struct net *net, struct sk_buff *skb)
 			break;
 		case NFT_MSG_DELCHAIN:
 			trans->ctx.table->use++;
-			list_add_tail_rcu(&trans->ctx.chain->list,
-					  &trans->ctx.table->chains);
+			list_add_rcu(&trans->ctx.chain->list,
+				     &trans->ctx.table->chains);
 			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_NEWRULE: