nftables add vs replace

nftables add vs replace

[Patrick McHardy]

We currently only support "add table" and "add chain" with NLM_F_EXCL.
This means we can't replace entire tables without a lot of extra effort,
also its not possible to create tables/chains just in case they don't
already exist.

To fix this, I'd propose to add two new commands, so we have the following:

- add: add without NLM_F_EXCL
- create: add with NLM_F_EXCL
- replace: replace the entire thing

This most likely will also require updates to the transaction handling
so we don't only process rules, but table, chain and set updates in a
transaction.

Comments?

Re: nftables add vs replace

[Pablo Neira Ayuso]

On Tue, Jan 21, 2014 at 11:06:46AM +0000, Patrick McHardy wrote:
> We currently only support "add table" and "add chain" with NLM_F_EXCL.
> This means we can't replace entire tables without a lot of extra effort,
> also its not possible to create tables/chains just in case they don't
> already exist.
> 
> To fix this, I'd propose to add two new commands, so we have the following:
> 
> - add: add without NLM_F_EXCL
> - create: add with NLM_F_EXCL
> - replace: replace the entire thing

I guess you have in mind to simplify current reloading via nft -f.
Currently, we have to manually flush and delete chain/tables at this
moment, which is a bit of PITA.

> This most likely will also require updates to the transaction handling
> so we don't only process rules, but table, chain and set updates in a
> transaction.
>
> Comments?

It would be indeed nice if we handle table/chain updates in the same
batch like the rules. I think we can find a way to keep the current
.call hook there so we allow both table/chain updates via batch and
via simple independent commands.

Re: nftables add vs replace

[Patrick McHardy]

On Tue, Jan 21, 2014 at 12:27:00PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Jan 21, 2014 at 11:06:46AM +0000, Patrick McHardy wrote:
> > We currently only support "add table" and "add chain" with NLM_F_EXCL.
> > This means we can't replace entire tables without a lot of extra effort,
> > also its not possible to create tables/chains just in case they don't
> > already exist.
> > 
> > To fix this, I'd propose to add two new commands, so we have the following:
> > 
> > - add: add without NLM_F_EXCL
> > - create: add with NLM_F_EXCL
> > - replace: replace the entire thing
> 
> I guess you have in mind to simplify current reloading via nft -f.
> Currently, we have to manually flush and delete chain/tables at this
> moment, which is a bit of PITA.

Correct. It would also make creation of the predefined tables a lot less
error prone.

> > This most likely will also require updates to the transaction handling
> > so we don't only process rules, but table, chain and set updates in a
> > transaction.
> >
> > Comments?
> 
> It would be indeed nice if we handle table/chain updates in the same
> batch like the rules. I think we can find a way to keep the current
> .call hook there so we allow both table/chain updates via batch and
> via simple independent commands.

Yes, that should be possible. We'd also need it for sets. I guess the
semantics would be:

- NLM_F_REPLACE: apply the entire batch to the newly created objects, IOW
  ignore everything that already exists in the table

- !NLM_F_REPLACE: apply the batch to existing or new objects

Re: nftables add vs replace

[Arturo Borrero Gonzalez]

On 21 January 2014 12:27, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jan 21, 2014 at 11:06:46AM +0000, Patrick McHardy wrote:
>> We currently only support "add table" and "add chain" with NLM_F_EXCL.
>> This means we can't replace entire tables without a lot of extra effort,
>> also its not possible to create tables/chains just in case they don't
>> already exist.
>>
>> To fix this, I'd propose to add two new commands, so we have the following:
>>
>> - add: add without NLM_F_EXCL
>> - create: add with NLM_F_EXCL
>> - replace: replace the entire thing
>
> I guess you have in mind to simplify current reloading via nft -f.
> Currently, we have to manually flush and delete chain/tables at this
> moment, which is a bit of PITA.
>

I have some old patches to allow operate over the entire ruleset:
 list ruleset
 flush ruleset
 delete ruleset
 wipe ruleset

I think they are handy for these situations.

Think about a 'ruleset.nft' file starting like this:
==== 8< ====
wipe ruleset
table ip filter {
[...]
}
table ip6 filter {
[...]
}
==== 8< ====

