netdev - Re: [RFC PATCH 2/6] net: wwan: core: split port creation and registration

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <01c17d6f-dfb4-4464-b33e-6e3ed64ecaae@gmail.com>
Date: Sat, 19 Apr 2025 19:51:07 +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

On 19.04.2025 14:44, Loic Poulain wrote:
> On Sat, Apr 19, 2025 at 1:04 AM Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>> ...
>> Looks like I've found another one solution to move the port resources
>> (memory) release back to the function allocating it. It is also a bit
>> hackish and I would like to hear a feedback from you.
>>
>> The port is released inside wwan_port_register_wwan() just because we
>> release it differently depending on the initialization stage, right?
>>
>> We cannot call put_device() before the device_register() call. And that
>> is why I've created that __wwan_port_destroy() routine. What if we make
>> port eligible to be released with put_device() ASAP? Internally,
>> device_register() just sequentially calls device_initialize() and then
>> device_add(). Indeed, device_initialize() makes a device struct eligible
>> to be released with put_device().
>>
>> We can avoid using device_register() and instead of it, do the
>> registration step by step. Just after the port memory allocation and
>> very basic initialization, we can call device_initialize(). And then
>> call device_add() when it is time to register the port with the kernel.
>> And if something going to go wrong, we can return just an error from
>> wwan_port_register_wwan() and release the port with put_device() in
>> wwan_create_port() where it was allocated. Something like this:
>>
>> static int wwan_port_register_wwan(struct wwan_port *port)
>> {
>>       ...
>>       if (something_wrong)
>>           return -E<ERROR_TYPE>;
>>       ...
>>       return 0;
>> }
>>
>> 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)
>> {
>>       ...
>>       port = kzalloc(sizeof(*port), GFP_KERNEL);
>>       /* Do basic port init here */
>>       port->dev.type = &wwan_port_dev_type;
>>       device_initialize(&port->dev);  /* allows put_device() usage */
>>
>>       if (port->type == WWAN_PORT_NMEA)
>>           err = wwan_port_register_gnss(port);
>>       else
>>           err = wwan_port_register_wwan(port);
>>
>>       if (err) {
>>           put_device(&port->dev);
>>           goto error_wwandev_remove;
>>       }
>>
>>       return port;
>>       ...
>> }
>>
>> The only drawback I see here is that we have to use put_device() to
>> release the port memory even in case of GNSS port. We don't actually
>> register the port as device, but I believe, this can be explained with a
>> proper comment.
> 
> Yes, that's a good alternative, so you would also have to use
> put_device in gnss_unregister, or do something like:
> 
> void wwan_remove_port(struct wwan_port *port)
> {
> [...]
>      if (port->type == WWAN_PORT_NMEA)
>                  wwan_port_unregister_gnss(port);
>      /* common unregistering (put_device), not necessarily in a
> separate function */
>     wwan_port_unregister(port);
> [...]
> }
> 
> And probably have a common wwan_port_destroy function:
> static void wwan_port_destroy(struct device *dev)
> {
>      if (port->type != WWAN_PORT_NMEA)
>          ida_free(&minors, MINOR(dev->devt));
>      [...]
>      kfree(port);
> }

Yep. This change also makes sense if we manage to release the port in a 
unified way. Will try to implement it next week to see how it's going to 
look like in as a code.

--
Sergey