netdev - Re: [RFC PATCH net-next v2 5/6] ice: implement device flash update via devlink

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]

Message-ID: <45bd606f-e0bf-de8b-7e1f-5bd01aa07618@intel.com>
Date:   Thu, 23 Jul 2020 16:33:12 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     Jakub Kicinski <kubakici@...pl>, Jiri Pirko <jiri@...nulli.us>,
        Tom Herbert <tom@...bertland.com>
Subject: Re: [RFC PATCH net-next v2 5/6] ice: implement device flash update
 via devlink

On 7/17/2020 11:35 AM, Jacob Keller wrote:

> +	devlink_flash_update_begin_notify(devlink);
> +	devlink_flash_update_status_notify(devlink, "Preparing to flash",
> +					   component, 0, 0);
> +	err = ice_flash_pldm_image(pf, fw, extack);
> +	devlink_flash_update_end_notify(devlink);
> +
> +	release_firmware(fw);
> +
> +	return err;
> +}
> +

Hi Jakub, Jiri,

I noticed something interesting recently in regards to the
devlink_flash_update_end_notify.

The way that iproute2/devlink works is that if
DEVLINK_CMD_FLASH_UPDATE_END is sent, then it will stop waiting for the
error response from DEVLINK_CMD_FLASH_UPDATE.

This means that if a driver sends DEVLINK_CMD_FLASH_UPDATE_END on a
failed update, the devlink program doesn't report the error or the
netlink status message.

Is it expected to send DEVLINK_CMD_FLASH_UPDATE_END on failures? All
current drivers appear to do so.

Perhaps its a bug in the devlink application on not waiting for the
flash command to properly complete?

Would it make sense to extend DEVLINK_FLASH_UPDATE_END to include an
attribute which specified the error code? But then would that include
the netlink extended status message?

It's not a huge deal but it seemed weird that if we detect any errors
during the main flash update process we will not properly report them.

I wasn't quite sure where the bug actually lies, so help here is
appreciated on figuring out what the best fix is.

Regards,
Jake