Then, the load via `nft -f' could be straightforward.
Let me know if you want me to reboot them and resend.

regards

-- 
Arturo Borrero González
--
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

Re: nftables add vs replace

[Patrick McHardy]

On Tue, Jan 21, 2014 at 12:37:52PM +0100, Arturo Borrero Gonzalez wrote:
> On 21 January 2014 12:27, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Jan 21, 2014 at 11:06:46AM +0000, Patrick McHardy wrote:
> >> We currently only support "add table" and "add chain" with NLM_F_EXCL.
> >> This means we can't replace entire tables without a lot of extra effort,
> >> also its not possible to create tables/chains just in case they don't
> >> already exist.
> >>
> >> To fix this, I'd propose to add two new commands, so we have the following:
> >>
> >> - add: add without NLM_F_EXCL
> >> - create: add with NLM_F_EXCL
> >> - replace: replace the entire thing
> >
> > I guess you have in mind to simplify current reloading via nft -f.
> > Currently, we have to manually flush and delete chain/tables at this
> > moment, which is a bit of PITA.
> >
> 
> I have some old patches to allow operate over the entire ruleset:
>  list ruleset
>  flush ruleset
>  delete ruleset
>  wipe ruleset
> 
> I think they are handy for these situations.
> 
> Think about a 'ruleset.nft' file starting like this:
> ==== 8< ====
> wipe ruleset
> table ip filter {
> [...]
> }
> table ip6 filter {
> [...]
> }
> ==== 8< ====
> 
> Then, the load via `nft -f' could be straightforward.
> Let me know if you want me to reboot them and resend.

That only helps for the case of loading an entire new ruleset. There are
also many cases where you simply want to create an object iff it doesn't
already exist, but not flush it otherwise. The predefined tables are an
example of this. The second issue is that it would be non-atomic, so
between flush and creating the new ruleset, we'd not have any active rules.

I think the semantics of "flush table" should be changed though. It should
kill *every* object in the table. Perhaps not the base chains, but at least
all rules, non base chain and also sets.

Re: nftables add vs replace

[Tomasz Bursztyka]

Hi Arturo,

> Think about a 'ruleset.nft' file starting like this:
> ==== 8< ====
> wipe ruleset

That hits the current problem: you have to wipe-out all before creating 
stuff: it's greedy,
instead of just letting untouched/replace what exists already and adding 
new ones.

