| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <41e78126.726c.19bc0b2e424.Coremail.slark_xiao@163.com>
Date: Thu, 15 Jan 2026 16:08:27 +0800 (CST)
From: "Slark Xiao" <slark_xiao@....com>
To: "Sergey Ryazanov" <ryazanov.s.a@...il.com>
Cc: "Loic Poulain" <loic.poulain@....qualcomm.com>,
"Johannes Berg" <johannes@...solutions.net>,
"Andrew Lunn" <andrew+netdev@...n.ch>,
"Eric Dumazet" <edumazet@...gle.com>,
"David S . Miller" <davem@...emloft.net>,
"Jakub Kicinski" <kuba@...nel.org>,
"Paolo Abeni" <pabeni@...hat.com>, netdev@...r.kernel.org,
"Daniele Palmas" <dnlplm@...il.com>
Subject: Re:Re: [RFC PATCH v5 2/7] net: wwan: core: explicit WWAN device
reference counting
At 2026-01-15 14:52:08, "Sergey Ryazanov" <ryazanov.s.a@...il.com> wrote:
>On January 14, 2026 11:31:45 PM, Loic Poulain <loic.poulain@....qualcomm.com> wrote:
>>On Fri, Jan 9, 2026 at 2:09 AM Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>>>
>>> We need information about existing WWAN device children since we remove
>>> the device after removing the last child. Previously, we tracked users
>>> implicitly by checking whether ops was registered and existence of a
>>> child device of the wwan_class class. Upcoming GNSS (NMEA) port type
>>> support breaks this approach by introducing a child device of the
>>> gnss_class class.
>>>
>>> And a modem driver can easily trigger a kernel Oops by removing regular
>>> (e.g., MBIM, AT) ports first and then removing a GNSS port. The WWAN
>>> device will be unregistered on removal of a last regular WWAN port. And
>>> subsequent GNSS port removal will cause NULL pointer dereference in
>>> simple_recursive_removal().
>>>
>>> In order to support ports of classes other than wwan_class, switch to
>>> explicit references counting. Introduce a dedicated counter to the WWAN
>>> device struct, increment it on every wwan_create_dev() call, decrement
>>> on wwan_remove_dev(), and actually unregister the WWAN device when there
>>> are no more references.
>>>
>>> Run tested with wwan_hwsim with NMEA support patches applied and
>>> different port removing sequences.
>>>
>>> Reported-by: Daniele Palmas <dnlplm@...il.com>
>>> Closes: https://lore.kernel.org/netdev/CAGRyCJE28yf-rrfkFbzu44ygLEvoUM7fecK1vnrghjG_e9UaRA@mail.gmail.com/
>>> Suggested-by: Loic Poulain <loic.poulain@....qualcomm.com>
>>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
>>> ---
>>> Changes:
>>> * RFCv2->RFCv5: new patch to address modem disconnection / system
>>> shutdown issues
>>> ---
>>> drivers/net/wwan/wwan_core.c | 37 ++++++++++++++++++------------------
>>> 1 file changed, 18 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
>>> index ade8bbffc93e..33f7a140fba9 100644
>>> --- a/drivers/net/wwan/wwan_core.c
>>> +++ b/drivers/net/wwan/wwan_core.c
>>> @@ -42,6 +42,9 @@ static struct dentry *wwan_debugfs_dir;
>>> * struct wwan_device - The structure that defines a WWAN device
>>> *
>>> * @id: WWAN device unique ID.
>>> + * @refcount: Reference count of this WWAN device. When this refcount reaches
>>> + * zero, the device is deleted. NB: access is protected by global
>>> + * wwan_register_lock mutex.
>>> * @dev: Underlying device.
>>> * @ops: wwan device ops
>>> * @ops_ctxt: context to pass to ops
>>> @@ -49,6 +52,7 @@ static struct dentry *wwan_debugfs_dir;
>>> */
>>> struct wwan_device {
>>> unsigned int id;
>>> + int refcount;
>>> struct device dev;
>>> const struct wwan_ops *ops;
>>> void *ops_ctxt;
>>> @@ -222,8 +226,10 @@ static struct wwan_device *wwan_create_dev(struct device *parent)
>>>
>>> /* If wwandev already exists, return it */
>>> wwandev = wwan_dev_get_by_parent(parent);
>>> - if (!IS_ERR(wwandev))
>>> + if (!IS_ERR(wwandev)) {
>>> + wwandev->refcount++;
>>> goto done_unlock;
>>> + }
>>>
>>> id = ida_alloc(&wwan_dev_ids, GFP_KERNEL);
>>> if (id < 0) {
>>> @@ -242,6 +248,7 @@ static struct wwan_device *wwan_create_dev(struct device *parent)
>>> wwandev->dev.class = &wwan_class;
>>> wwandev->dev.type = &wwan_dev_type;
>>> wwandev->id = id;
>>> + wwandev->refcount = 1;
>>> dev_set_name(&wwandev->dev, "wwan%d", wwandev->id);
>>>
>>> err = device_register(&wwandev->dev);
>>> @@ -263,30 +270,21 @@ static struct wwan_device *wwan_create_dev(struct device *parent)
>>> return wwandev;
>>> }
>>>
>>> -static int is_wwan_child(struct device *dev, void *data)
>>> -{
>>> - return dev->class == &wwan_class;
>>> -}
>>> -
>>> static void wwan_remove_dev(struct wwan_device *wwandev)
>>> {
>>> - int ret;
>>> -
>>> /* Prevent concurrent picking from wwan_create_dev */
>>> mutex_lock(&wwan_register_lock);
>>
>>FYI, you can use guarded mutex:
>>guard(mutex)(&wwan_register_lock);
>>This ensures the lock is 'automatically' released when leaving the
>>scope/function, and would save the below goto/out_unlock.
>
>Sounds curious. Will keep in my mind for a future development. I would rather keep this patch as simple and clear as possible.
>
>>> - /* WWAN device is created and registered (get+add) along with its first
>>> - * child port, and subsequent port registrations only grab a reference
>>> - * (get). The WWAN device must then be unregistered (del+put) along with
>>> - * its last port, and reference simply dropped (put) otherwise. In the
>>> - * same fashion, we must not unregister it when the ops are still there.
>>> - */
>>> - if (wwandev->ops)
>>> - ret = 1;
>>> - else
>>> - ret = device_for_each_child(&wwandev->dev, NULL, is_wwan_child);
>>> + if (--wwandev->refcount <= 0) {
>>> + struct device *child = device_find_any_child(&wwandev->dev);
>>> +
>>> + if (WARN_ON(wwandev->ops)) /* Paranoid */
>>
>>You may keep a reference to child (if existing)
>>
>>> + goto out_unlock;
>>> + if (WARN_ON(child)) { /* Paranoid */
>>> + put_device(child);
>>> + goto out_unlock;
>>> + }
>>
>>Maybe you can simplify that with:
>>```
>>struct device *child = device_find_any_child(&wwandev->dev);
>>put_device(child) /* NULL param is ok */
>>if (WARN_ON(child || wwandev->ops))
>> return; /* or goto */
>>```
>
>Good point.
>
>Slark, could you adjust this in your future submission or do you want me to send another RFC series?
>
I will update it in my next submission.
>>>
>>> - if (!ret) {
>>> #ifdef CONFIG_WWAN_DEBUGFS
>>> debugfs_remove_recursive(wwandev->debugfs_dir);
>>> #endif
>>> @@ -295,6 +293,7 @@ static void wwan_remove_dev(struct wwan_device *wwandev)
>>> put_device(&wwandev->dev);
>>> }
>>>
>>> +out_unlock:
>>> mutex_unlock(&wwan_register_lock);
>>> }
>
>--
>Sergey
>
>Hi Loic,
Powered by blists - more mailing lists