Re: [PATCH ipsec] xfrm: keep the mode module pinned until destroy_state returns

[Leon Romanovsky <leon@kernel.org>]

On Thu, Oct 29, 2026 at 11:19:11AM +0800, Cen Zhang wrote:
> An IPTFS state keeps its mode module referenced while mode_data exists.
> However, iptfs_destroy_state() drops that reference before returning to
> xfrm_state_gc_destroy(), which has no reference covering the callback's
> return path.

This is a good example of why module dependencies should be handled through
direct calls to functions exported with EXPORT_SYMBOL(), rather than through
callbacks for inter-module communication.

Thanks

> 
> With CONFIG_XFRM_IPTFS=m and module unloading enabled, deleting the last
> IPTFS SA can overlap with ordinary module removal in this order:
> 
>   XFRM GC worker                      rmmod xfrm_iptfs
>   iptfs_destroy_state()
>     free mode_data
>     module_put(owner)
>     <preempted>
>                                       delete_module()
>                                         xfrm_iptfs_fini()
>                                           unregister mode callbacks
>                                         free_module()
>     resume in freed module text
> 
> An unload started after the put can finish before the callback resumes.
> GC invokes the callback outside an RCU read-side critical section, after
> its grace period. The RCU waits in mode unregistration and module
> freeing therefore do not protect the callback. The GC mutex only
> serializes GC with device state flushes. Resuming the callback can cause
> an instruction-fetch fault in the freed module text.
> 
> The Oops below was observed on an earlier kernel with scheduling probes
> holding the worker in module_put() after the decrement until ordinary
> rmmod completed. The fixture created and deleted the SA over
> NETLINK_XFRM.
> 
> Take an additional reference to the already pinned owner before calling
> destroy_state(), and release it in XFRM core after the callback returns.
> Cache the owner before the call so releasing the extra reference does
> not need to access the callback table after the callback has finished.
> 
> Oops report as below:
>     BUG: unable to handle page fault for address: ffffffffc0407455
>     #PF: supervisor instruction fetch in kernel mode
>     #PF: error_code(0x0010) - not-present page
>     PGD 80a9067 P4D 80a9067 PUD 80ab067 PMD 0 
>     Oops: Oops: 0010 [#1] SMP KASAN NOPTI
>     CPU: 3 UID: 0 PID: 53 Comm: kworker/3:1 Not tainted 7.2.0-rc5-pmb-xfrm-functional-v1+ #11 PREEMPT(lazy) 
>     Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
>     Workqueue: events xfrm_state_gc_task
>     RIP: 0010:0xffffffffc0407455
>     Code: Unable to access opcode bytes at 0xffffffffc040742b.
>     RSP: 0018:ffff8881013e7aa0 EFLAGS: 00010282
>     RAX: 0000000000000000 RBX: ffffffffc02199c0 RCX: ffffffff816fc84e
>     RDX: ffff8881013c0000 RSI: 0000000000000000 RDI: ffffffff86338344
>     RBP: ffff8881013e7ba8 R08: 0000000000000001 R09: 0000000000000001
>     R10: ffffffff893e0257 R11: ffff8881013c0000 R12: ffff888107e9ec00
>     R13: ffff888102b07980 R14: ffff888107412ec0 R15: ffff8881013e7ae0
>     FS:  0000000000000000(0000) GS:ffff8881fd929000(0000) knlGS:0000000000000000
>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     CR2: ffffffffc040742b CR3: 00000000080a6006 CR4: 0000000000770ef0
>     PKRU: 55555554
>     Call Trace:
>      <TASK>
>      ? pmbd_gate_maybe+0x47/0x60
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      ? lock_is_held_type+0x8f/0x100
>      xfrm_state_gc_task+0x17e/0x790
>      ? lock_acquire+0x17b/0x2f0
>      ? __pfx_xfrm_state_gc_task+0x10/0x10
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      ? lock_release+0xc8/0x280
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      process_one_work+0x908/0x19c0
>      ? __pfx_process_one_work+0x10/0x10
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      ? lock_is_held_type+0x8f/0x100
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      worker_thread+0x65c/0xe40
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      ? __kthread_parkme+0x175/0x220
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      ? __pfx_worker_thread+0x10/0x10
>      kthread+0x34f/0x460
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      ? __pfx_kthread+0x10/0x10
>      ret_from_fork+0x659/0x940
>      ? __pfx_ret_from_fork+0x10/0x10
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      ? __switch_to+0x74f/0xf80
>      ? __pfx_kthread+0x10/0x10
>      ret_from_fork_asm+0x1a/0x30
>      </TASK>
>     Modules linked in: [last unloaded: xfrm_iptfs]
>     CR2: ffffffffc0407455
>     ---[ end trace 0000000000000000 ]---
>     RIP: 0010:0xffffffffc0407455
>     Code: Unable to access opcode bytes at 0xffffffffc040742b.
>     RSP: 0018:ffff8881013e7aa0 EFLAGS: 00010282
>     RAX: 0000000000000000 RBX: ffffffffc02199c0 RCX: ffffffff816fc84e
>     RDX: ffff8881013c0000 RSI: 0000000000000000 RDI: ffffffff86338344
>     RBP: ffff8881013e7ba8 R08: 0000000000000001 R09: 0000000000000001
>     R10: ffffffff893e0257 R11: ffff8881013c0000 R12: ffff888107e9ec00
>     R13: ffff888102b07980 R14: ffff888107412ec0 R15: ffff8881013e7ae0
>     FS:  0000000000000000(0000) GS:ffff8881fd929000(0000) knlGS:0000000000000000
>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     CR2: ffffffffc040742b CR3: 00000000080a6006 CR4: 0000000000770ef0
>     PKRU: 55555554
>     Kernel panic - not syncing: Fatal exception
>     Kernel Offset: disabled
> 
> Fixes: 4b3faf610cc6 ("xfrm: iptfs: add new iptfs xfrm mode impl")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> Assisted-by: LLM
> 
> ---
>  net/xfrm/xfrm_state.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 0555e5d796ed..45a52771796b 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -596,8 +596,14 @@ EXPORT_SYMBOL(xfrm_state_free);
>  static void xfrm_state_delete_tunnel(struct xfrm_state *x);
>  static void xfrm_state_gc_destroy(struct xfrm_state *x)
>  {
> -	if (x->mode_cbs && x->mode_cbs->destroy_state)
> +	if (x->mode_cbs && x->mode_cbs->destroy_state) {
> +		struct module *owner = x->mode_cbs->owner;
> +
> +		/* The callback may drop the last module reference. */
> +		__module_get(owner);
>  		x->mode_cbs->destroy_state(x);
> +		module_put(owner);
> +	}
>  	hrtimer_cancel(&x->mtimer);
>  	timer_delete_sync(&x->rtimer);
>  	kfree_sensitive(x->aead);
>