> table ip filter {

Re: nftables add vs replace

[Patrick McHardy]

On Tue, Jan 21, 2014 at 01:46:24PM +0200, Tomasz Bursztyka wrote:
> Hi Arturo,
> 
> >Think about a 'ruleset.nft' file starting like this:
> >==== 8< ====
> >wipe ruleset
> 
> That hits the current problem: you have to wipe-out all before
> creating stuff: it's greedy,
> instead of just letting untouched/replace what exists already and
> adding new ones.

I'm working on my proposed solution. So far just the add vs. create,
the replace command will be a bit more involved.

# src/nft create table filter
<cmdline>:1:1-19: Error: Could not add table: File exists
create table filter
^^^^^^^^^^^^^^^^^^^
# src/nft add table filter
#

Re: nftables add vs replace

[Tomasz Bursztyka]

Hi Patrick,

> I'm working on my proposed solution. So far just the add vs. create,
> the replace command will be a bit more involved.
>
> # src/nft create table filter
> <cmdline>:1:1-19: Error: Could not add table: File exists
> create table filter
> ^^^^^^^^^^^^^^^^^^^
> # src/nft add table filter
> #

Actually, after your patch and Arturo's, it could be possible to improve 
the ruleset management so
it would use create/add/replace accordingly.

Though it means it would need to dump first the targeted tables/chains 
to do so,
thus I am not sure how relevant is my blabbering from performance point 
of view.

Tomasz

Re: nftables add vs replace

[Patrick McHardy]

On Tue, Jan 21, 2014 at 02:08:17PM +0200, Tomasz Bursztyka wrote:
> Hi Patrick,
> 
> >I'm working on my proposed solution. So far just the add vs. create,
> >the replace command will be a bit more involved.
> >
> ># src/nft create table filter
> ><cmdline>:1:1-19: Error: Could not add table: File exists
> >create table filter
> >^^^^^^^^^^^^^^^^^^^
> ># src/nft add table filter
> >#
> 
> Actually, after your patch and Arturo's, it could be possible to
> improve the ruleset management so
> it would use create/add/replace accordingly.
> 
> Though it means it would need to dump first the targeted
> tables/chains to do so,
> thus I am not sure how relevant is my blabbering from performance
> point of view.

How would that work? Dumping rules, flushing the old ones and reinstalling
them is prone to race conditions.

Re: nftables add vs replace

[Tomasz Bursztyka]

>> Actually, after your patch and Arturo's, it could be possible to
>> improve the ruleset management so
>> it would use create/add/replace accordingly.
>>
>> Though it means it would need to dump first the targeted
>> tables/chains to do so,
>> thus I am not sure how relevant is my blabbering from performance
>> point of view.
> How would that work? Dumping rules, flushing the old ones and reinstalling
> them is prone to race conditions.

There would be no flushing involved.
Comparing the dump vs the input ruleset you would know what to 
remove/replace/add.

But maybe there is no benefit from that anyway.

Tomasz

Re: nftables add vs replace

[Patrick McHardy]

On Tue, Jan 21, 2014 at 02:17:31PM +0200, Tomasz Bursztyka wrote:
> 
> >>Actually, after your patch and Arturo's, it could be possible to
> >>improve the ruleset management so
> >>it would use create/add/replace accordingly.
> >>
> >>Though it means it would need to dump first the targeted
> >>tables/chains to do so,
> >>thus I am not sure how relevant is my blabbering from performance
> >>point of view.
> >How would that work? Dumping rules, flushing the old ones and reinstalling
> >them is prone to race conditions.
> 
> There would be no flushing involved.
> Comparing the dump vs the input ruleset you would know what to
> remove/replace/add.
> 
> But maybe there is no benefit from that anyway.

I still I don't see how this helps. Incremental updates already work,
the two problems I see are:

- create something only iff it doesn't exist: easy, not use NLM_F_EXCL
- replace entire tables or chains: harder since the transactions need to
  handle tables, chains and sets which they currently don't. For basechains
  this goes down all the way to nf_register_hooks() since we need to
  atomically replace the hooks.

Re: nftables add vs replace

[Tomasz Bursztyka]

> I still I don't see how this helps. Incremental updates already work,

I messed up on how this replace command could be used, never mind then.

Tomasz

Re: nftables add vs replace

[Arturo Borrero Gonzalez]

On 21 January 2014 13:49, Tomasz Bursztyka
<tomasz.bursztyka@linux.intel.com> wrote:
>
> I messed up on how this replace command could be used, never mind then.
>

Well, I was working on these ops:

% nft export <xml|json>
% nft import <xml|json>

The `export' patch is in shape (i'm sending it right now for you to review).

The problem comes in the `import' operation.
I think that importing the ruleset means that any previous ruleset is
wiped, and I'm working in that way.

In my import code, I flush rules (with a batch), then query sets,
delete each one, query chains, delete each and query tables, delete.
(then add the ruleset)

I surprisingly discovered that when deleting tables, I get EBUSY:

if (!list_empty(&table->chains) || !list_empty(&table->sets))
   return -EBUSY;

because the set list is not empty.
But if I add in my code a sleep(1); between deleting sets and deleting
tables, then all go fine.

Is this the race condition you were referring to, Patrick? I can send
the `import' patch if you want.
-- 
Arturo Borrero González
--
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

Re: nftables add vs replace

[Patrick McHardy]

On Tue, Jan 21, 2014 at 03:05:36PM +0100, Arturo Borrero Gonzalez wrote:
> On 21 January 2014 13:49, Tomasz Bursztyka
> <tomasz.bursztyka@linux.intel.com> wrote:
> >
> > I messed up on how this replace command could be used, never mind then.
> >
> 
> Well, I was working on these ops:
> 
> % nft export <xml|json>
> % nft import <xml|json>
> 
> The `export' patch is in shape (i'm sending it right now for you to review).

Yep, looks pretty good, please see my comments in my reply.

> The problem comes in the `import' operation.
> I think that importing the ruleset means that any previous ruleset is
> wiped, and I'm working in that way.

Yes, that seems reasonable.

> In my import code, I flush rules (with a batch), then query sets,
> delete each one, query chains, delete each and query tables, delete.
> (then add the ruleset)
> 
> I surprisingly discovered that when deleting tables, I get EBUSY:
> 
> if (!list_empty(&table->chains) || !list_empty(&table->sets))
>    return -EBUSY;
> 
> because the set list is not empty.
> But if I add in my code a sleep(1); between deleting sets and deleting
> tables, then all go fine.
> 
> Is this the race condition you were referring to, Patrick? I can send
> the `import' patch if you want.

Hmm that seems very odd, we're not using RCU for freeing sets or something
(even in that case it would be a bug, but at least explainable).

Please send your patch, I'll have a look.

