| 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: <a43d7bce-5f70-4d69-8bad-c65976245996@gmail.com>
Date: Tue, 15 Apr 2025 00:29:14 +0300
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Loic Poulain <loic.poulain@....qualcomm.com>
Cc: 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
Subject: Re: [RFC PATCH 2/6] net: wwan: core: split port creation and
registration
Hi Loic,
thank you that you found a time to check it. See the explanation below,
might be you can suggest a better solution.
On 14.04.2025 21:50, Loic Poulain wrote:
> Hi Sergey,
>
> On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>>
>> Upcoming GNSS (NMEA) port type support requires exporting it via the
>> GNSS subsystem. On another hand, we still need to do basic WWAN core
>> work: find or allocate the WWAN device, make it the port parent, etc. To
>> reuse as much code as possible, split the port creation function into
>> the registration of a regular WWAN port device, and basic port struct
>> initialization.
>>
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
>> ---
>> drivers/net/wwan/wwan_core.c | 86 ++++++++++++++++++++++--------------
>> 1 file changed, 53 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
>> index ade8bbffc93e..045246d7cd50 100644
>> --- a/drivers/net/wwan/wwan_core.c
>> +++ b/drivers/net/wwan/wwan_core.c
>> @@ -357,16 +357,19 @@ static struct attribute *wwan_port_attrs[] = {
>> };
>> ATTRIBUTE_GROUPS(wwan_port);
>>
>> -static void wwan_port_destroy(struct device *dev)
>> +static void __wwan_port_destroy(struct wwan_port *port)
>> {
>> - struct wwan_port *port = to_wwan_port(dev);
>> -
>> - ida_free(&minors, MINOR(port->dev.devt));
>> mutex_destroy(&port->data_lock);
>> mutex_destroy(&port->ops_lock);
>> kfree(port);
>> }
>>
>> +static void wwan_port_destroy(struct device *dev)
>> +{
>> + ida_free(&minors, MINOR(dev->devt));
>> + __wwan_port_destroy(to_wwan_port(dev));
>> +}
>> +
>> static const struct device_type wwan_port_dev_type = {
>> .name = "wwan_port",
>> .release = wwan_port_destroy,
>> @@ -440,6 +443,49 @@ static int __wwan_port_dev_assign_name(struct wwan_port *port, const char *fmt)
>> return dev_set_name(&port->dev, "%s", buf);
>> }
>>
>> +/* Register a regular WWAN port device (e.g. AT, MBIM, etc.)
>> + *
>> + * NB: in case of error function frees the port memory.
>> + */
>> +static int wwan_port_register_wwan(struct wwan_port *port)
>> +{
>> + struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>> + char namefmt[0x20];
>> + int minor, err;
>> +
>> + /* A port is exposed as character device, get a minor */
>> + minor = ida_alloc_range(&minors, 0, WWAN_MAX_MINORS - 1, GFP_KERNEL);
>> + if (minor < 0) {
>> + __wwan_port_destroy(port);
>
> I see this is documented above, but it's a bit weird that the port is
> freed inside the register function, it should be up to the caller to
> do this. Is there a reason for this?
I agree that this looks weird and asymmetrical. I left the port
allocation in wwan_create_port() because both WWAN-exported and
GNSS-exported types of port share the same port allocation. And the port
struct is used as a container to keep all the port registration arguments.
I did the port freeing inside this function because we free the port
differently depending of the device registration state. If we fail to
initialize the port at earlier stage then we use __wwan_port_destroy()
which basically just releases the memory.
But if device_register() fails then we are required to use put_device()
which does more job.
I do not think it is acceptable to skip put_device() call and just
release the memory. Also I do not find maintainable to partially open
code put_device() here in the WWAN-exportable handler and release the
memory in caller function wwan_create_port().
We could somehow try to return this information from
wwan_port_register_wwan() to wwan_create_port(), so the caller could
decide, shall it use __wwan_port_destroy() or put_device() in case of
failure.
But I can not see a way to clearly indicate, which releasing approach
should be used by the caller. And even in this case it going to look
weird since the called function controls the caller.
Another solution for the asymmetry problem is to move the allocation
from the caller to the called function. So the memory will be allocated
and released in the same function. But in this case we will need to pass
all the parameters from wwan_create_port() to wwan_port_register_wwan().
Even if we consolidate the port basic allocation/initialization in a
common routine, the final solution going to look a duplication. E.g.
struct wwan_port *wwan_port_allocate(struct wwan_device *wwandev,
enum wwan_port_type type,
const struct wwan_port_ops *ops,
struct wwan_port_caps *caps,
void *drvdata)
{
/* Do the mem allocation and init here */
return port;
}
struct wwan_port *wwan_port_register_wwan(struct wwan_device *wwandev,
enum wwan_port_type type,
const struct wwan_port_ops *ops,
struct wwan_port_caps *caps,
void *drvdata)
{
port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
/* Proceed with chardev registration or release on failure */
/* return port; or return ERR_PTR(-err); */
}
struct wwan_port *wwan_port_register_gnss(struct wwan_device *wwandev,
enum wwan_port_type type,
const struct wwan_port_ops *ops,
struct wwan_port_caps *caps,
void *drvdata)
{
port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
/* Proceed with GNSS registration or release on failure */
/* return port; or return ERR_PTR(-err); */
}
struct wwan_port *wwan_create_port(struct device *parent,
enum wwan_port_type type,
const struct wwan_port_ops *ops,
struct wwan_port_caps *caps,
void *drvdata)
{
...
wwandev = wwan_create_dev(parent);
if (type == WWAN_PORT_NMEA)
port = wwan_port_register_gnss(wwandev, type, ops,
caps, drvdata);
else
port = wwan_port_register_wwan(wwandev, type, ops,
caps, drvdata);
if (!IS_ERR(port))
return port;
wwan_remove_dev(wwandev);
return ERR_CAST(port);
}
wwan_create_port() looks better in prices of passing a list of arguments
and allocating the port in multiple places.
Maybe some other design approach, what was overseen?
For me, the ideal solution would be a routine that works like
put_device() except calling the device type release handler. Then we can
use it to cleanup leftovers of the failed device_register() call and
then release the memory in the calling wwan_create_port() function.
>> + return minor;
>> + }
>> +
>> + port->dev.class = &wwan_class;
>> + port->dev.type = &wwan_port_dev_type;
>> + port->dev.devt = MKDEV(wwan_major, minor);
>> +
>> + /* allocate unique name based on wwan device id, port type and number */
>> + snprintf(namefmt, sizeof(namefmt), "wwan%u%s%%d", wwandev->id,
>> + wwan_port_types[port->type].devsuf);
>> +
>> + /* Serialize ports registration */
>> + mutex_lock(&wwan_register_lock);
>> +
>> + __wwan_port_dev_assign_name(port, namefmt);
>> + err = device_register(&port->dev);
>> +
>> + mutex_unlock(&wwan_register_lock);
>> +
>> + if (err) {
>> + put_device(&port->dev);
>> + return err;
>> + }
>> +
>> + dev_info(&wwandev->dev, "port %s attached\n", dev_name(&port->dev));
>> +
>> + return 0;
>> +}
>> +
>> struct wwan_port *wwan_create_port(struct device *parent,
>> enum wwan_port_type type,
>> const struct wwan_port_ops *ops,
>> @@ -448,8 +494,7 @@ struct wwan_port *wwan_create_port(struct device *parent,
>> {
>> struct wwan_device *wwandev;
>> struct wwan_port *port;
>> - char namefmt[0x20];
>> - int minor, err;
>> + int err;
>>
>> if (type > WWAN_PORT_MAX || !ops)
>> return ERR_PTR(-EINVAL);
>> @@ -461,17 +506,9 @@ struct wwan_port *wwan_create_port(struct device *parent,
>> if (IS_ERR(wwandev))
>> return ERR_CAST(wwandev);
>>
>> - /* A port is exposed as character device, get a minor */
>> - minor = ida_alloc_range(&minors, 0, WWAN_MAX_MINORS - 1, GFP_KERNEL);
>> - if (minor < 0) {
>> - err = minor;
>> - goto error_wwandev_remove;
>> - }
>> -
>> port = kzalloc(sizeof(*port), GFP_KERNEL);
>> if (!port) {
>> err = -ENOMEM;
>> - ida_free(&minors, minor);
>> goto error_wwandev_remove;
>> }
>>
>> @@ -485,31 +522,14 @@ struct wwan_port *wwan_create_port(struct device *parent,
>> mutex_init(&port->data_lock);
>>
>> port->dev.parent = &wwandev->dev;
>> - port->dev.class = &wwan_class;
>> - port->dev.type = &wwan_port_dev_type;
>> - port->dev.devt = MKDEV(wwan_major, minor);
>> dev_set_drvdata(&port->dev, drvdata);
>>
>> - /* allocate unique name based on wwan device id, port type and number */
>> - snprintf(namefmt, sizeof(namefmt), "wwan%u%s%%d", wwandev->id,
>> - wwan_port_types[port->type].devsuf);
>> -
>> - /* Serialize ports registration */
>> - mutex_lock(&wwan_register_lock);
>> -
>> - __wwan_port_dev_assign_name(port, namefmt);
>> - err = device_register(&port->dev);
>> -
>> - mutex_unlock(&wwan_register_lock);
>> -
>> + err = wwan_port_register_wwan(port);
>> if (err)
>> - goto error_put_device;
>> + goto error_wwandev_remove;
>>
>> - dev_info(&wwandev->dev, "port %s attached\n", dev_name(&port->dev));
>> return port;
>>
>> -error_put_device:
>> - put_device(&port->dev);
>> error_wwandev_remove:
>> wwan_remove_dev(wwandev);
>>
--
Sergey
Powered by blists - more mailing lists