netdev - Re: [PATCH] net: wwan: fix port open

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAMZdPi9DB09dsjuFt-d6U5-NdVsoZ3NqDbmegLyCndRU9G9gJQ@mail.gmail.com>
Date:   Wed, 4 May 2022 16:44:32 +0200
From:   Loic Poulain <loic.poulain@...aro.org>
To:     M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, davem@...emloft.net,
        johannes@...solutions.net, ryazanov.s.a@...il.com,
        krishna.c.sudi@...el.com, m.chetan.kumar@...el.com,
        linuxwwan@...el.com
Subject: Re: [PATCH] net: wwan: fix port open

Hi Chetan,

On Wed, 4 May 2022 at 16:09, M Chetan Kumar
<m.chetan.kumar@...ux.intel.com> wrote:
>
> Wwan device registered port can be opened as many number of times.
> The first port open() call binds dev file to driver wwan port device
> and subsequent open() call references to same wwan port instance.
>
> When dev file is opened multiple times, all contexts still refers to
> same instance of wwan port. So in tx path, the received data will be
> fwd to wwan device but in rx path the wwan port has a single rx queue.
> Depending on which context goes for early read() the rx data gets
> dispatched to it.
>
> Since the wwan port is not handling dispatching of rx data to right
> context restrict wwan port open to single context.

The reason for this behavior comes from:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2313348.html

Especially:
---
"I told you before, do not try to keep a device node from being opened
multiple times, as it will always fail (think about passing file
handles around between programs...) If userspace wants to do this, it
will do it.  If your driver can't handle that, that's fine, userspace
will learn not to do that. But the kernel can not prevent this from
happening."
---

So maybe a user-side solution would be more appropriate, /var/lock/ ?

Regards,
Loic


>
> Signed-off-by: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
> ---
>  drivers/net/wwan/wwan_core.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index b8c7843730ed..9ca2d8d76587 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -33,6 +33,7 @@ static struct dentry *wwan_debugfs_dir;
>
>  /* WWAN port flags */
>  #define WWAN_PORT_TX_OFF       0
> +#define WWAN_PORT_OPEN         1
>
>  /**
>   * struct wwan_device - The structure that defines a WWAN device
> @@ -58,7 +59,6 @@ struct wwan_device {
>  /**
>   * struct wwan_port - The structure that defines a WWAN port
>   * @type: Port type
> - * @start_count: Port start counter
>   * @flags: Store port state and capabilities
>   * @ops: Pointer to WWAN port operations
>   * @ops_lock: Protect port ops
> @@ -70,7 +70,6 @@ struct wwan_device {
>   */
>  struct wwan_port {
>         enum wwan_port_type type;
> -       unsigned int start_count;
>         unsigned long flags;
>         const struct wwan_port_ops *ops;
>         struct mutex ops_lock; /* Serialize ops + protect against removal */
> @@ -496,7 +495,7 @@ void wwan_remove_port(struct wwan_port *port)
>         struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>
>         mutex_lock(&port->ops_lock);
> -       if (port->start_count)
> +       if (test_and_clear_bit(WWAN_PORT_OPEN, &port->flags))
>                 port->ops->stop(port);
>         port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */
>         mutex_unlock(&port->ops_lock);
> @@ -549,11 +548,14 @@ static int wwan_port_op_start(struct wwan_port *port)
>         }
>
>         /* If port is already started, don't start again */
> -       if (!port->start_count)
> -               ret = port->ops->start(port);
> +       if (test_bit(WWAN_PORT_OPEN, &port->flags)) {
> +               ret = -EBUSY;
> +               goto out_unlock;
> +       }
> +       ret = port->ops->start(port);
>
>         if (!ret)
> -               port->start_count++;
> +               set_bit(WWAN_PORT_OPEN, &port->flags);
>
>  out_unlock:
>         mutex_unlock(&port->ops_lock);
> @@ -564,8 +566,7 @@ static int wwan_port_op_start(struct wwan_port *port)
>  static void wwan_port_op_stop(struct wwan_port *port)
>  {
>         mutex_lock(&port->ops_lock);
> -       port->start_count--;
> -       if (!port->start_count) {
> +       if (test_and_clear_bit(WWAN_PORT_OPEN, &port->flags)) {
>                 if (port->ops)
>                         port->ops->stop(port);
>                 skb_queue_purge(&port->rxq);
> --
> 2.25.1
>