Re: [PATCH] nf_queue: Make the queue_handler pernet

2016-05-30 Thread Pablo Neira Ayuso
On Fri, May 13, 2016 at 09:18:52PM -0500, Eric W. Biederman wrote:
> 
> Florian Weber reported:
> > Under full load (unshare() in loop -> OOM conditions) we can
> > get kernel panic:
> >
> > BUG: unable to handle kernel NULL pointer dereference at 0008
> > IP: [] nfqnl_nf_hook_drop+0x35/0x70
> > [..]
> > task: 88012dfa3840 ti: 88012dffc000 task.ti: 88012dffc000
> > RIP: 0010:[]  [] 
> > nfqnl_nf_hook_drop+0x35/0x70
> > RSP: :88012dfffd80  EFLAGS: 00010206
> > RAX: 0008 RBX: 81add0c0 RCX: 88013fd8
> > [..]
> > Call Trace:
> >  [] nf_queue_nf_hook_drop+0x18/0x20
> >  [] nf_unregister_net_hook+0xdb/0x150
> >  [] netfilter_net_exit+0x2f/0x60
> >  [] ops_exit_list.isra.4+0x38/0x60
> >  [] setup_net+0xc2/0x120
> >  [] copy_net_ns+0x79/0x120
> >  [] create_new_namespaces+0x11b/0x1e0
> >  [] unshare_nsproxy_namespaces+0x57/0xa0
> >  [] SyS_unshare+0x1b2/0x340
> >  [] entry_SYSCALL_64_fastpath+0x1e/0xa8
> > Code: 65 00 48 89 e5 41 56 41 55 41 54 53 83 e8 01 48 8b 97 70 12 00 00 48 
> > 98 49 89 f4 4c 8b 74 c2 18 4d 8d 6e 08 49 81 c6 88 00 00 00 <49> 8b 5d 00 
> > 48 85 db 74 1a 48 89 df 4c 89 e2 48 c7 c6 90 68 47
> >
> 
> The simple fix for this requires a new pernet variable for struct
> nf_queue that indicates when it is safe to use the dynamically
> allocated nf_queue state.
> 
> As we need a variable anyway make nf_register_queue_handler and
> nf_unregister_queue_handler pernet.  This allows the existing logic of
> when it is safe to use the state from the nfnetlink_queue module to be
> reused with no changes except for making it per net.
> 
> The syncrhonize_rcu from nf_unregister_queue_handler is moved to a new
> function nfnl_queue_net_exit_batch so that the worst case of having a
> syncrhonize_rcu in the pernet exit path is not experienced in batch
> mode.

Applied, thanks.


[PATCH] nf_queue: Make the queue_handler pernet

2016-05-13 Thread Eric W. Biederman

Florian Weber reported:
> Under full load (unshare() in loop -> OOM conditions) we can
> get kernel panic:
>
> BUG: unable to handle kernel NULL pointer dereference at 0008
> IP: [] nfqnl_nf_hook_drop+0x35/0x70
> [..]
> task: 88012dfa3840 ti: 88012dffc000 task.ti: 88012dffc000
> RIP: 0010:[]  [] 
> nfqnl_nf_hook_drop+0x35/0x70
> RSP: :88012dfffd80  EFLAGS: 00010206
> RAX: 0008 RBX: 81add0c0 RCX: 88013fd8
> [..]
> Call Trace:
>  [] nf_queue_nf_hook_drop+0x18/0x20
>  [] nf_unregister_net_hook+0xdb/0x150
>  [] netfilter_net_exit+0x2f/0x60
>  [] ops_exit_list.isra.4+0x38/0x60
>  [] setup_net+0xc2/0x120
>  [] copy_net_ns+0x79/0x120
>  [] create_new_namespaces+0x11b/0x1e0
>  [] unshare_nsproxy_namespaces+0x57/0xa0
>  [] SyS_unshare+0x1b2/0x340
>  [] entry_SYSCALL_64_fastpath+0x1e/0xa8
> Code: 65 00 48 89 e5 41 56 41 55 41 54 53 83 e8 01 48 8b 97 70 12 00 00 48 98 
> 49 89 f4 4c 8b 74 c2 18 4d 8d 6e 08 49 81 c6 88 00 00 00 <49> 8b 5d 00 48 85 
> db 74 1a 48 89 df 4c 89 e2 48 c7 c6 90 68 47
>

