Re: [PATCH] nft: allow deletion of rule by full statement form

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Thu, May 26, 2022 at 07:11:23PM +0530, Chander Govindarajan wrote:
> Currently, rules can only be deleted by handle. You cannot use the
> full rule statement to find and delete rules. This is a documented
> limitation and a pressing one, since people are used to the iptables
> style of deletion.
> 
> Allow deletion of rules by specifying the full rule
> 
> The way this works is as follows:
> 1. Add a new parser rule to match the full rule specification
> 2. Update the cache mechanism to load the full cache when trying to
> delete rule with the new syntax.
> 3. When deleting, check for presence of the rule. If present, loop
> over the existing rules in the chain looking for a match.
> 4. A match is done using the following approach:
>    1. First compare if the number of statments match.
>    2. Check if the type of the statements match between the supplied
>    rule and the present rule.
>    3. Finally, convert the rules into the stateless string
>    output form and compare.
> 5. If we have a match, use the handle id from the match.

I started a patchset to add this that is unfinished. First thing I
made was to update tests/py to see how many rules failed to be deleted
by name.

The problem is that, sometimes, there are subtle differences between
the abstract syntax tree between the linearize and the delinearize
path that need to be normalized.

Another possibility would be to translate the rule into json objects
then compare them. The json format is actually a syntax tree
representation. Downside is that some users compile nftables without
json support.

