nft: I broke interactive mode with iface changes

nft: I broke interactive mode with iface changes

[Phil Sutter]

Hi,

When preparing my patch 94a945ffa81b7 ("libnftables: Get rid of explicit
cache flushes"), I missed that 'nft -i' was explicitly dropping the
interface cache after every command, so now it won't recognize interface
changes anymore. Sadly, there is no lightweight method available to
detect an outdated interface cache like we have with ruleset cache, so I
played around a bit with alternatives.

First thing to mention, using the iface cache is considerably faster
than calling if_nametoindex/if_indextoname each time.

OTOH, I'm not sure having interface indexes in rules is a good idea to
begin with: A seemingly obvious rule like e.g. 'oif br0 accept' all of a
sudden stops working if br0 is recreated (typically by restarting
libvirt or whatever manages it). This becomes obvious when inspecting
the rule set (said rule won't print the interface name anymore but just
the ifindex value), but it's something users have to be aware of first.

So given that oifname/iifname expressions should be recommended over
plain oif/iif ones for general-purpose applications, I wonder how
important that interface cache really is and whether there is a relevant
drawback when dropping it.

Getting back to the original issue, I see the following alternatives:

* Drop interface cache altogether.
* Make nft_run_cmd_from_*() drop the interface cache before parsing
  commands.
* Reintroduce a 'drop_cache()' API function for cli.c to call it.

Obviously, I prefer the first one, but would appreciate others' opinions
before sending patches.

Thanks, Phil

Re: nft: I broke interactive mode with iface changes

[Pablo Neira Ayuso]

Hi Phil,

On Wed, Nov 08, 2017 at 11:15:45AM +0100, Phil Sutter wrote:
> Hi,
> 
> When preparing my patch 94a945ffa81b7 ("libnftables: Get rid of explicit
> cache flushes"), I missed that 'nft -i' was explicitly dropping the
> interface cache after every command, so now it won't recognize interface
> changes anymore. Sadly, there is no lightweight method available to
> detect an outdated interface cache like we have with ruleset cache, so I
> played around a bit with alternatives.
> 
> First thing to mention, using the iface cache is considerably faster
> than calling if_nametoindex/if_indextoname each time.

Indeed, this is saving quite a lot of syscall going back and forth due
to netlink socket create, dump, then release under the curtain of
if_nametoindex calls.

> OTOH, I'm not sure having interface indexes in rules is a good idea to
> begin with: A seemingly obvious rule like e.g. 'oif br0 accept' all of a
> sudden stops working if br0 is recreated (typically by restarting
> libvirt or whatever manages it).

For this usecase, people should use iifname instead. Probably we can
insists on this in wiki.nftables.org, if you have an account, I'd
appreciate if you can document this. Same thing for man nft(8).

> This becomes obvious when inspecting the rule set (said rule won't
> print the interface name anymore but just the ifindex value), but
> it's something users have to be aware of first.
> 
> So given that oifname/iifname expressions should be recommended over
> plain oif/iif ones for general-purpose applications, I wonder how
> important that interface cache really is and whether there is a relevant
> drawback when dropping it.
> 
> Getting back to the original issue, I see the following alternatives:
> 
> * Drop interface cache altogether.
> * Make nft_run_cmd_from_*() drop the interface cache before parsing
>   commands.

This is what we were doing before your change, right? So this would be
fine to me.

> * Reintroduce a 'drop_cache()' API function for cli.c to call it.

Thanks.

[nft PATCH] libnftables: Flush iface cache after command execution

[Phil Sutter]

Commit 94a945ffa81b7 ("libnftables: Get rid of explicit cache flushes")
was a bit too optimistic in that it missed the remaining need to flush
interface cache after each command in interactive mode - otherwise,
newly added interfaces won't be recognized.

Although cli.c only calls nft_run_cmd_from_buffer(), flush caches in
nft_run_cmd_from_filename() as well for matters of consistency.

Fixes: 94a945ffa81b7 ("libnftables: Get rid of explicit cache flushes")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/libnftables.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/libnftables.c b/src/libnftables.c
index 0d04ec21d57d3..dc6a5fdf32640 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -290,6 +290,7 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen)
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
 	nft_ctx_set_output(nft, fp);
 	scanner_destroy(scanner);
+	iface_cache_release();
 
 	return rc;
 }
@@ -322,6 +323,7 @@ err:
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
 	nft_ctx_set_output(nft, fp);
 	scanner_destroy(scanner);
+	iface_cache_release();
 
 	return rc;
 }
-- 
2.13.1

Re: [nft PATCH] libnftables: Flush iface cache after command execution

[Pablo Neira Ayuso]

On Thu, Nov 09, 2017 at 01:24:57PM +0100, Phil Sutter wrote:
> Commit 94a945ffa81b7 ("libnftables: Get rid of explicit cache flushes")
> was a bit too optimistic in that it missed the remaining need to flush
> interface cache after each command in interactive mode - otherwise,
> newly added interfaces won't be recognized.
> 
> Although cli.c only calls nft_run_cmd_from_buffer(), flush caches in
> nft_run_cmd_from_filename() as well for matters of consistency.

Applied, thanks Phil.