* [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort
@ 2015-12-07 10:48 Xin Long
2015-12-09 14:03 ` Pablo Neira Ayuso
0 siblings, 1 reply; 7+ messages in thread
From: Xin Long @ 2015-12-07 10:48 UTC (permalink / raw)
To: network dev, netfilter-devel; +Cc: davem, fw
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort
2015-12-07 10:48 [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort Xin Long
@ 2015-12-09 14:03 ` Pablo Neira Ayuso
2015-12-09 16:19 ` Xin Long
2015-12-09 16:24 ` Xin Long
0 siblings, 2 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2015-12-09 14:03 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, netfilter-devel, davem, fw
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort
2015-12-09 14:03 ` Pablo Neira Ayuso
@ 2015-12-09 16:19 ` Xin Long
2015-12-09 16:24 ` Xin Long
1 sibling, 0 replies; 7+ messages in thread
From: Xin Long @ 2015-12-09 16:19 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: network dev, netfilter-devel, davem, fw
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort
2015-12-09 14:03 ` Pablo Neira Ayuso
2015-12-09 16:19 ` Xin Long
@ 2015-12-09 16:24 ` Xin Long
2015-12-13 21:47 ` Pablo Neira Ayuso
1 sibling, 1 reply; 7+ messages in thread
From: Xin Long @ 2015-12-09 16:24 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: network dev, netfilter-devel, davem, fw
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort
2015-12-09 16:24 ` Xin Long
@ 2015-12-13 21:47 ` Pablo Neira Ayuso
2016-02-01 10:47 ` Xin Long
0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2015-12-13 21:47 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, netfilter-devel, davem, fw
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort
2015-12-13 21:47 ` Pablo Neira Ayuso
@ 2016-02-01 10:47 ` Xin Long
2016-02-01 11:08 ` Pablo Neira Ayuso
0 siblings, 1 reply; 7+ messages in thread
From: Xin Long @ 2016-02-01 10:47 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: network dev, netfilter-devel, davem, fw
>
> 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 ?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort
2016-02-01 10:47 ` Xin Long
@ 2016-02-01 11:08 ` Pablo Neira Ayuso
0 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2016-02-01 11:08 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, netfilter-devel, davem, fw
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-02-01 11:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-07 10:48 [PATCH net] nftables: use list_for_each_entry_safe_reverse to traversal commit_list in nf_tables_abort Xin Long
2015-12-09 14:03 ` Pablo Neira Ayuso
2015-12-09 16:19 ` Xin Long
2015-12-09 16:24 ` Xin Long
2015-12-13 21:47 ` Pablo Neira Ayuso
2016-02-01 10:47 ` Xin Long
2016-02-01 11:08 ` Pablo Neira Ayuso
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.