Re: [PATCH v1 00/10] devfreq: Fix NULL pointer dereference when a governor module is unloaded

[Jie Zhan <zhanjie9@hisilicon.com>]

On 3/31/2026 9:30 AM, Yaxiong Tian wrote:
> 
> 在 2026/3/30 14:29, Jie Zhan 写道:
>>
>> On 3/27/2026 10:06 AM, Yaxiong Tian wrote:
>>> 在 2026/3/26 21:14, Jie Zhan 写道:
>>>> On 3/26/2026 8:34 PM, Jie Zhan wrote:
>>>>> When compiled as a kernel module, the governor module can be dynamically
>>>>> inserted or removed.  'devfreq->governor' would become NULL if the governor
>>>>> module is removed when it's in use, and NULL pointer dereference would be
>>>>> triggered.  A similar issue was also reported in [1].
>>>>>
>>>>> To address this issue:
>>>>>
>>>>> Patch 1-5 rework mutex, factor out a common governor setting function, and
>>>>> clean up some unreachable code.
>>>>>
>>>>> Patch 6-8 prevent a governor module in use from being removed (except for
>>>>> force unload) by getting/putting a refcount of the governor's module when
>>>>> switching governors.
>>>>>
>>>>> Patch 9-10 allow 'governor' and 'available_governors' to work normally even
>>>>> when a governor module in use is force unloaded.
>>>>>
>>>>> Note that this series is based on [1] or devfreq-next, otherwise code
>>> Sorry, please ignore the "remember to CC me on the patches." in my previous email.
>>>
>>>   In my opinion, it would be better to prioritize the FIX first before proceeding with the lock mechanism optimizations and other work. This would make it easier to backport the patches to lower-version kernels. I noticed the patch is already in the devfreq-testing branch. I hope the FIX work can be moved forward smoothly to resolve the null pointer and other bugs. Thank you!
>> Hi Yaxiong,
>>
>> I don't think this issue is urgent because it can only possibly happen when
>> governors are built as loadable modules, which is typically used for
>> development rather than production.
> No, loading/unloading modules is also a user operational requirement, not just for development.
> 
>>
>> For downstream kernels, feel free to go ahead with quick fixes.  For the
>> upstream kernel, however, I'd prefer to make the devfreq core clean and
>> sensible.
> 
> I don't think code cleanup should take priority over bug fixes, especially when you already know there's an issue with the functionality. In fact, the version users are running is not the upstream version, but rather individual stable releases.

The point is to do the bugfix in a different way rather than cleanups.

> 
> Most users don't have the time or energy to research and fix kernel issues themselves; they rely on upstream community patches most of the time.
> 
> By the way, although your patch subject says "FIX", I don't see any fix tag.
> 
>>
>> Your approach is to prevent kernel panics when unloading governor modules
>> before changing governors.
>>
>> This patchset achieves that, and additionally let userspace get a friendly
>> warning when trying to remove a governor module in use, e.g.
>> "rmmod: ERROR: Module governor_performance is in use".
>> This requires using the module refcount stuff, and brings out a set of
>> cleanups and refactoring.  BTW, cpufreq implements a similar mechanism like
>> this.
>>
>> I may carry your fourth patch that changes the return code of
>> governor_show() in v2 and address some Sashiko review comments along the
>> way.
>>
>> Please let me know if this works for you?
> So I don't approve of this. Maybe you should ask for others' opinions, such as Chanwoo Choi.
> 
No problem.  I see your point.
I'll take a closer look at your version and see if that works.
The module owner refcount related stuff can be rebased later if needed.

Thanks,
Jie
>>
>> Regards,
>> Jie
>>>
>>>
>>>> sorry, based on [2] or devfreq-next
>>>>> would conflict.
>>>>>
>>>>> [1] https://lore.kernel.org/all/20260319091409.998397-1-tianyaxiong@kylinos.cn/
>>>>> [2] https://lore.kernel.org/all/20251216031153.2242306-1-zhangpengjie2@huawei.com/