Re: [PATCH v2] module: Fix use-after-free in module_put() tracepoint

[Petr Pavlu <petr.pavlu@suse.com>]

On 11/9/26 3:28 PM, Aaron Tomlin wrote:
> On Mon, Nov 09, 2026 at 02:47:31PM +0100, Petr Pavlu wrote:
>> On 10/22/26 8:45 PM, Aaron Tomlin wrote:
>>> A use-after-free condition can arise in module_put() should a thread be
>>> preempted immediately after decrementing the module's reference count
>>> while the module is concurrently being unloaded.
>>>
>>> In module_put(), the module's reference count is decremented via
>>> atomic_dec_if_positive() before the module pointer is passed to
>>> trace_module_put(). Consider a scenario where CPU 0 decrements the
>>> reference count without reaching the baseline MODULE_REF_BASE level and
>>> is immediately preempted: it relinquishes its reference while still
>>> retaining a local pointer to the module. If a concurrent CPU 1
>>> subsequently drops the final active reference, the module's reference
>>> count reaches MODULE_REF_BASE. This permits the module unload sequence
>>> (via delete_module() or the initialisation-failure tear-down path) to
>>> proceed and invoke free_module().
>>>
>>> Function free_module() invokes synchronize_rcu() and subsequently
>>> releases the memory backing struct module via free_mod_mem(). Because
>>> CPU 0 was preempted outside an RCU read-side critical section,
>>> synchronize_rcu() does not await CPU 0. When CPU 0 eventually resumes
>>> execution, it passes the dangling module pointer to trace_module_put(),
>>> which dereferences mod->name and mod->refcnt, culminating in a
>>> use-after-free. This sequence of events is illustrated below:
>>>
>>>     CPU 0                                       CPU 1
>>>     -----                                       -----
>>>
>>>     module_put(module)
>>>       atomic_dec_if_positive()
>>>       [preempted]
>>>                                         module_put(module)
>>>                                           atomic_dec_if_positive()
>>>                                           // drops final reference
>>>                                         free_module(module)
>>>                                           synchronize_rcu()
>>>                                           free_mod_mem(module)
>>>       trace_module_put(module, ...)
>>>         mod->name
>>>
>>> Resolve this by moving trace_module_put() prior to
>>> atomic_dec_if_positive(). Because the caller holds an active reference
>>> to the module at this point, the module structure and its name are
>>> guaranteed to remain valid throughout the tracepoint invocation without
>>> requiring preemption disabling or RCU synchronisation.
>>>
>>> Fixes: cb0b50b813f6 ("module: Remove preempt_disable() from module reference counting.")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Aaron Tomlin <atomlin@atomlin.com>
>>
>> This version #2 looks ok to me. I plan to pick it up on the modules
>> tree.
>>
>> Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
> 
> Hi Petr,
> 
>> The additional version #3 [1] seems unnecessarily complicated to me. It
>> is ok to report the value of module->refcnt before it is decremented. As
>> noted earlier, other code, such as xfs_group_put() and objagg_obj_put(),
>> takes the same approach.
>>
>> [1] https://lore.kernel.org/linux-modules/20261103163103.13195-1-atomlin@atomlin.com/
> 
> Thank you.
> 
> However, I do prefer the next iteration [1], since the use-after-free is
> resolved without altering existing tracepoint semantics or introducing an
> off-by-one discrepancy in the recorded reference count. Finally, the fast
> code path is fully preserved when the tracepoint is disabled.
Why is it important to preserve what this tracepoint reports? The value
isn't off by one, rather the tracepoint would now report the value
before the decrement.

I'm trying to understand why this case matters enough to justify the
additional complexity in version #3.

-- 
Cheers,
Petr