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
| ||
|
Message-ID: <474077db-df42-6791-0253-74ca6e0d7b34@linux.intel.com> Date: Fri, 26 Aug 2022 14:24:52 +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, hua.yang@...iatek.com Subject: Re: [patch net-next 0/4] net: devlink: sync flash and dev info command 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 ? -- Chetan
Powered by blists - more mailing lists