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] [day] [month] [year] [list]
Date:   Fri, 26 Aug 2022 13:01:03 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
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, hua.yang@...iatek.com
Subject: Re: [patch net-next 0/4] net: devlink: sync flash and dev info
 command

Fri, Aug 26, 2022 at 10:54:52AM CEST, m.chetan.kumar@...ux.intel.com wrote:
>Looped hua.yang@...iatek.com to email.
>
>On 8/24/2022 2:17 PM, Jiri Pirko wrote:
>> Tue, Aug 23, 2022 at 06:29:48PM CEST, m.chetan.kumar@...ux.intel.com wrote:
>> > 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.
>> 
>> So don't pass NULL :)
>> 
>> 
>> > 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 ?
>> 
>> Of course.
>> 
>> 
>> > 
>> > 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.
>> 
>> The user should have a visibility about what version is currently
>> stored/running in the device. You should expose it.
>
>If the only intention of this component version is to give a visbility to
>user, the WWAN Driver exposes the AT & MBIM control ports. Applications like
>Modem Manager uses AT/MBIM commands to obtain fw version info.
>
>So would it be ok to keep component version as an optional for WWAN drivers ?

Nope. It is mandatory now for the devlink flash component name to match
version of component reported. If you cannot do that, there is
most likely something wrong with your design

Powered by blists - more mailing lists