Hi Phil,
On Sat, May 18, 2019 at 01:00:30AM +0200, Phil Sutter wrote:
> This series implements a fix for situations where a cache update removes
> local (still uncommitted) items from cache leading to spurious errors
> afterwards.
>
> The series is based on Eric's "src: update cache if cmd is more
> specific" patch which is still under review but resolves a distinct
> problem from the one addressed in this series.
>
> The first patch improves Eric's patch a bit. If he's OK with my change,
> it may very well be just folded into his.
>
> Phil Sutter (3):
> src: Improve cache_needs_more() algorithm
> libnftables: Keep list of commands in nft context
> src: Restore local entries after cache update
>
> include/nftables.h | 1 +
> src/libnftables.c | 21 +++++------
> src/rule.c | 91 +++++++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 96 insertions(+), 17 deletions(-)
>
> --
> 2.21.0
I've been testing this series. I found anonymous sets are mistakenly
free()d if a cache_release occurs.
I think this occurs because the table objects are added to the cache.
Sets (named and anonymous) hang off the table object. But only named set
objects are added to the cache. As such cache_release() causes a
table_free() and set_free() which frees the anonymous sets because
refcount == 1.
To illustrate this, I tried this HACK and no longer see issues.
diff --git a/src/rule.c b/src/rule.c
index 4f015fc5354b..3604bcbcfa7f 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -350,6 +350,14 @@ static void __cache_flush(struct list_head *table_list)
list_for_each_entry_safe(table, next, table_list, list) {
list_del(&table->list);
+ if (table->refcnt == 1) {
+ struct set *set, *nset;
+ list_for_each_entry_safe(set, nset, &table->sets, list) {
+ if (set->flags & NFT_SET_ANONYMOUS) {
+ set_get(set);
+ }
+ }
+ }
table_free(table);
}
}
--->8---
Below is an example of the double free of the anonymous set.
(gdb) bt
#0 0x00007f894cf92491 in free () at /lib64/libc.so.6
#1 0x00007f893736a73b in expr_destroy (e=0x55be74bf28c0) at expression.c:79
#2 0x00007f893736a73b in expr_free (expr=0x55be74bf28c0) at expression.c:93
#3 0x00007f89373622b3 in set_free (set=0x55be74bc8970) at rule.c:424
#4 0x00007f8937363565 in table_free (table=0x55be74b22220) at rule.c:1232
#5 0x00007f8937363608 in __cache_flush (table_list=table_list@entry=0x55be74abc1e8) at rule.c:353
#6 0x00007f893736368d in cache_release (cache=cache@entry=0x55be74abc1e0) at rule.c:372
#7 0x00007f893736376e in cache_update (nft=0x55be74abc160, cmd=CMD_REPLACE, msgs=<optimized out>) at rule.c:330
#8 0x00007f8937363ce1 in do_command_add (ctx=0x7fff52b44c10, cmd=0x55be74b7fed0, excl=excl@entry=false) at rule.c:1573
#9 0x00007f8937365a27 in do_command (ctx=ctx@entry=0x7fff52b44c10, cmd=cmd@entry=0x55be74b7fed0) at rule.c:2566
#10 0x00007f89373871ad in nft_netlink (nft=nft@entry=0x55be74abc160, cmds=cmds@entry=0x55be74abc220, msgs=msgs@entry=0x7fff52b44c90, nf_sock=<optimized out>) at libnftables.c:45
#11 0x00007f8937387721 in nft_run_cmd_from_buffer (nft=0x55be74abc160, buf=<optimized out>) at libnftables.c:398
[...]
--->8---
It can also manifest as assertion failures in the expression due to
garbage data in the expr.
{"nftables": [
{"metainfo": {"json_schema_version": 1}},
{"add" : {"rule": {"family": "inet", "table": "firewalld", "chain": "filter_IN_public_allow", "expr": [{"match": {"left": {"payload": {"protocol": "dccp", "field": "dport"}}, "op": "==", "right": 222}}, {"match": {"left": {"ct": {"key": "state"}}, "op": "in", "right": {"set": ["new", "untracked"]}}}, {"accept": null}]}}}
]}
BUG: Unknown expression type 44
expression.c:1208: expr_ops: Assertion `0' failed.
