netdev - Re: [patch net-next 0/4] net: devlink: sync flash and dev info command

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]

Date:   Tue, 23 Aug 2022 21:59:48 +0530
From:   "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, idosch@...dia.com, pabeni@...hat.com,
        edumazet@...gle.com, saeedm@...dia.com, jacob.e.keller@...el.com,
        vikas.gupta@...adcom.com, gospo@...adcom.com,
        chandrashekar.devegowda@...el.com, soumya.prakash.mishra@...el.com,
        linuxwwan@...el.com
Subject: Re: [patch net-next 0/4] net: devlink: sync flash and dev info
 command

On 8/23/2022 5:50 PM, Jiri Pirko wrote:
> Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@...ux.intel.com wrote:
>> On 8/19/2022 1:55 PM, Jiri Pirko wrote:
>>> Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@...nel.org wrote:
>>>> On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote:
>>>>> Currently it is up to the driver what versions to expose and what flash
>>>>> update component names to accept. This is inconsistent. Thankfully, only
>>>>> netdevsim is currently using components, so it is a good time
>>>>> to sanitize this.
>>>>
>>>> Please take a look at recently merged code - 5417197dd516 ("Merge branch
>>>> 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions
>>>> there so I think you're gonna break them?
>>>
>>> Ah, crap. Too late :/ They are passing the string to FW (cmd is
>>> the component name here):
>>> static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
>>> {
>>>           char flash_command[T7XX_FB_COMMAND_SIZE];
>>>
>>>           snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
>>>           return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
>>> }
>>>
>>> This breaks the pairing with info.versions assumption. Any possibility
>>> to revert this and let them redo?
>>>
>>> Ccing m.chetan.kumar@...ux.intel.com, chandrashekar.devegowda@...el.com,
>>> soumya.prakash.mishra@...el.com
>>>
>>> Guys, could you expose one version for component you are flashing? We
>>> need 1:1 mapping here.
>>
>> Thanks for the heads-up.
>> I had a look at the patch & my understanding is driver is supposed
>> to expose flash update component name & version details via
>> devlink_info_version_running_put_ext().
> 
> Yes.
> 
>>
>> Is version value a must ? Internally version value is not used for making any
>> decision so in case driver/device doesn't support it should be ok to pass
>> empty string ?
> 
> No.
> 
>>
>> Ex:
>> devlink_info_version_running_put_ext(req, "fw", "",
>> DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>
>> One observation:-
>> While testing I noticed "flash_components:" is not getting displayed as
>> mentioned in cover note.
> 
> You need iproute2 patch for that which is still in my queue:
> https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683

Thanks. After applying this patch "flash_components" details are getting 
displayed.

Another observation is if NULL is passed for version_value there is a 
crash. Below is the backtrace.

3187.556637] BUG: kernel NULL pointer dereference, address: 0000000000000000
[ 3187.556659] #PF: supervisor read access in kernel mode
[ 3187.556666] #PF: error_code(0x0000) - not-present page
3187.556791] Call Trace:
[ 3187.556796]  <TASK>
[ 3187.556801]  ? devlink_info_version_put+0x112/0x1d0
[ 3187.556823]  ? __nla_put+0x20/0x30
[ 3187.556833]  devlink_info_version_running_put_ext+0x1c/0x30
[ 3187.556851]  t7xx_devlink_info_get+0x37/0x40 [mtk_t7xx]
[ 3187.556880]  devlink_nl_info_fill.constprop.0+0xa1/0x120
[ 3187.556892]  devlink_nl_cmd_info_get_dumpit+0xa8/0x140
[ 3187.556901]  netlink_dump+0x1a3/0x340
[ 3187.556913]  __netlink_dump_start+0x1d0/0x290

Is driver expected to set version number along with component name ?

mtk_t7xx WWAN driver is using the devlink interface for flashing the fw 
to WWAN device. If WWAN device is not capable of supporting the 
versioning for each component how should we handle ? Please suggest.

-- 
Chetan