| Message ID | 20190429165614.1506-1-ap420073@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | netfilter: nf_flow_table: fix several flowtable bugs | expand |
On Tue, Apr 30, 2019 at 01:56:14AM +0900, Taehee Yoo wrote: > flow offload of CT can be deleted by the masquerade module. then, > flow offload should be deleted too. but GC and data-path of flow offload > do not check CT's status. hence they will be removed only by the timeout. > > GC and data-path routine will check ct->status. > If IPS_DYING_BIT is set, GC will delete CT and data-path routine > do not use it. > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > > v1 -> v2 : use IPS_DYING_BIT instead of ct->ct_general.use refcnt. > > net/netfilter/nf_flow_table_core.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c > index 7aabfd4b1e50..50d04a718b41 100644 > --- a/net/netfilter/nf_flow_table_core.c > +++ b/net/netfilter/nf_flow_table_core.c > @@ -232,6 +232,7 @@ flow_offload_lookup(struct nf_flowtable *flow_table, > { > struct flow_offload_tuple_rhash *tuplehash; > struct flow_offload *flow; > + struct flow_offload_entry *e; > int dir; > > tuplehash = rhashtable_lookup(&flow_table->rhashtable, tuple, > @@ -244,6 +245,10 @@ flow_offload_lookup(struct nf_flowtable *flow_table, > if (flow->flags & (FLOW_OFFLOAD_DYING | FLOW_OFFLOAD_TEARDOWN)) > return NULL; > > + e = container_of(flow, struct flow_offload_entry, flow); > + if (unlikely(test_bit(IPS_DYING_BIT, &e->ct->status))) Please, send a new version of this patch that uses: nf_ct_is_dying() Thanks. > + return NULL; > + > return tuplehash; > } > EXPORT_SYMBOL_GPL(flow_offload_lookup); > @@ -290,9 +295,12 @@ static inline bool nf_flow_has_expired(const struct flow_offload *flow) > static void nf_flow_offload_gc_step(struct flow_offload *flow, void *data) > { > struct nf_flowtable *flow_table = data; > + struct flow_offload_entry *e; > > + e = container_of(flow, struct flow_offload_entry, flow); > if (nf_flow_has_expired(flow) || > - (flow->flags & (FLOW_OFFLOAD_DYING | FLOW_OFFLOAD_TEARDOWN))) > + (flow->flags & (FLOW_OFFLOAD_DYING | FLOW_OFFLOAD_TEARDOWN)) || > + (test_bit(IPS_DYING_BIT, &e->ct->status))) > flow_offload_del(flow_table, flow); > } > > -- > 2.17.1 >
On Tue, 30 Apr 2019 at 20:59, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Tue, Apr 30, 2019 at 01:56:14AM +0900, Taehee Yoo wrote: > > flow offload of CT can be deleted by the masquerade module. then, > > flow offload should be deleted too. but GC and data-path of flow offload > > do not check CT's status. hence they will be removed only by the timeout. > > > > GC and data-path routine will check ct->status. > > If IPS_DYING_BIT is set, GC will delete CT and data-path routine > > do not use it. > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > v1 -> v2 : use IPS_DYING_BIT instead of ct->ct_general.use refcnt. > > > > net/netfilter/nf_flow_table_core.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c > > index 7aabfd4b1e50..50d04a718b41 100644 > > --- a/net/netfilter/nf_flow_table_core.c > > +++ b/net/netfilter/nf_flow_table_core.c > > @@ -232,6 +232,7 @@ flow_offload_lookup(struct nf_flowtable *flow_table, > > { > > struct flow_offload_tuple_rhash *tuplehash; > > struct flow_offload *flow; > > + struct flow_offload_entry *e; > > int dir; > > > > tuplehash = rhashtable_lookup(&flow_table->rhashtable, tuple, > > @@ -244,6 +245,10 @@ flow_offload_lookup(struct nf_flowtable *flow_table, > > if (flow->flags & (FLOW_OFFLOAD_DYING | FLOW_OFFLOAD_TEARDOWN)) > > return NULL; > > > > + e = container_of(flow, struct flow_offload_entry, flow); > > + if (unlikely(test_bit(IPS_DYING_BIT, &e->ct->status))) > > Please, send a new version of this patch that uses: > > nf_ct_is_dying() > > Thanks. > Thank you for the review and I will send v3 patch for using nf_ct_is_dying() Thank you again! > > + return NULL; > > + > > return tuplehash; > > } > > EXPORT_SYMBOL_GPL(flow_offload_lookup); > > @@ -290,9 +295,12 @@ static inline bool nf_flow_has_expired(const struct flow_offload *flow) > > static void nf_flow_offload_gc_step(struct flow_offload *flow, void *data) > > { > > struct nf_flowtable *flow_table = data; > > + struct flow_offload_entry *e; > > > > + e = container_of(flow, struct flow_offload_entry, flow); > > if (nf_flow_has_expired(flow) || > > - (flow->flags & (FLOW_OFFLOAD_DYING | FLOW_OFFLOAD_TEARDOWN))) > > + (flow->flags & (FLOW_OFFLOAD_DYING | FLOW_OFFLOAD_TEARDOWN)) || > > + (test_bit(IPS_DYING_BIT, &e->ct->status))) > > flow_offload_del(flow_table, flow); > > } > > > > -- > > 2.17.1 > >
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c index 7aabfd4b1e50..50d04a718b41 100644 --- a/net/netfilter/nf_flow_table_core.c +++ b/net/netfilter/nf_flow_table_core.c @@ -232,6 +232,7 @@ flow_offload_lookup(struct nf_flowtable *flow_table, { struct flow_offload_tuple_rhash *tuplehash; struct flow_offload *flow; + struct flow_offload_entry *e; int dir; tuplehash = rhashtable_lookup(&flow_table->rhashtable, tuple, @@ -244,6 +245,10 @@ flow_offload_lookup(struct nf_flowtable *flow_table, if (flow->flags & (FLOW_OFFLOAD_DYING | FLOW_OFFLOAD_TEARDOWN)) return NULL; + e = container_of(flow, struct flow_offload_entry, flow); + if (unlikely(test_bit(IPS_DYING_BIT, &e->ct->status))) + return NULL; + return tuplehash; } EXPORT_SYMBOL_GPL(flow_offload_lookup); @@ -290,9 +295,12 @@ static inline bool nf_flow_has_expired(const struct flow_offload *flow) static void nf_flow_offload_gc_step(struct flow_offload *flow, void *data) { struct nf_flowtable *flow_table = data; + struct flow_offload_entry *e; + e = container_of(flow, struct flow_offload_entry, flow); if (nf_flow_has_expired(flow) || - (flow->flags & (FLOW_OFFLOAD_DYING | FLOW_OFFLOAD_TEARDOWN))) + (flow->flags & (FLOW_OFFLOAD_DYING | FLOW_OFFLOAD_TEARDOWN)) || + (test_bit(IPS_DYING_BIT, &e->ct->status))) flow_offload_del(flow_table, flow); }
flow offload of CT can be deleted by the masquerade module. then, flow offload should be deleted too. but GC and data-path of flow offload do not check CT's status. hence they will be removed only by the timeout. GC and data-path routine will check ct->status. If IPS_DYING_BIT is set, GC will delete CT and data-path routine do not use it. Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- v1 -> v2 : use IPS_DYING_BIT instead of ct->ct_general.use refcnt. net/netfilter/nf_flow_table_core.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)