The simple fix for this requires a new pernet variable for struct
nf_queue that indicates when it is safe to use the dynamically
allocated nf_queue state.

As we need a variable anyway make nf_register_queue_handler and
nf_unregister_queue_handler pernet.  This allows the existing logic of
when it is safe to use the state from the nfnetlink_queue module to be
reused with no changes except for making it per net.

The syncrhonize_rcu from nf_unregister_queue_handler is moved to a new
function nfnl_queue_net_exit_batch so that the worst case of having a
syncrhonize_rcu in the pernet exit path is not experienced in batch
mode.

Reported-by: Florian Westphal 
Acked-by: Florian Westphal 
Signed-off-by: "Eric W. Biederman" 
---
 include/net/netfilter/nf_queue.h |  4 ++--
 include/net/netns/netfilter.h|  2 ++
 net/netfilter/nf_queue.c | 17 -
 net/netfilter/nfnetlink_queue.c  | 18 --
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
index 9c5638ad872e..0dbce55437f2 100644
--- a/include/net/netfilter/nf_queue.h
+++ b/include/net/netfilter/nf_queue.h
@@ -28,8 +28,8 @@ struct nf_queue_handler {
struct nf_hook_ops *ops);
 };
 
-void nf_register_queue_handler(const struct nf_queue_handler *qh);
-void nf_unregister_queue_handler(void);
+void nf_register_queue_handler(struct net *net, const struct nf_queue_handler 
*qh);
+void nf_unregister_queue_handler(struct net *net);
 void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict);
 
 void nf_queue_entry_get_refs(struct nf_queue_entry *entry);
diff --git a/include/net/netns/netfilter.h b/include/net/netns/netfilter.h
index 38aa4983e2a9..36d723579af2 100644
--- a/include/net/netns/netfilter.h
+++ b/include/net/netns/netfilter.h
@@ -5,11 +5,13 @@
 
 struct proc_dir_entry;
 struct nf_logger;
+struct nf_queue_handler;
 
 struct netns_nf {
 #if defined CONFIG_PROC_FS
struct proc_dir_entry *proc_netfilter;
 #endif
+   const struct nf_queue_handler __rcu *queue_handler;
const struct nf_logger __rcu *nf_loggers[NFPROTO_NUMPROTO];
 #ifdef CONFIG_SYSCTL
struct ctl_table_header *nf_log_dir_header;
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 5baa8e24e6ac..b19ad20a705c 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -26,23 +26,21 @@
  * Once the queue is registered it must reinject all packets it
  * receives, no matter what.
  */
-static const struct nf_queue_handler __rcu *queue_handler __read_mostly;
 
 /* return EBUSY when somebody else is registered, return EEXIST if the
  * same handler is registered, return 0 in case of success. */
-void nf_register_queue_handler(const struct nf_queue_handler *qh)
+void nf_register_queue_handler(struct net *net, const struct nf_queue_handler 
*qh)
 {
/* should never happen, we only have one queueing backend in kernel */
-   WARN_ON(rcu_access_pointer(queue_handler));
-   rcu_assign_pointer(queue_handler, qh);
+   WARN_ON(rcu_access_pointer(net->nf.queue_handler));
+   rcu_assign_pointer(net->nf.queue_handler, qh);
 }
 EXPORT_SYMBOL(nf_register_queue_handler);
 
 /* The caller must flush their queue before this */
-void nf_unregister_queue_handler(void)
+void nf_unregister_queue_handler(struct net *net)
 {
-   RCU_INIT_POINTER(queue_handler, NULL);
-   synchronize_rcu();
+   RCU_INIT_POINTER(net->nf.queue_handler, NULL);
 }
 EXPORT_SYMBOL(nf_unregister_queue_handler);
 
@@ -103,7 +101,7 @@ void nf_queue_nf_hook_drop(struct net *net, struct 
nf_hook_ops *ops)
const struct nf_queue_handler *qh;
 
rcu_read_lock();
-   qh = rcu_dereference(queue_handler);
+   qh = rcu_dereference(net->nf.queue_handler);
if (qh)
qh->nf_hook_drop(net, ops);
rcu_read_unlock();