[PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort

[PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort

[Xin Long]

when we use 'nft -f' to sumbit rules, it will build multiple rules into
one netlink skb to send to kernel, kernel will process them one by one.
meanwhile, it add the trans into commit_list to record every commit.
if one of them's return value is -EAGAIN, status |= NFNL_BATCH_REPLAY
will be marked. after all the process is done. it will roll back all the
commits.

now kernel use list_add_tail to add trans to commit, and use
list_for_each_entry_safe to roll back. which means the order of adding
and rollback is the same. that will cause some cases cannot work well,
even trigger call trace, like:

1. add a set into table foo  [return -EAGAIN]:
   commit_list = 'add set trans'
2. del foo:
   commit_list = 'add set trans' -> 'del set trans' -> 'del tab trans'
then nf_tables_abort will be called to roll back:
firstly process 'add set trans':
                   case NFT_MSG_NEWSET:
                        trans->ctx.table->use--;
                        list_del_rcu(&nft_trans_set(trans)->list);

  it will del the set from the table foo, but it has removed when del
  table foo [step 2], then the kernel will panic.

the right order of rollback should be:
  'del tab trans' -> 'del set trans' -> 'add set trans'.
which is opposite with commit_list order.

so fix it by rolling back commits with reverse order in nf_tables_abort.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/netfilter/nf_tables_api.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 93cc473..4511a78 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4030,7 +4030,8 @@ static int nf_tables_abort(struct sk_buff *skb)
 	struct nft_trans *trans, *next;
 	struct nft_trans_elem *te;
 
-	list_for_each_entry_safe(trans, next, &net->nft.commit_list, list) {
+	list_for_each_entry_safe_reverse(trans, next, &net->nft.commit_list,
+					 list) {
 		switch (trans->msg_type) {
 		case NFT_MSG_NEWTABLE:
 			if (nft_trans_table_update(trans)) {
-- 
2.1.0

Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort

[Pablo Neira Ayuso]

On Mon, Dec 07, 2015 at 06:48:07PM +0800, Xin Long wrote:
> when we use 'nft -f' to sumbit rules, it will build multiple rules into
> one netlink skb to send to kernel, kernel will process them one by one.
> meanwhile, it add the trans into commit_list to record every commit.
> if one of them's return value is -EAGAIN, status |= NFNL_BATCH_REPLAY
> will be marked. after all the process is done. it will roll back all the
> commits.
> 
> now kernel use list_add_tail to add trans to commit, and use
> list_for_each_entry_safe to roll back. which means the order of adding
> and rollback is the same. that will cause some cases cannot work well,
> even trigger call trace, like:
> 
> 1. add a set into table foo  [return -EAGAIN]:
>    commit_list = 'add set trans'
> 2. del foo:
>    commit_list = 'add set trans' -> 'del set trans' -> 'del tab trans'
> then nf_tables_abort will be called to roll back:
> firstly process 'add set trans':
>                    case NFT_MSG_NEWSET:
>                         trans->ctx.table->use--;
>                         list_del_rcu(&nft_trans_set(trans)->list);
> 
>   it will del the set from the table foo, but it has removed when del
>   table foo [step 2], then the kernel will panic.
> 
> the right order of rollback should be:
>   'del tab trans' -> 'del set trans' -> 'add set trans'.
> which is opposite with commit_list order.
> 
> so fix it by rolling back commits with reverse order in nf_tables_abort.


You're reporting a kernel panic.

Could you please provide a sequence of commands to reproduce it with
the existing code?

Thanks.

Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort

[Xin Long]

ok, the reproducer:
1.
#nft delete table foo
#nft add table foo
#nft list tables
#nft list table foo
#nft add chain foo bar
#nft add chain foo baz
#nft add chain foo bok
#nft list table foo

2. #nft -f panic.rules
------panic.rules-------
add rule foo bar ip saddr 127.0.0.1 accept
add rule foo bar ip saddr {192.168.1.2, 192.168.2.3} jump baz
add rule foo bar ip saddr {192.168.1.2, 192.168.2.3} jump bok

add rule foo baz ip saddr {192.168.1.2, 192.168.2.3} jump bok
add rule foo bok ip saddr {192.168.1.2, 192.168.2.3} jump baz

delete table foo
-------end-----------
the panic will happen 1/1

Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort

[Xin Long]

On Wed, Dec 9, 2015 at 10:03 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Dec 07, 2015 at 06:48:07PM +0800, Xin Long wrote:
>> when we use 'nft -f' to sumbit rules, it will build multiple rules into
>> one netlink skb to send to kernel, kernel will process them one by one.
>> meanwhile, it add the trans into commit_list to record every commit.
>> if one of them's return value is -EAGAIN, status |= NFNL_BATCH_REPLAY
>> will be marked. after all the process is done. it will roll back all the
>> commits.
>>
>> now kernel use list_add_tail to add trans to commit, and use
>> list_for_each_entry_safe to roll back. which means the order of adding
>> and rollback is the same. that will cause some cases cannot work well,
>> even trigger call trace, like:
>>
>> 1. add a set into table foo  [return -EAGAIN]:
>>    commit_list = 'add set trans'
>> 2. del foo:
>>    commit_list = 'add set trans' -> 'del set trans' -> 'del tab trans'
>> then nf_tables_abort will be called to roll back:
>> firstly process 'add set trans':
>>                    case NFT_MSG_NEWSET:
>>                         trans->ctx.table->use--;
>>                         list_del_rcu(&nft_trans_set(trans)->list);
>>
>>   it will del the set from the table foo, but it has removed when del
>>   table foo [step 2], then the kernel will panic.
>>
>> the right order of rollback should be:
>>   'del tab trans' -> 'del set trans' -> 'add set trans'.
>> which is opposite with commit_list order.
>>
>> so fix it by rolling back commits with reverse order in nf_tables_abort.
>
>
> You're reporting a kernel panic.
>
> Could you please provide a sequence of commands to reproduce it with
> the existing code?
>
> Thanks.

the reproduce is a kind of long, do i need to repost this patch with
the reproduce?

Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort

[Pablo Neira Ayuso]

On Thu, Dec 10, 2015 at 12:24:21AM +0800, Xin Long wrote:
> On Wed, Dec 9, 2015 at 10:03 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Dec 07, 2015 at 06:48:07PM +0800, Xin Long wrote:
[...]
> >> the right order of rollback should be:
> >>   'del tab trans' -> 'del set trans' -> 'add set trans'.
> >> which is opposite with commit_list order.
> >>
> >> so fix it by rolling back commits with reverse order in nf_tables_abort.
> >
> >
> > You're reporting a kernel panic.
> >
> > Could you please provide a sequence of commands to reproduce it with
> > the existing code?
> >
> > Thanks.
> 
> the reproduce is a kind of long, do i need to repost this patch with
> the reproduce?

No need to resend.

Yes, we need this reverse iteration there to handle the 'delete table'
command in the batch. This problem happens since we have
nft_flush_table().

Other callsites are artificially restriction deletion of inactive
objects but that should be removed as we already discuss on the
mailing list.

So I'm applying this, thanks.

Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort

[Xin Long]

>
> No need to resend.
>
> Yes, we need this reverse iteration there to handle the 'delete table'
> command in the batch. This problem happens since we have
> nft_flush_table().
>
> Other callsites are artificially restriction deletion of inactive
> objects but that should be removed as we already discuss on the
> mailing list.
>
> So I'm applying this, thanks.

Hi Pablo, has this one been applied ?

Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort

[Pablo Neira Ayuso]

On Mon, Feb 01, 2016 at 06:47:33PM +0800, Xin Long wrote:
> >
> > No need to resend.
> >
> > Yes, we need this reverse iteration there to handle the 'delete table'
> > command in the batch. This problem happens since we have
> > nft_flush_table().
> >
> > Other callsites are artificially restriction deletion of inactive
> > objects but that should be removed as we already discuss on the
> > mailing list.
> >
> > So I'm applying this, thanks.
> 
> Hi Pablo, has this one been applied ?

commit a907e36d54e0ff836e55e04531be201bf6b4d8c8
Author: Xin Long <lucien.xin@gmail.com>
Date:   Mon Dec 7 18:48:07 2015 +0800

    netfilter: nf_tables: use reverse traversal commit_list in nf_tables_abort