| Message ID | 871th7frlg.fsf@x220.int.ebiederm.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jun 19, 2015 at 02:03:39PM -0500, Eric W. Biederman wrote: > > Add code to nf_unregister_hook to flush the nf_queue when a hook is > unregistered. This guarantees that the pointer that the nf_queue code > retains into the nf_hook list will remain valid while a packet is > queued. I think the real problem is that struct nf_queue_entry holds a pointer to struct nf_hook_ops, which will be gone after removal. So you uncovered a long standing problem that will amplify by when pernet hooks are in place. Regarding the pointer to nf_hook_list, now that new netdevice variant doesn't support nf_queue yet, so that nf_hook_list will be always valid since it will point to the global nf_hooks in the core. > I tested what would happen if we do not flush queued packets and was > trivially able to obtain the oops below. All that was required was > to stop the nf_queue listening process, to delete all of the nf_tables, > and to awaken the nf_queue listening process. [...] Please, route netfilter patches through the netfilter trees, ie. nf and nf-next. > Cc: stable@vger.kernel.org I guess this is a leftover since there is no Cc to stable. Anyway, we have to wait until this hits master before we ask for -stable inclusion. More comments below. Thanks for this fix BTW. > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > > Apologies for the duplicate send but I forgot to include the appropriate > mailing lists. > > include/net/netfilter/nf_queue.h | 2 ++ > net/netfilter/core.c | 1 + > net/netfilter/nf_internals.h | 1 + > net/netfilter/nf_queue.c | 17 +++++++++++++++++ > net/netfilter/nfnetlink_queue_core.c | 24 +++++++++++++++++++++++- > 5 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h > index d81d584157e1..e8635854a55b 100644 > --- a/include/net/netfilter/nf_queue.h > +++ b/include/net/netfilter/nf_queue.h > @@ -24,6 +24,8 @@ struct nf_queue_entry { > struct nf_queue_handler { > int (*outfn)(struct nf_queue_entry *entry, > unsigned int queuenum); > + void (*nf_hook_drop)(struct net *net, > + struct nf_hook_ops *ops); > }; > > void nf_register_queue_handler(const struct nf_queue_handler *qh); > diff --git a/net/netfilter/core.c b/net/netfilter/core.c > index 653e32eac08c..a0e54974e2c9 100644 > --- a/net/netfilter/core.c > +++ b/net/netfilter/core.c > @@ -118,6 +118,7 @@ void nf_unregister_hook(struct nf_hook_ops *reg) > static_key_slow_dec(&nf_hooks_needed[reg->pf][reg->hooknum]); > #endif > synchronize_net(); > + nf_queue_nf_hook_drop(reg); > } > EXPORT_SYMBOL(nf_unregister_hook); > > diff --git a/net/netfilter/nf_internals.h b/net/netfilter/nf_internals.h > index ea7f36784b3d..399210693c2a 100644 > --- a/net/netfilter/nf_internals.h > +++ b/net/netfilter/nf_internals.h > @@ -19,6 +19,7 @@ unsigned int nf_iterate(struct list_head *head, struct sk_buff *skb, > /* nf_queue.c */ > int nf_queue(struct sk_buff *skb, struct nf_hook_ops *elem, > struct nf_hook_state *state, unsigned int queuenum); > +void nf_queue_nf_hook_drop(struct nf_hook_ops *ops); > int __init netfilter_queue_init(void); > > /* nf_log.c */ > diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c > index 2e88032cd5ad..cd60d397fe05 100644 > --- a/net/netfilter/nf_queue.c > +++ b/net/netfilter/nf_queue.c > @@ -105,6 +105,23 @@ bool nf_queue_entry_get_refs(struct nf_queue_entry *entry) > } > EXPORT_SYMBOL_GPL(nf_queue_entry_get_refs); > > +void nf_queue_nf_hook_drop(struct nf_hook_ops *ops) I'd suggest you rename all these 'nf_hook_drop' to 'flush'. > +{ > + const struct nf_queue_handler *qh; > + struct net *net; > + > + rtnl_lock(); Why rtnl_lock() here? > + rcu_read_lock(); > + qh = rcu_dereference(queue_handler); > + if (qh) { > + for_each_net(net) { > + qh->nf_hook_drop(net, ops); > + } > + } > + rcu_read_unlock(); > + rtnl_unlock(); > +} > + > /* > * Any packet that leaves via this function must come back > * through nf_reinject(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in
On 20.06, Pablo Neira Ayuso wrote: > On Fri, Jun 19, 2015 at 02:03:39PM -0500, Eric W. Biederman wrote: > > > > Add code to nf_unregister_hook to flush the nf_queue when a hook is > > unregistered. This guarantees that the pointer that the nf_queue code > > retains into the nf_hook list will remain valid while a packet is > > queued. > > I think the real problem is that struct nf_queue_entry holds a pointer > to struct nf_hook_ops, which will be gone after removal. So you > uncovered a long standing problem that will amplify by when pernet > hooks are in place. > > Regarding the pointer to nf_hook_list, now that new netdevice variant > doesn't support nf_queue yet, so that nf_hook_list will be always > valid since it will point to the global nf_hooks in the core. I think Eric's patch is the right thing to do. I'm not sure I get your netdev comment, but we certainly do want to drop packets once a hook is gone. > > +{ > > + const struct nf_queue_handler *qh; > > + struct net *net; > > + > > + rtnl_lock(); > > Why rtnl_lock() here? for_each_net(). Would actually be nice to have a variant that doesn't need the rtnl since it makes locking order analysis a lot harder. > > + rcu_read_lock(); > > + qh = rcu_dereference(queue_handler); > > + if (qh) { > > + for_each_net(net) { > > + qh->nf_hook_drop(net, ops); > > + } > > + } > > + rcu_read_unlock(); > > + rtnl_unlock(); -- To unsubscribe from this list: send the line "unsubscribe netdev" in
Pablo Neira Ayuso <pablo@netfilter.org> writes: > On Fri, Jun 19, 2015 at 02:03:39PM -0500, Eric W. Biederman wrote: >> >> Add code to nf_unregister_hook to flush the nf_queue when a hook is >> unregistered. This guarantees that the pointer that the nf_queue code >> retains into the nf_hook list will remain valid while a packet is >> queued. > > I think the real problem is that struct nf_queue_entry holds a pointer > to struct nf_hook_ops, which will be gone after removal. Yes that is what I meant, when I was talking about the pointer that the nf_queue code holds into the nf_hook list. That list is threaded through nf_hook_ops, and is used to retain the place in the nf_hook list for when the packet returns through nf_reinject. > So you > uncovered a long standing problem that will amplify by when pernet > hooks are in place. Yes. This will apply to more than just nftables when the pernet hooks are in place. The try_module_get prevents this for everything except for nftables today. So in practice this problem has existed since the merge of nftables. The try_module_get shows this problem has existed in some form longer than git. > Regarding the pointer to nf_hook_list, now that new netdevice variant > doesn't support nf_queue yet, so that nf_hook_list will be always > valid since it will point to the global nf_hooks in the core. > >> I tested what would happen if we do not flush queued packets and was >> trivially able to obtain the oops below. All that was required was >> to stop the nf_queue listening process, to delete all of the nf_tables, >> and to awaken the nf_queue listening process. > [...] > > Please, route netfilter patches through the netfilter trees, ie. nf > and nf-next. Whatever works. I just see this as a bug in the networking stack that needs to be fixed. I don't care who I send it to as long as Linus gets it. >> Cc: stable@vger.kernel.org > > I guess this is a leftover since there is no Cc to stable. Anyway, > we have to wait until this hits master before we ask for -stable > inclusion. This is a marker that this should be backported to stable, and the typicall way this is remembered outside of the network trees. The stable folks grep the git log for Cc: stable... > More comments below. Thanks for this fix BTW. > >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- >> >> Apologies for the duplicate send but I forgot to include the appropriate >> mailing lists. >> >> include/net/netfilter/nf_queue.h | 2 ++ >> net/netfilter/core.c | 1 + >> net/netfilter/nf_internals.h | 1 + >> net/netfilter/nf_queue.c | 17 +++++++++++++++++ >> net/netfilter/nfnetlink_queue_core.c | 24 +++++++++++++++++++++++- >> 5 files changed, 44 insertions(+), 1 deletion(-) >> >> diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h >> index d81d584157e1..e8635854a55b 100644 >> --- a/include/net/netfilter/nf_queue.h >> +++ b/include/net/netfilter/nf_queue.h >> @@ -24,6 +24,8 @@ struct nf_queue_entry { >> struct nf_queue_handler { >> int (*outfn)(struct nf_queue_entry *entry, >> unsigned int queuenum); >> + void (*nf_hook_drop)(struct net *net, >> + struct nf_hook_ops *ops); >> }; >> >> void nf_register_queue_handler(const struct nf_queue_handler *qh); >> diff --git a/net/netfilter/core.c b/net/netfilter/core.c >> index 653e32eac08c..a0e54974e2c9 100644 >> --- a/net/netfilter/core.c >> +++ b/net/netfilter/core.c >> @@ -118,6 +118,7 @@ void nf_unregister_hook(struct nf_hook_ops *reg) >> static_key_slow_dec(&nf_hooks_needed[reg->pf][reg->hooknum]); >> #endif >> synchronize_net(); >> + nf_queue_nf_hook_drop(reg); >> } >> EXPORT_SYMBOL(nf_unregister_hook); >> >> diff --git a/net/netfilter/nf_internals.h b/net/netfilter/nf_internals.h >> index ea7f36784b3d..399210693c2a 100644 >> --- a/net/netfilter/nf_internals.h >> +++ b/net/netfilter/nf_internals.h >> @@ -19,6 +19,7 @@ unsigned int nf_iterate(struct list_head *head, struct sk_buff *skb, >> /* nf_queue.c */ >> int nf_queue(struct sk_buff *skb, struct nf_hook_ops *elem, >> struct nf_hook_state *state, unsigned int queuenum); >> +void nf_queue_nf_hook_drop(struct nf_hook_ops *ops); >> int __init netfilter_queue_init(void); >> >> /* nf_log.c */ >> diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c >> index 2e88032cd5ad..cd60d397fe05 100644 >> --- a/net/netfilter/nf_queue.c >> +++ b/net/netfilter/nf_queue.c >> @@ -105,6 +105,23 @@ bool nf_queue_entry_get_refs(struct nf_queue_entry *entry) >> } >> EXPORT_SYMBOL_GPL(nf_queue_entry_get_refs); >> >> +void nf_queue_nf_hook_drop(struct nf_hook_ops *ops) > > I'd suggest you rename all these 'nf_hook_drop' to 'flush'. The functions in nfnetfilter_queue_core.c are also named drop, and I am not in a mood to change the convention. >> +{ >> + const struct nf_queue_handler *qh; >> + struct net *net; >> + >> + rtnl_lock(); > > Why rtnl_lock() here? Because we need a race free way to visit all of the network namespaces. I would perform the for_each_net on the other side of nf_hook_drop but the rcu locking would not allow that. >> + rcu_read_lock(); >> + qh = rcu_dereference(queue_handler); >> + if (qh) { >> + for_each_net(net) { >> + qh->nf_hook_drop(net, ops); >> + } >> + } >> + rcu_read_unlock(); >> + rtnl_unlock(); >> +} >> + >> /* >> * Any packet that leaves via this function must come back >> * through nf_reinject(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in
Patrick McHardy <kaber@trash.net> writes: > On 20.06, Pablo Neira Ayuso wrote: >> On Fri, Jun 19, 2015 at 02:03:39PM -0500, Eric W. Biederman wrote: >> > >> > Add code to nf_unregister_hook to flush the nf_queue when a hook is >> > unregistered. This guarantees that the pointer that the nf_queue code >> > retains into the nf_hook list will remain valid while a packet is >> > queued. >> >> I think the real problem is that struct nf_queue_entry holds a pointer >> to struct nf_hook_ops, which will be gone after removal. So you >> uncovered a long standing problem that will amplify by when pernet >> hooks are in place. >> >> Regarding the pointer to nf_hook_list, now that new netdevice variant >> doesn't support nf_queue yet, so that nf_hook_list will be always >> valid since it will point to the global nf_hooks in the core. > > I think Eric's patch is the right thing to do. I'm not sure I get > your netdev comment, but we certainly do want to drop packets once > a hook is gone. > >> > +{ >> > + const struct nf_queue_handler *qh; >> > + struct net *net; >> > + >> > + rtnl_lock(); >> >> Why rtnl_lock() here? > > for_each_net(). Would actually be nice to have a variant that doesn't > need the rtnl since it makes locking order analysis a lot harder. Someone added a for_each_net_rcu. But right now I am not at all certain I trust an rcu variant not to miss something, in a weird corner case. When missing something translates to an unprivileged user triggerable kernel oops I am not ready to play games. As for the lock analysis. Except for nf_tables nf_unregister_hook is called by module removal routines where rtnl_lock() is safe. With nftables we seem to do everything under some version of the nfnl_lock. Does the nfnl_lock have any problems with taking the rtnl_lock to nest underneath it? I tested this path and I did not have any practical problems, but I don't think I had lockdep enabled at the time. Eric >> > + rcu_read_lock(); >> > + qh = rcu_dereference(queue_handler); >> > + if (qh) { >> > + for_each_net(net) { >> > + qh->nf_hook_drop(net, ops); >> > + } >> > + } >> > + rcu_read_unlock(); >> > + rtnl_unlock(); -- To unsubscribe from this list: send the line "unsubscribe netdev" in
On 20.06, Eric W. Biederman wrote: > Patrick McHardy <kaber@trash.net> writes: > > > On 20.06, Pablo Neira Ayuso wrote: > >> On Fri, Jun 19, 2015 at 02:03:39PM -0500, Eric W. Biederman wrote: > >> > > >> > Add code to nf_unregister_hook to flush the nf_queue when a hook is > >> > unregistered. This guarantees that the pointer that the nf_queue code > >> > retains into the nf_hook list will remain valid while a packet is > >> > queued. > >> > >> I think the real problem is that struct nf_queue_entry holds a pointer > >> to struct nf_hook_ops, which will be gone after removal. So you > >> uncovered a long standing problem that will amplify by when pernet > >> hooks are in place. > >> > >> Regarding the pointer to nf_hook_list, now that new netdevice variant > >> doesn't support nf_queue yet, so that nf_hook_list will be always > >> valid since it will point to the global nf_hooks in the core. > > > > I think Eric's patch is the right thing to do. I'm not sure I get > > your netdev comment, but we certainly do want to drop packets once > > a hook is gone. > > > >> > +{ > >> > + const struct nf_queue_handler *qh; > >> > + struct net *net; > >> > + > >> > + rtnl_lock(); > >> > >> Why rtnl_lock() here? > > > > for_each_net(). Would actually be nice to have a variant that doesn't > > need the rtnl since it makes locking order analysis a lot harder. > > Someone added a for_each_net_rcu. But right now I am not at all certain > I trust an rcu variant not to miss something, in a weird corner case. > When missing something translates to an unprivileged user triggerable > kernel oops I am not ready to play games. > > As for the lock analysis. Except for nf_tables nf_unregister_hook is > called by module removal routines where rtnl_lock() is safe. > > With nftables we seem to do everything under some version of the > nfnl_lock. Does the nfnl_lock have any problems with taking the > rtnl_lock to nest underneath it? No, its fine, we have almost none interactions except for network namespaces and device lookups. Main reason why I'd prefer a non-RTNL version is because your callbacks introduce bigger chunks of code under the RTNL, so it might complicate things in the future. But your reasoning is sound and for now this is perfectly fine. -- To unsubscribe from this list: send the line "unsubscribe netdev" in
On Sat, Jun 20, 2015 at 01:32:48PM +0200, Patrick McHardy wrote: > On 20.06, Pablo Neira Ayuso wrote: > > On Fri, Jun 19, 2015 at 02:03:39PM -0500, Eric W. Biederman wrote: > > > > > > Add code to nf_unregister_hook to flush the nf_queue when a hook is > > > unregistered. This guarantees that the pointer that the nf_queue code > > > retains into the nf_hook list will remain valid while a packet is > > > queued. > > > > I think the real problem is that struct nf_queue_entry holds a pointer > > to struct nf_hook_ops, which will be gone after removal. So you > > uncovered a long standing problem that will amplify by when pernet > > hooks are in place. > > > > Regarding the pointer to nf_hook_list, now that new netdevice variant > > doesn't support nf_queue yet, so that nf_hook_list will be always > > valid since it will point to the global nf_hooks in the core. > > I think Eric's patch is the right thing to do. I'm not sure I get > your netdev comment, but we certainly do want to drop packets once > a hook is gone. I agree this patch is fine, of course. > > > +{ > > > + const struct nf_queue_handler *qh; > > > + struct net *net; > > > + > > > + rtnl_lock(); > > > > Why rtnl_lock() here? > > for_each_net(). Would actually be nice to have a variant that doesn't > need the rtnl since it makes locking order analysis a lot harder. OK, thanks for explaining. -- To unsubscribe from this list: send the line "unsubscribe netdev" in
diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h index d81d584157e1..e8635854a55b 100644 --- a/include/net/netfilter/nf_queue.h +++ b/include/net/netfilter/nf_queue.h @@ -24,6 +24,8 @@ struct nf_queue_entry { struct nf_queue_handler { int (*outfn)(struct nf_queue_entry *entry, unsigned int queuenum); + void (*nf_hook_drop)(struct net *net, + struct nf_hook_ops *ops); }; void nf_register_queue_handler(const struct nf_queue_handler *qh); diff --git a/net/netfilter/core.c b/net/netfilter/core.c index 653e32eac08c..a0e54974e2c9 100644 --- a/net/netfilter/core.c +++ b/net/netfilter/core.c @@ -118,6 +118,7 @@ void nf_unregister_hook(struct nf_hook_ops *reg) static_key_slow_dec(&nf_hooks_needed[reg->pf][reg->hooknum]); #endif synchronize_net(); + nf_queue_nf_hook_drop(reg); } EXPORT_SYMBOL(nf_unregister_hook); diff --git a/net/netfilter/nf_internals.h b/net/netfilter/nf_internals.h index ea7f36784b3d..399210693c2a 100644 --- a/net/netfilter/nf_internals.h +++ b/net/netfilter/nf_internals.h @@ -19,6 +19,7 @@ unsigned int nf_iterate(struct list_head *head, struct sk_buff *skb, /* nf_queue.c */ int nf_queue(struct sk_buff *skb, struct nf_hook_ops *elem, struct nf_hook_state *state, unsigned int queuenum); +void nf_queue_nf_hook_drop(struct nf_hook_ops *ops); int __init netfilter_queue_init(void); /* nf_log.c */ diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c index 2e88032cd5ad..cd60d397fe05 100644 --- a/net/netfilter/nf_queue.c +++ b/net/netfilter/nf_queue.c @@ -105,6 +105,23 @@ bool nf_queue_entry_get_refs(struct nf_queue_entry *entry) } EXPORT_SYMBOL_GPL(nf_queue_entry_get_refs); +void nf_queue_nf_hook_drop(struct nf_hook_ops *ops) +{ + const struct nf_queue_handler *qh; + struct net *net; + + rtnl_lock(); + rcu_read_lock(); + qh = rcu_dereference(queue_handler); + if (qh) { + for_each_net(net) { + qh->nf_hook_drop(net, ops); + } + } + rcu_read_unlock(); + rtnl_unlock(); +} + /* * Any packet that leaves via this function must come back * through nf_reinject(). diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c index 22a5ac76683e..d0479e12d977 100644 --- a/net/netfilter/nfnetlink_queue_core.c +++ b/net/netfilter/nfnetlink_queue_core.c @@ -824,6 +824,27 @@ static struct notifier_block nfqnl_dev_notifier = { .notifier_call = nfqnl_rcv_dev_event, }; +static int nf_hook_cmp(struct nf_queue_entry *entry, unsigned long ops_ptr) +{ + return entry->elem == (struct nf_hook_ops *)ops_ptr; +} + +static void nfqnl_nf_hook_drop(struct net *net, struct nf_hook_ops *hook) +{ + struct nfnl_queue_net *q = nfnl_queue_pernet(net); + int i; + + rcu_read_lock(); + for (i = 0; i < INSTANCE_BUCKETS; i++) { + struct nfqnl_instance *inst; + struct hlist_head *head = &q->instance_table[i]; + + hlist_for_each_entry_rcu(inst, head, hlist) + nfqnl_flush(inst, nf_hook_cmp, (unsigned long)hook); + } + rcu_read_unlock(); +} + static int nfqnl_rcv_nl_event(struct notifier_block *this, unsigned long event, void *ptr) @@ -1031,7 +1052,8 @@ static const struct nla_policy nfqa_cfg_policy[NFQA_CFG_MAX+1] = { }; static const struct nf_queue_handler nfqh = { - .outfn = &nfqnl_enqueue_packet, + .outfn = &nfqnl_enqueue_packet, + .nf_hook_drop = &nfqnl_nf_hook_drop, }; static int