> Now, there are some small improvements and cleanups needed as
> documented in line. I am sure that there are other conventions that I
> have broken. However, the approach itself seems to work fine.
> 
> PS: This is all a single logical change, not sure if I should have
> used a patchset here.
> 
> PPS: Right now, this expects comments to match if any.
> 
> Signed-off-by: ChanderG <mail@xxxxxxxxxxxxxxxxx>
> ---
>  src/cache.c        | 15 ++++++++--
>  src/mnl.c          | 73 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/parser_bison.y |  4 +++
>  3 files changed, 90 insertions(+), 2 deletions(-)
> 
> diff --git a/src/cache.c b/src/cache.c
> index fd8df884..5a80b324 100644
> --- a/src/cache.c
> +++ b/src/cache.c
> @@ -74,12 +74,23 @@ static unsigned int evaluate_cache_add(struct cmd *cmd,
> unsigned int flags)
>  	return flags;
>  }
> 
> -static unsigned int evaluate_cache_del(struct cmd *cmd, unsigned int flags)
> +static unsigned int evaluate_cache_del(struct cmd *cmd,
> +				       unsigned int flags,
> +				       struct nft_cache_filter *filter)
>  {
>  	switch (cmd->obj) {
>  	case CMD_OBJ_ELEMENTS:
>  		flags |= NFT_CACHE_SETELEM_MAYBE;
>  		break;
> +	case CMD_OBJ_RULE:
> +		// only for delete rule with full rule specified
> +		if (filter && cmd->handle.chain.name && cmd->rule) {
> +			filter->list.family = cmd->handle.family;
> +			filter->list.table = cmd->handle.table.name;
> +			filter->list.chain = cmd->handle.chain.name;
> +			flags |= NFT_CACHE_FULL;
> +		}
> +		break;
>  	default:
>  		break;
>  	}
> @@ -290,7 +301,7 @@ unsigned int nft_cache_evaluate(struct nft_ctx *nft,
> struct list_head *cmds,
>  				 NFT_CACHE_FLOWTABLE |
>  				 NFT_CACHE_OBJECT;
> 
> -			flags = evaluate_cache_del(cmd, flags);
> +			flags = evaluate_cache_del(cmd, flags, filter);
>  			break;
>  		case CMD_GET:
>  			flags = evaluate_cache_get(cmd, flags);
> diff --git a/src/mnl.c b/src/mnl.c
> index 7dd77be1..c611c89f 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -590,6 +590,49 @@ int mnl_nft_rule_replace(struct netlink_ctx *ctx,
> struct cmd *cmd)
>  	return 0;
>  }
> 
> +bool __compare_rules(struct rule *rule1, struct rule *rule2) {
> +	if (rule1->num_stmts != rule2->num_stmts)
> +		return false;
> +
> +	// check for type match for all stmts
> +
> +	struct stmt *stmt1, *stmt2;
> +	stmt1 = &rule1->stmts;
> +	stmt2 = &rule2->stmts;
> +	int count = rule1->num_stmts;
> +
> +	while(count) {
> +		stmt1 = list_entry(stmt1->list.next, typeof(*stmt1), list);
> +		stmt2 = list_entry(stmt2->list.next, typeof(*stmt2), list);
> +
> +		if (stmt1->ops->type != stmt2->ops->type)
> +			return false;
> +
> +		count--;
> +	}
> +
> +	// now check the full string match
> +
> +	// TODO: convert to malloc - but how large?
> +	char buf1[500], buf2[500];
> +	struct output_ctx octx1, octx2;
> +	unsigned int flags = 0;
> +	flags |= NFT_CTX_OUTPUT_STATELESS;
> +
> +	octx1.output_fp = fmemopen(buf1, 500, "w");
> +	octx1.flags = flags;
> +	octx2.output_fp = fmemopen(buf2, 500, "w");
> +	octx2.flags = flags;
> +
> +	rule_print(rule1, &octx1);
> +	rule_print(rule2, &octx2);
> +
> +	if (!strcmp(buf1, buf2))
> +		return true;
> +
> +	return false;
> +}
> +
>  int mnl_nft_rule_del(struct netlink_ctx *ctx, struct cmd *cmd)
>  {
>  	struct handle *h = &cmd->handle;
> @@ -617,6 +660,36 @@ int mnl_nft_rule_del(struct netlink_ctx *ctx, struct
> cmd *cmd)
>  		cmd_add_loc(cmd, nlh->nlmsg_len, &h->handle.location);
>  		mnl_attr_put_u64(nlh, NFTA_RULE_HANDLE, htobe64(h->handle.id));
>  	}
> +	if (cmd->rule) {
> +		// TODO: short-circuit if no stmts in rule
> +		struct table *table;
> +		struct chain *chain;
> +		struct rule *rule;
> +		bool matched = false;
> +
> +		// TODO: anything special to be done for unspecified family?
> +		table = table_cache_find(&ctx->nft->cache.table_cache,
> +					 h->table.name,
> +					 cmd->handle.family);
> +
> +		chain = chain_cache_find(table, h->chain.name);
> +
> +		list_for_each_entry(rule, &chain->rules, list) {
> +			if (__compare_rules(rule, cmd->rule)) {
> +				cmd_add_loc(cmd, nlh->nlmsg_len, &rule->handle.handle.location);
> +				mnl_attr_put_u64(nlh, NFTA_RULE_HANDLE,
> htobe64(rule->handle.handle.id));
> +				matched = true;
> +				break;
> +			}
> +		}
> +
> +		if (!matched) {
> +			errno = ENOENT;
> +			nftnl_rule_free(nlr);
> +			return -1;
> +		}
> +	}
> +	// TODO: handle situation when both are not present, if needed
> 
>  	nftnl_rule_nlmsg_build_payload(nlh, nlr);
>  	nftnl_rule_free(nlr);
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index ca5c488c..f911f7f0 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -1322,6 +1322,10 @@ delete_cmd		:	TABLE		table_or_id_spec
>  			{
>  				$$ = cmd_alloc(CMD_DELETE, CMD_OBJ_RULE, &$2, &@$, NULL);
>  			}
> +			|	RULE		rule_position   rule
> +			{
> +				$$ = cmd_alloc(CMD_DELETE, CMD_OBJ_RULE, &$2, &@$, $3);
> +			}
>  			|	SET		set_or_id_spec
>  			{
>  				$$ = cmd_alloc(CMD_DELETE, CMD_OBJ_SET, &$2, &@$, NULL);
> -- 
> 2.27.0



[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux