netdev - Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure

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>] [day] [month] [year] [list]
Date:   Wed, 27 Apr 2022 04:35:58 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     "Veleta, Moises" <moises.veleta@...el.com>
Cc:     Ricardo Martinez <ricardo.martinez@...ux.intel.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-wireless@...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>,
        "Kumar, M Chetan" <m.chetan.kumar@...el.com>,
        "Devegowda, Chandrashekar" <chandrashekar.devegowda@...el.com>,
        linuxwwan <linuxwwan@...el.com>,
        "chiranjeevi.rapolu@...ux.intel.com" 
        <chiranjeevi.rapolu@...ux.intel.com>,
        "Liu, Haijun" <haijun.liu@...iatek.com>,
        "Hanania, Amir" <amir.hanania@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Sharma, Dinesh" <dinesh.sharma@...el.com>,
        "Lee, Eliot" <eliot.lee@...el.com>,
        "Jarvinen, Ilpo Johannes" <ilpo.johannes.jarvinen@...el.com>,
        "Bossart, Pierre-louis" <pierre-louis.bossart@...el.com>,
        "Sethuraman, Muralidharan" <muralidharan.sethuraman@...el.com>,
        "Mishra, Soumya Prakash" <soumya.prakash.mishra@...el.com>,
        "Kancharla, Sreehari" <sreehari.kancharla@...el.com>,
        "Sahu, Madhusmita" <madhusmita.sahu@...el.com>
Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure

On Wed, Apr 27, 2022 at 4:14 AM Veleta, Moises <moises.veleta@...el.com> wrote:
> From: Sergey Ryazanov <ryazanov.s.a@...il.com>
> Sent: Tuesday, April 26, 2022 4:06 PM
> To: Veleta, Moises <moises.veleta@...el.com>
> Cc: Ricardo Martinez <ricardo.martinez@...ux.intel.com>; netdev@...r.kernel.org <netdev@...r.kernel.org>; linux-wireless@...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>; Kumar, M Chetan <m.chetan.kumar@...el.com>; Devegowda, Chandrashekar <chandrashekar.devegowda@...el.com>; linuxwwan <linuxwwan@...el.com>; chiranjeevi.rapolu@...ux.intel.com <chiranjeevi.rapolu@...ux.intel.com>; Liu, Haijun <haijun.liu@...iatek.com>; Hanania, Amir <amir.hanania@...el.com>; Andy Shevchenko <andriy.shevchenko@...ux.intel.com>; Sharma, Dinesh <dinesh.sharma@...el.com>; Lee, Eliot <eliot.lee@...el.com>; Jarvinen, Ilpo Johannes <ilpo.johannes.jarvinen@...el.com>; Bossart, Pierre-louis <pierre-louis.bossart@...el.com>; Sethuraman, Muralidharan <muralidharan.sethuraman@...el.com>; Mishra, Soumya Prakash <soumya.prakash.mishra@...el.com>; Kancharla, Sreehari <sreehari.kancharla@...el.com>; Sahu, Madhusmita <madhusmita.sahu@...el.com>
> Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure
>
> Hello Moises,
>
> On Tue, Apr 26, 2022 at 10:46 PM Veleta, Moises <moises.veleta@...el.com> wrote:
>> From: Sergey Ryazanov <ryazanov.s.a@...il.com>
>> Sent: Monday, April 25, 2022 4:53 PM
>> To: Ricardo Martinez <ricardo.martinez@...ux.intel.com>
>> Cc: netdev@...r.kernel.org <netdev@...r.kernel.org>; linux-wireless@...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>; Kumar, M Chetan <m.chetan.kumar@...el.com>; Devegowda, Chandrashekar <chandrashekar.devegowda@...el.com>; linuxwwan <linuxwwan@...el.com>; chiranjeevi.rapolu@...ux.intel.com <chiranjeevi.rapolu@...ux.intel.com>; Liu, Haijun <haijun.liu@...iatek.com>; Hanania, Amir <amir.hanania@...el.com>; Andy Shevchenko <andriy.shevchenko@...ux.intel.com>; Sharma, Dinesh <dinesh.sharma@...el.com>; Lee, Eliot <eliot.lee@...el.com>; Jarvinen, Ilpo Johannes <ilpo.johannes.jarvinen@...el.com>; Veleta, Moises <moises.veleta@...el.com>; Bossart, Pierre-louis <pierre-louis.bossart@...el.com>; Sethuraman, Muralidharan <muralidharan.sethuraman@...el.com>; Mishra, Soumya Prakash <soumya.prakash.mishra@...el.com>; Kancharla, Sreehari <sreehari.kancharla@...el.com>; Sahu, Madhusmita <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]
>>
>>> +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?
>
> Yes, that would remove the unintended obfuscation . Unless, you think this approach is inappropriate
> and should be refactored.  Otherwise, I will proceed with adding those clarifying comments.
> Thank you.

I am Ok with the current approach. It does not contain obvious errors.
It might look puzzled, but proper comments should solve this.

-- 
Sergey