| 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: <CAFEp6-1veH3N+eVw2Bc+=2ZhrQAzTcU8Lw9fHTmY2334gaDBSg@mail.gmail.com>
Date: Thu, 17 Apr 2025 22:35:08 +0200
From: Loic Poulain <loic.poulain@....qualcomm.com>
To: Sergey Ryazanov <ryazanov.s.a@...il.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 Sergey,
On Mon, Apr 14, 2025 at 11:28 PM Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>
> 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.
Ok I see, thanks for the clear explanation, I don't see a perfect
solution here without over complication. So the current approach is
acceptable, can you add a comment in the caller function as well,so
that it's clear why we don't have to release the port on error.
Powered by blists - more mailing lists