Re: [nft RFC PATCH] src: add import operation

[Patrick McHardy <kaber@trash.net>]

On Tue, Jan 21, 2014 at 05:22:09PM +0000, Patrick McHardy wrote:
> On Tue, Jan 21, 2014 at 06:02:11PM +0100, Arturo Borrero Gonzalez wrote:
> > The import operation reads XML/JSON data from stdin.
> > 
> > A basic way to test is:
> >  % nft export json | nft import json
> > 
> > This operation flush the kernel ruleset before adding the one imported.
> > 
> > Adding data from a file:
> >  % cat file.json | nft import json
> >  % nft import json < file.json
> 
> OK I think I understand your -EBUSY problem. We're destroying the rules'
> expressions from within the RCU callback, so its happening asynchronous.
> You most likely don't get EBUSY when deleting the table, but when deleting
> the set because the bindings from the lookup rules are not released yet.
> You then will *also* get -EBUSY from table deletion.
> 
> Basically I see these possibilities to fix this:
> 
> - Use synchronize_rcu() instead of call_rcu() for rule deletion. We
>   added the call_rcu() because deletion performance was suffering
>   badly from lots of synchronize_rcu() calls. This is obviously not
>   a problem anymore with batched deletion.
> 
> - Use synchronize_rcu() before deleting sets. Same effect, but in case
>   someone really does unbatched deletes, it will very likely result in
>   less synchronize_rcu() calls.

Actually this one is nonsense. synchronize_rcu() doesn't guarantee that
a scheduled RCU callback has already returned. I'd suggest to switch back
rule deletion to use synchronize_rcu().

Basically our API is currently broken since it contains a race condition.
Userspace must be able to assume that the referenced objects can be
removed after the DELRULE command has completed.

> Regarding this patch, I don't think we should add this before we support
> atomic replacement of tables, chains and sets as well. IOW, this needs
> quite a bit of kernel work.