Date:   Tue, 26 Apr 2022 02:53:54 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     Ricardo Martinez <ricardo.martinez@...ux.intel.com>
Cc:     netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Johannes Berg <johannes@...solutions.net>,
        Loic Poulain <loic.poulain@...aro.org>,
        M Chetan Kumar <m.chetan.kumar@...el.com>,
        chandrashekar.devegowda@...el.com,
        Intel Corporation <linuxwwan@...el.com>,
        chiranjeevi.rapolu@...ux.intel.com,
        Haijun Liu (刘海军) 
        <haijun.liu@...iatek.com>, amir.hanania@...el.com,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        dinesh.sharma@...el.com, eliot.lee@...el.com,
        ilpo.johannes.jarvinen@...el.com, moises.veleta@...el.com,
        pierre-louis.bossart@...el.com, muralidharan.sethuraman@...el.com,
        Soumya.Prakash.Mishra@...el.com, sreehari.kancharla@...el.com,
        madhusmita.sahu@...el.com
Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure

On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez
<ricardo.martinez@...ux.intel.com> wrote:
> Port-proxy provides a common interface to interact with different types
> of ports. Ports export their configuration via `struct t7xx_port` and
> operate as defined by `struct port_ops`.
>
> Signed-off-by: Haijun Liu <haijun.liu@...iatek.com>
> Co-developed-by: Chandrashekar Devegowda <chandrashekar.devegowda@...el.com>
> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@...el.com>
> Co-developed-by: Ricardo Martinez <ricardo.martinez@...ux.intel.com>
> Signed-off-by: Ricardo Martinez <ricardo.martinez@...ux.intel.com>
>
> From a WWAN framework perspective:
> Reviewed-by: Loic Poulain <loic.poulain@...aro.org>

[skipped]

> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c

[skipped]

> +static struct t7xx_port_conf t7xx_md_port_conf[] = {};

Please spell this definition in two lines (i.e. move closing brace
into next line):

+static struct t7xx_port_conf t7xx_md_port_conf[] = {
+};

Such spelling in this patch will help you avoid editing the line when
you add the first entry in the next (control port introducing) patch:

-static struct t7xx_port_conf t7xx_md_port_conf[] = {};
+static struct t7xx_port_conf t7xx_md_port_conf[] = {
+       {
+               ...
+               .name = "t7xx_ctrl",
+       },
+};

It will become as simple as:

 static struct t7xx_port_conf t7xx_md_port_conf[] = {
+       {
+               ...
+               .name = "t7xx_ctrl",
+       },
 };

BTW, if the t7xx_md_port_conf contents are not expected to change at
run-time, should this array, as well as all pointers to it, be const?

[skipped]

> +int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&port->rx_wq.lock, flags);
> +       if (port->rx_skb_list.qlen >= port->rx_length_th) {
> +               spin_unlock_irqrestore(&port->rx_wq.lock, flags);

Probably skb should be freed here before returning. The caller assumes
that skb will be consumed even in case of error.

> +               return -ENOBUFS;
> +       }
> +       __skb_queue_tail(&port->rx_skb_list, skb);
> +       spin_unlock_irqrestore(&port->rx_wq.lock, flags);
> +
> +       wake_up_all(&port->rx_wq);
> +       return 0;
> +}

[skipped]

> +static int t7xx_port_proxy_recv_skb(struct cldma_queue *queue, struct sk_buff *skb)
> +{
> +       struct ccci_header *ccci_h = (struct ccci_header *)skb->data;
> +       struct t7xx_pci_dev *t7xx_dev = queue->md_ctrl->t7xx_dev;
> +       struct t7xx_fsm_ctl *ctl = t7xx_dev->md->fsm_ctl;
> +       struct device *dev = queue->md_ctrl->dev;
> +       struct t7xx_port_conf *port_conf;
> +       struct t7xx_port *port;
> +       u16 seq_num, channel;
> +       int ret;
> +
> +       if (!skb)
> +               return -EINVAL;
> +
> +       channel = FIELD_GET(CCCI_H_CHN_FLD, le32_to_cpu(ccci_h->status));
> +       if (t7xx_fsm_get_md_state(ctl) == MD_STATE_INVALID) {
> +               dev_err_ratelimited(dev, "Packet drop on channel 0x%x, modem not ready\n", channel);
> +               goto drop_skb;
> +       }
> +
> +       port = t7xx_port_proxy_find_port(t7xx_dev, queue, channel);
> +       if (!port) {
> +               dev_err_ratelimited(dev, "Packet drop on channel 0x%x, port not found\n", channel);
> +               goto drop_skb;
> +       }
> +
> +       seq_num = t7xx_port_next_rx_seq_num(port, ccci_h);
> +       port_conf = port->port_conf;
> +       skb_pull(skb, sizeof(*ccci_h));
> +
> +       ret = port_conf->ops->recv_skb(port, skb);
> +       if (ret) {
> +               skb_push(skb, sizeof(*ccci_h));

Header can not be pushed back here, since the .recv_skb() callback
consumes (frees) skb even in case of error. See
t7xx_port_wwan_recv_skb() and my comment in t7xx_port_enqueue_skb().
Pushing the header back after failure will trigger the use-after-free
error.

> +               return ret;
> +       }
> +
> +       port->seq_nums[MTK_RX] = seq_num;

The expected sequence number updated only on successful .recv_skb()
exit. This will trigger the out-of-order seqno warning on a next
message after a .recv_skb() failure. Is this intentional behaviour?

Maybe the expected sequence number should be updated before the
.recv_skb() call? Or even the sequence number update should be moved
to t7xx_port_next_rx_seq_num()?

> +       return 0;
> +
> +drop_skb:
> +       dev_kfree_skb_any(skb);
> +       return 0;
> +}

--
Sergey

Powered by blists - more mailing lists