[PATCH nft] evaluate: force full cache update on rule index translation

[PATCH nft] evaluate: force full cache update on rule index translation

[Eric Garver]

If we've done a partial fetch of the cache and the genid is the same the
cache update will be skipped without fetching the rules. This causes the
index to handle lookup to fail. To remedy the situation we flush the
cache and force a full update.

Fixes: 816d8c7659c1 ("Support 'add/insert rule index <IDX>'")
Signed-off-by: Eric Garver <eric@garver.life>
---
 src/evaluate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 3593eb80a6a6..a2585291e7c4 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3182,7 +3182,11 @@ static int rule_translate_index(struct eval_ctx *ctx, struct rule *rule)
 	struct rule *r;
 	int ret;
 
-	/* update cache with CMD_LIST so that rules are fetched, too */
+	/* Update cache with CMD_LIST so that rules are fetched, too. The explicit
+	 * release is necessary because the genid may be the same, in which case
+	 * the update would be a no-op.
+	 */
+	cache_release(&ctx->nft->cache);
 	ret = cache_update(ctx->nft, CMD_LIST, ctx->msgs);
 	if (ret < 0)
 		return ret;
-- 
2.20.1

Re: [PATCH nft] evaluate: force full cache update on rule index translation

[Pablo Neira Ayuso]

On Wed, May 01, 2019 at 12:35:10PM -0400, Eric Garver wrote:
> If we've done a partial fetch of the cache and the genid is the same the
> cache update will be skipped without fetching the rules. This causes the
> index to handle lookup to fail. To remedy the situation we flush the
> cache and force a full update.

@Eric: Would you mind to post a reproducer? I'd like to make a test
for tests/shell/ infrastructure to make sure future changes don't
break this.

@Phil: Not related to this, but do you think it would be good to
rework rule index insertion to support for NFTA_RULE_POSITION_ID?

Thanks!

> Fixes: 816d8c7659c1 ("Support 'add/insert rule index <IDX>'")
> Signed-off-by: Eric Garver <eric@garver.life>
> ---
>  src/evaluate.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 3593eb80a6a6..a2585291e7c4 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3182,7 +3182,11 @@ static int rule_translate_index(struct eval_ctx *ctx, struct rule *rule)
>  	struct rule *r;
>  	int ret;
>  
> -	/* update cache with CMD_LIST so that rules are fetched, too */
> +	/* Update cache with CMD_LIST so that rules are fetched, too. The explicit
> +	 * release is necessary because the genid may be the same, in which case
> +	 * the update would be a no-op.
> +	 */
> +	cache_release(&ctx->nft->cache);
>  	ret = cache_update(ctx->nft, CMD_LIST, ctx->msgs);
>  	if (ret < 0)
>  		return ret;
> -- 
> 2.20.1
> 

Re: [PATCH nft] evaluate: force full cache update on rule index translation

[Eric Garver]

On Mon, May 06, 2019 at 12:18:47AM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 01, 2019 at 12:35:10PM -0400, Eric Garver wrote:
> > If we've done a partial fetch of the cache and the genid is the same the
> > cache update will be skipped without fetching the rules. This causes the
> > index to handle lookup to fail. To remedy the situation we flush the
> > cache and force a full update.
> 
> @Eric: Would you mind to post a reproducer? I'd like to make a test
> for tests/shell/ infrastructure to make sure future changes don't
> break this.

I'll post a v2 with test additions.