Hi Pablo,
On Tue, 28 Jun 2022 18:45:27 +0200
Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> New elements that reside in the clone are not released in case that the
> transaction is aborted.
>
> [16302.231754] ------------[ cut here ]------------
> [16302.231756] WARNING: CPU: 0 PID: 100509 at net/netfilter/nf_tables_api.c:1864 nf_tables_chain_destroy+0x26/0x127 [nf_tables]
> [...]
> [16302.231882] CPU: 0 PID: 100509 Comm: nft Tainted: G W 5.19.0-rc3+ #155
> [...]
> [16302.231887] RIP: 0010:nf_tables_chain_destroy+0x26/0x127 [nf_tables]
> [16302.231899] Code: f3 fe ff ff 41 55 41 54 55 53 48 8b 6f 10 48 89 fb 48 c7 c7 82 96 d9 a0 8b 55 50 48 8b 75 58 e8 de f5 92 e0 83 7d 50 00 74 09 <0f> 0b 5b 5d 41 5c 41 5d c3 4c 8b 65 00 48 8b 7d 08 49 39 fc 74 05
> [...]
> [16302.231917] Call Trace:
> [16302.231919] <TASK>
> [16302.231921] __nf_tables_abort.cold+0x23/0x28 [nf_tables]
> [16302.231934] nf_tables_abort+0x30/0x50 [nf_tables]
> [16302.231946] nfnetlink_rcv_batch+0x41a/0x840 [nfnetlink]
> [16302.231952] ? __nla_validate_parse+0x48/0x190
> [16302.231959] nfnetlink_rcv+0x110/0x129 [nfnetlink]
> [16302.231963] netlink_unicast+0x211/0x340
> [16302.231969] netlink_sendmsg+0x21e/0x460
>
> Add nft_set_pipapo_match_destroy() helper function to release the
> elements in the lookup tables.
>
> Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> Hi Stefano,
>
> I triggered this splat with this test.nft file:
>
> table inet test {
> chain x {
> }
>
> chain y {
> udp length . @th,160,128 vmap { 47-63 . 0xe373135363130333131303735353203 : goto x }
> }
> }
>
> then, exercise the abort path with -c:
>
> # nft -c -f test.nft
>
> I don't see the splat anymore here.
>
> This bug uncovered with the -o/--optimize infrastructure, which has a
> test similar to the file described above.
Thanks for catching this.
> priv->dirty seems to be a safe indication that this is in the abort path
> when calling .destroy().
I'm not sure about that, it looks quite difficult to me to prove.
However, is it relevant? The point here is that if priv->dirty is set,
we might (should) have at least one different element in priv->clone
compared to priv->match, so we should go ahead and destroy elements
also found there. The issue you discovered might even happen on
non-abort paths I guess.
> net/netfilter/nft_set_pipapo.c | 43 ++++++++++++++++++++++------------
> 1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> index 2c8051d8cca6..02f6cc061a2e 100644
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -2124,6 +2124,27 @@ static int nft_pipapo_init(const struct nft_set *set,
> return err;
> }
>
> +static void nft_set_pipapo_match_destroy(const struct nft_set *set,
> + struct nft_pipapo_match *m)
For consistency (and perhaps clarity), I would also add a kernel-doc
comment for this one:
/**
* nft_set_pipapo_match_destroy() - Destroy elements from key mapping array
* @set: nftables API set representation
* @m: matching data pointing to key mapping array
*/
> +{
> + struct nft_pipapo_field *f;
> + int i, r;
> +
> + for (i = 0, f = m->f; i < m->field_count - 1; i++, f++)
> + ;
> +
> + for (r = 0; r < f->rules; r++) {
> + struct nft_pipapo_elem *e;
> +
> + if (r < f->rules - 1 && f->mt[r + 1].e == f->mt[r].e)
> + continue;
> +
> + e = f->mt[r].e;
> +
> + nft_set_elem_destroy(set, e, true);
> + }
> +}
> +
> /**
> * nft_pipapo_destroy() - Free private data for set and all committed elements
> * @set: nftables API set representation
> @@ -2132,26 +2153,13 @@ static void nft_pipapo_destroy(const struct nft_set *set)
> {
> struct nft_pipapo *priv = nft_set_priv(set);
> struct nft_pipapo_match *m;
> - struct nft_pipapo_field *f;
> - int i, r, cpu;
> + int cpu;
>
> m = rcu_dereference_protected(priv->match, true);
> if (m) {
> rcu_barrier();
>
> - for (i = 0, f = m->f; i < m->field_count - 1; i++, f++)
> - ;
> -
> - for (r = 0; r < f->rules; r++) {
> - struct nft_pipapo_elem *e;
> -
> - if (r < f->rules - 1 && f->mt[r + 1].e == f->mt[r].e)
> - continue;
> -
> - e = f->mt[r].e;
> -
> - nft_set_elem_destroy(set, e, true);
> - }
> + nft_set_pipapo_match_destroy(set, m);
>
> #ifdef NFT_PIPAPO_ALIGN
> free_percpu(m->scratch_aligned);
> @@ -2165,6 +2173,11 @@ static void nft_pipapo_destroy(const struct nft_set *set)
> }
>
> if (priv->clone) {
> + m = priv->clone;
> +
> + if (priv->dirty)
> + nft_set_pipapo_match_destroy(set, m);
> +
> #ifdef NFT_PIPAPO_ALIGN
> free_percpu(priv->clone->scratch_aligned);
> #endif
Other than that, it looks good to me.
I would also specify in the commit message that we additionally look
for elements pointers in the cloned matching data if priv->dirty is
set, because that means that cloned data might point to additional
elements we didn't commit to the working copy yet (such as the abort
path case, but perhaps not limited to it).
--
Stefano
