On Thu, Jan 31, 2019 at 01:05:49PM -0800, Cong Wang wrote:
> xfrm_state_put() moves struct xfrm_state to the GC list
> and schedules the GC work to clean it up. On net exit call
> path, xfrm_state_flush() is called to clean up and
> xfrm_flush_gc() is called to wait for the GC work to complete
> before exit.
>
> However, this doesn't work because one of the ->destructor(),
> ipcomp_destroy(), schedules the same GC work again inside
> the GC work. It is hard to wait for such a nested async
> callback. This is also why syzbot still reports the following
> warning:
>
> WARNING: CPU: 1 PID: 33 at net/ipv6/xfrm6_tunnel.c:351
> xfrm6_tunnel_net_exit+0x2cb/0x500 net/ipv6/xfrm6_tunnel.c:351
> ...
> ops_exit_list.isra.0+0xb0/0x160 net/core/net_namespace.c:153
> cleanup_net+0x51d/0xb10 net/core/net_namespace.c:551
> process_one_work+0xd0c/0x1ce0 kernel/workqueue.c:2153
> worker_thread+0x143/0x14a0 kernel/workqueue.c:2296
> kthread+0x357/0x430 kernel/kthread.c:246
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
>
> In fact, it is perfectly fine to bypass GC and destroy xfrm_state
> synchronously on net exit call path, because it is in process context
> and doesn't need a work struct to do any blocking work.
>
> This patch introduces xfrm_state_put_sync() which simply bypasses
> GC, and lets its callers to decide whether to use this synchronous
> version. On net exit path, xfrm_state_fini() and
> xfrm6_tunnel_net_exit() use it. And, as ipcomp_destroy() itself is
> blocking, it can use xfrm_state_put_sync() directly too.
>
> Also rename xfrm_state_gc_destroy() to ___xfrm_state_destroy() to
> reflect this change.
>
> Fixes: b48c05ab5d32 ("xfrm: Fix warning in xfrm6_tunnel_net_exit.")
> Reported-and-tested-by: syzbot+e9aebef558e3ed673...@syzkaller.appspotmail.com
> Cc: Steffen Klassert <steffen.klass...@secunet.com>
> Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
Applied, thanks a lot!
