Hello Moises,
On Tue, Apr 26, 2022 at 10:46 PM Veleta, Moises <moises.veleta@xxxxxxxxx> wrote:
> From: Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx>
> Sent: Monday, April 25, 2022 4:53 PM
> To: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx <netdev@xxxxxxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx <linux-wireless@xxxxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; David Miller <davem@xxxxxxxxxxxxx>; Johannes Berg <johannes@xxxxxxxxxxxxxxxx>; Loic Poulain <loic.poulain@xxxxxxxxxx>; Kumar, M Chetan <m.chetan.kumar@xxxxxxxxx>; Devegowda, Chandrashekar <chandrashekar.devegowda@xxxxxxxxx>; linuxwwan <linuxwwan@xxxxxxxxx>; chiranjeevi.rapolu@xxxxxxxxxxxxxxx <chiranjeevi.rapolu@xxxxxxxxxxxxxxx>; Liu, Haijun <haijun.liu@xxxxxxxxxxxx>; Hanania, Amir <amir.hanania@xxxxxxxxx>; Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Sharma, Dinesh <dinesh.sharma@xxxxxxxxx>; Lee, Eliot <eliot.lee@xxxxxxxxx>; Jarvinen, Ilpo Johannes <ilpo.johannes.jarvinen@xxxxxxxxx>; Veleta, Moises <moises.veleta@xxxxxxxxx>; Bossart, Pierre-louis <pierre-louis.bossart@xxxxxxxxx>; Sethuraman, Muralidharan <muralidharan.sethuraman@xxxxxxxxx>; Mishra, Soumya Prakash <soumya.prakash.mishra@xxxxxxxxx>; Kancharla, Sreehari <sreehari.kancharla@xxxxxxxxx>; Sahu, Madhusmita <madhusmita.sahu@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> Co-developed-by: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
>> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
>> Co-developed-by: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
>> Signed-off-by: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
>>
>> From a WWAN framework perspective:
>> Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxx>
>
> [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.
>
> [MV] We do not drop port ctrl messages. We keep them and try again later.
> Whereas WWAN skbs are dropped if conditions are met.
I missed that the WWAN port returns no error when it drops the skb.
And then I concluded that any failure to process the CCCI message
should be accomplished with the skb freeing. Now the handling of CCCI
messages is more clear to me. Thank you for the clarifications!
To avoid similar misinterpretation in the future, I thought that the
skb freeing in the WWAN port worth a comment. Something to describe
that despite dropping the message, the return code is zero, indicating
skb consumption. Similarly in this (t7xx_port_enqueue_skb) function.
Something like: "return an error here, the caller will try again
later". And maybe in t7xx_cldma_gpd_rx_from_q() near the loop break
after the .skb_recv() failure test. Something like: "break message
processing for now will try this message later".
What do you think?
>> + 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.
>
> [MV] Since t7xx_port_wwan_recv_skb returns 0 when dropping a skb, it skips this push operation and
> will not trigger the error stated. Inside of that function an error is printed.
>
>> + 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()?
>
> [MV] For t7xx_port_wwan_recv_skb, it drops skb and seq_nums is updated.
> for t7xx_port_enqueue_skb, it retains skb and can be processed again later, skipping this update upon error.
Now this is clear to me. Thanks.
>> + return 0;
>> +
>> +drop_skb:
>> + dev_kfree_skb_any(skb);
>> + return 0;
>> +}
--
Sergey
