On Fri, May 31, 2019 at 12:56:25PM -0400, Eric Garver wrote:
> [..]
> > diff --git a/src/rule.c b/src/rule.c
> > index b00161e0e4350..0048a8e064523 100644
> > --- a/src/rule.c
> > +++ b/src/rule.c
> > @@ -293,6 +293,23 @@ static int cache_add_set_cmd(struct eval_ctx *ectx)
> > return 0;
> > }
> >
> > +static int cache_add_rule_cmd(struct eval_ctx *ectx)
> > +{
> > + struct table *table;
> > + struct chain *chain;
> > +
> > + table = table_lookup(&ectx->cmd->rule->handle, &ectx->nft->cache);
> > + if (!table)
> > + return table_not_found(ectx);
> > +
> > + chain = chain_lookup(table, &ectx->cmd->rule->handle);
> > + if (!chain)
> > + return chain_not_found(ectx);
> > +
> > + rule_cache_update(ectx->cmd->op, chain, ectx->cmd->rule, NULL);
> > + return 0;
> > +}
> > +
> > static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
> > {
> > struct eval_ctx ectx = {
> > @@ -314,6 +331,11 @@ static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
> > continue;
> > ret = cache_add_set_cmd(&ectx);
> > break;
> > + case CMD_OBJ_RULE:
> > + if (!cache_is_complete(&nft->cache, CMD_LIST))
> > + continue;
> > + ret = cache_add_rule_cmd(&ectx);
> > + break;
> > default:
> > break;
> > }
> > @@ -727,6 +749,37 @@ struct rule *rule_lookup_by_index(const struct chain *chain, uint64_t index)
> > return NULL;
> > }
> >
> > +void rule_cache_update(enum cmd_ops op, struct chain *chain,
> > + struct rule *rule, struct rule *ref)
> > +{
> > + switch (op) {
> > + case CMD_INSERT:
> > + rule_get(rule);
> > + if (ref)
> > + list_add_tail(&rule->list, &ref->list);
> > + else
> > + list_add(&rule->list, &chain->rules);
> > + break;
> > + case CMD_ADD:
> > + rule_get(rule);
> > + if (ref)
> > + list_add(&rule->list, &ref->list);
> > + else
> > + list_add_tail(&rule->list, &chain->rules);
> > + break;
> > + case CMD_REPLACE:
> > + rule_get(rule);
> > + list_add(&rule->list, &ref->list);
> > + /* fall through */
> > + case CMD_DELETE:
> > + list_del(&ref->list);
> > + rule_free(ref);
> > + break;
> > + default:
> > + break;
> > + }
> > +}
>
> I'm seeing a NULL pointer dereferenced here. It occurs when we delete a rule
> and add a new rule using the "index" keyword in the same transaction/batch.
I think we need two new things here:
#1 We need a new initial step, before evalution, to calculate the cache
completeness level. This means, we interate over the batch to see what
kind of completeness is needed. Then, cache is fetched only once, at
the beginning of the batch processing. Ensure that cache is
consistent from that step.
#2 Update the cache incrementally: Add new objects from the evaluation
phase. If RESTART is hit, then release the cache, and restart the
evaluation. Probably we don't need to restart the evaluation, just
a function to refresh the batch, ie. check if several objects are
there.
