Hi Pablo,
On Sun, May 19, 2019 at 01:51:21PM +0200, Pablo Neira Ayuso wrote:
> Phil Sutter says:
>
> "The problem is that data in h->obj_list potentially sits in cache, too.
> At least rules have to be there so insert with index works correctly. If
> the cache is flushed before regenerating the batch, use-after-free
> occurs which crashes the program."
>
> This patch keeps the old cache around until we have refreshed the batch.
>
> Fixes: 862818ac3a0de ("xtables: add and use nft_build_cache")
> Signed-off-by: Pablo Neira Ayuso <[email protected]>
> ---
> @Phil: I'd like to avoid the refcount infrastructure in libnftnl.
OK, then see below:
> Compile tested-only.
Please test it at least against
iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0. You are
reworking a code-path which is hit only in rare cases, Florian did a
great job in creating something that triggers it.
My point is, I don't trust this code:
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 14141bb7dbcf..51f05b6a6267 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
[...]
> @@ -2902,10 +2918,13 @@ retry:
> errno = 0;
> ret = mnl_batch_talk(h->nl, h->batch, &h->err_list);
> if (ret && errno == ERESTART) {
> - nft_rebuild_cache(h);
> + struct nft_cache *old_cache = nft_rebuild_cache(h);
>
> nft_refresh_transaction(h);
>
> + if (old_cache)
> + flush_cache(old_cache, h->tables, NULL);
> +
The call to flush_cache() should free objects referenced in batch jobs.
Note that nft_refresh_transaction() does not care about batch jobs'
payloads, it just toggles them on/off via 'skip' bit.
The only way to make the above work is by keeping the original cache
copy around until mnl_batch_talk has finally succeeded or failed with
something else than ERESTART.
Cheers, Phil
