Re: [PATCH nft] src: add support to flush sets

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Dec 06, 2016 at 12:13:37PM +0100, Anatole Denis wrote:
> On lun., déc. 5, 2016 at 11:37 , Pablo Neira Ayuso <pablo@netfilter.org>
> wrote:
> >You can use this new command to remove all existing elements in a set:
> >
> > # nft flush set filter xyz
> >
> >After this command, the set 'xyz' in table 'filter' becomes empty.
> >
> >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> >---
> > include/netlink.h | 2 ++
> > src/evaluate.c    | 3 +++
> > src/netlink.c     | 9 ++++++++-
> > src/rule.c        | 3 +++
> > 4 files changed, 16 insertions(+), 1 deletion(-)
> >
> >diff --git a/include/netlink.h b/include/netlink.h
> >index 28c11f603ed2..363b5251968f 100644
> >--- a/include/netlink.h
> >+++ b/include/netlink.h
> >@@ -165,6 +165,8 @@ extern int netlink_delete_setelems(struct netlink_ctx
> >*ctx, const struct handle
> > 				   const struct expr *expr);
> > extern int netlink_get_setelems(struct netlink_ctx *ctx, const struct
> >handle *h,
> > 				const struct location *loc, struct set *set);
> >+extern int netlink_flush_setelems(struct netlink_ctx *ctx, const struct
> >handle *h,
> >+				  const struct location *loc);
> >
> > extern void netlink_dump_table(const struct nftnl_table *nlt);
> > extern void netlink_dump_chain(const struct nftnl_chain *nlc);
> >diff --git a/src/evaluate.c b/src/evaluate.c
> >index e11a455a5f53..8a3da54e5b2d 100644
> >--- a/src/evaluate.c
> >+++ b/src/evaluate.c
> >@@ -2857,9 +2857,11 @@ static int cmd_evaluate_list(struct eval_ctx *ctx,
> >struct cmd *cmd)
> > static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
> > {
> > 	int ret;
> >+
> > 	ret = cache_update(cmd->op, ctx->msgs);
> > 	if (ret < 0)
> > 		return ret;
> >+
> > 	switch (cmd->obj) {
> > 	case CMD_OBJ_RULESET:
> > 		cache_flush();
> >@@ -2870,6 +2872,7 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx,
> >struct cmd *cmd)
> > 		 */
> > 	case CMD_OBJ_CHAIN:
> > 		/* Chains don't hold sets */
> >+	case CMD_OBJ_SET:
> > 		break;
> 
> I think you need to empty the cache for said set here, otherwise this will
> lead to the same errors we had earlier with flush ruleset not emptying the
> set cache.

Thanks for spotting this, I'd appreciate if you send me a follow up
patch that I can apply to fix this.

> Example test which fails:
> Running
> ```
> table t{
>  set s { type ipv4_addr; flags interval
>    elements={127.0.0.1/8}
>  }
> }
> ```
> then:
> ```
> flush set t s
> add element t s {
>  127.0.0.1/8,
> }
> ```
> errors out with:
> Error: interval overlaps with an existing one
>  127.0.0.1/8,
>  ^^^^^^^^^^^
> 
> [...]
> 
> Maybe a question for another patch but: if there is support for emptying
> specific sets, maybe flushing a table should empty sets in the table as well
> as the chains ? (currently it only empties the chains and leaves the sets
> intact, which is kind of unintuitive)

That's a good point.

If we follow this approach, main problem is that set flushing is going
to be supported by recent kernels, while older don't. So if nft flush
table also flush sets, then we'll have people experiencing different
behaviours, which is not good.

So I would prefer an explicit 'nft flush sets' to empty elements from
all sets on top of this new 'nft flush set x y', and we document that
table flushing only empties rules.