| Message ID | 20220628164527.101413-1-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf] netfilter: nft_set_pipapo: release elements in clone from abort path | expand |
Hi Pablo, On Tue, 28 Jun 2022 18:45:27 +0200 Pablo Neira Ayuso <pablo@netfilter.org> 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@netfilter.org> > --- > 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).
On Sat, Jul 02, 2022 at 12:39:28AM +0200, Stefano Brivio wrote: [...] > 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). This v2, I forgot to tag it properly: https://patchwork.ozlabs.org/project/netfilter-devel/patch/20220702021631.796822-2-pablo@netfilter.org/ it is updating documentation and it also adds a paragraph to the commit description as you suggested.
On Sat, 2 Jul 2022 04:19:47 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Sat, Jul 02, 2022 at 12:39:28AM +0200, Stefano Brivio wrote: > [...] > > 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). > > This v2, I forgot to tag it properly: > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20220702021631.796822-2-pablo@netfilter.org/ > > it is updating documentation and it also adds a paragraph to the > commit description as you suggested. Thanks! Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
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) +{ + 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
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@netfilter.org> --- 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. priv->dirty seems to be a safe indication that this is in the abort path when calling .destroy(). net/netfilter/nft_set_pipapo.c | 43 ++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 15 deletions(-)