Regarding the race condition, I'm referring to the time between the time
where you start dumping and removing things and the time where the new
ruleset is fully active. We consequetively have less and less of the old
ruleset and then incrementally more and more of the new ruleset until we
have fully switched. Any packets processed during that time will have
unpredictable rules applied to them.

We really need to make this switch atomic to be useful.

Re: nftables add vs replace

[Phil Oester]

On Tue, Jan 21, 2014 at 11:45:25AM +0000, Patrick McHardy wrote:
> I think the semantics of "flush table" should be changed though. It should
> kill *every* object in the table. Perhaps not the base chains, but at least
> all rules, non base chain and also sets.

+1, as current method of having to clear out a table in 2 or 3 steps is
a major PITA.

Phil

Re: nftables add vs replace

[Patrick McHardy]

On Tue, Jan 21, 2014 at 07:15:42AM -0800, Phil Oester wrote:
> On Tue, Jan 21, 2014 at 11:45:25AM +0000, Patrick McHardy wrote:
> > I think the semantics of "flush table" should be changed though. It should
> > kill *every* object in the table. Perhaps not the base chains, but at least
> > all rules, non base chain and also sets.
> 
> +1, as current method of having to clear out a table in 2 or 3 steps is
> a major PITA.

I've added it to my TODO list. I don't insist on doing it myself though :)

Re: nftables add vs replace

[Pablo Neira Ayuso]

On Tue, Jan 21, 2014 at 11:45:25AM +0000, Patrick McHardy wrote:
[...]
> I think the semantics of "flush table" should be changed though. It should
> kill *every* object in the table. Perhaps not the base chains, but at least
> all rules, non base chain and also sets.

I think we need to add a new flush operation with the new semantics
and keep the old one, at least the compat layer needs a flush
operation that leaves all chain objects intact to imitate iptables -F.

Re: nftables add vs replace

[Patrick McHardy]

On Wed, Jan 22, 2014 at 09:38:11AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Jan 21, 2014 at 11:45:25AM +0000, Patrick McHardy wrote:
> [...]
> > I think the semantics of "flush table" should be changed though. It should
> > kill *every* object in the table. Perhaps not the base chains, but at least
> > all rules, non base chain and also sets.
> 
> I think we need to add a new flush operation with the new semantics
> and keep the old one, at least the compat layer needs a flush
> operation that leaves all chain objects intact to imitate iptables -F.

How about:

flush table: flushes everything, removes chains and sets
flush chains: flushes rules within all chains (iptables -F)
flush chain: flushes rules within a chain

Re: nftables add vs replace

[Pablo Neira Ayuso]

On Wed, Jan 22, 2014 at 08:54:40AM +0000, Patrick McHardy wrote:
> On Wed, Jan 22, 2014 at 09:38:11AM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Jan 21, 2014 at 11:45:25AM +0000, Patrick McHardy wrote:
> > [...]
> > > I think the semantics of "flush table" should be changed though. It should
> > > kill *every* object in the table. Perhaps not the base chains, but at least
> > > all rules, non base chain and also sets.
> > 
> > I think we need to add a new flush operation with the new semantics
> > and keep the old one, at least the compat layer needs a flush
> > operation that leaves all chain objects intact to imitate iptables -F.
> 
> How about:
> 
> flush table: flushes everything, removes chains and sets

This is what Arturo has been asking for his new import/export feature.

> flush chains: flushes rules within all chains (iptables -F)
> flush chain: flushes rules within a chain

This last one also allows -F tablename. There's code for these two, so
I think we only need the "massive destruction" flush mode :)

Re: nftables add vs replace

[Patrick McHardy]

On Wed, Jan 22, 2014 at 10:17:25AM +0100, Pablo Neira Ayuso wrote:
> On Wed, Jan 22, 2014 at 08:54:40AM +0000, Patrick McHardy wrote:
> > > 
> > > I think we need to add a new flush operation with the new semantics
> > > and keep the old one, at least the compat layer needs a flush
> > > operation that leaves all chain objects intact to imitate iptables -F.
> > 
> > How about:
> > 
> > flush table: flushes everything, removes chains and sets
> 
> This is what Arturo has been asking for his new import/export feature.

Right, but IMO this is not what it should be used for since the import
should most likely perform an atomic replace. 

> > flush chains: flushes rules within all chains (iptables -F)
> > flush chain: flushes rules within a chain
> 
> This last one also allows -F tablename. There's code for these two, so
> I think we only need the "massive destruction" flush mode :)

Yep. I can look into this, probably next week.