netdev - Re: [PATCH net-next 2/2] ionic: add devlink firmware update

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: <20200903173029.GD2997@nanopsycho.orion>
Date:   Thu, 3 Sep 2020 19:30:29 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Shannon Nelson <snelson@...sando.io>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        moshe@...lanox.com
Subject: Re: [PATCH net-next 2/2] ionic: add devlink firmware update

Thu, Sep 03, 2020 at 05:58:42PM CEST, snelson@...sando.io wrote:
>On 9/2/20 11:01 PM, Jiri Pirko wrote:
>> Wed, Sep 02, 2020 at 09:57:17PM CEST, snelson@...sando.io wrote:
>> > Add support for firmware update through the devlink interface.
>> > This update copies the firmware object into the device, asks
>> > the current firmware to install it, then asks the firmware to
>> > set the device to use the new firmware on the next boot-up.
>> > 
>> > The install and activate steps are launched as asynchronous
>> > requests, which are then followed up with status requests
>> > commands.  These status request commands will be answered with
>> > an EAGAIN return value and will try again until the request
>> > has completed or reached the timeout specified.
>> > 
>> > Signed-off-by: Shannon Nelson <snelson@...sando.io>
>[...]
>> > +
>> > +	netdev_info(netdev, "Installing firmware %s\n", fw_name);
>> You don't need this dmesg messagel.
>> 
>> 
>> > +
>> > +	dl = priv_to_devlink(ionic);
>> > +	devlink_flash_update_begin_notify(dl);
>> > +	devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0, 0);
>> > +
>[...]
>> > +		if (err) {
>> > +			netdev_err(netdev,
>> > +				   "download failed offset 0x%x addr 0x%lx len 0x%x\n",
>> > +				   offset, offsetof(union ionic_dev_cmd_regs, data),
>> > +				   copy_sz);
>> And this one.
>> 
>> 
>> > +			NL_SET_ERR_MSG_MOD(extack, "Segment download failed");
>> > +			goto err_out;
>> > +		}
>[...]
>> > +	devlink_flash_update_status_notify(dl, "Activating", NULL, 2, 2);
>> > +
>> > +	netdev_info(netdev, "Firmware update completed\n");
>> And this one.
>> 
>> 
>> > +
>> > +err_out:
>> > +	if (err)
>> > +		devlink_flash_update_status_notify(dl, "Flash failed", NULL, 0, 0);
>> > +	release_firmware(fw);
>> > +	devlink_flash_update_end_notify(dl);
>> > +	return err;
>> > +}
>> > 
>
>True, they aren't "needed" for operational purposes, but they are rather
>useful when inspecting a system after getting a report of bad behavior, and

I don't think it is nice to pollute dmesg with any arbitrary driver-specific
messages.


>since this should be seldom performed there should be no risk of filling the
>log.  As far as I can tell, the devlink messages are only seen at the time
>the flash is performed as output from the flash command, or from a devlink
>monitor if someone started it before the flash operation.  Is there any other
>place that can be inspected later that will indicate someone was fussing with
>the firmware?

Not really, no. But perhaps we can have a counter for that. Similar to
what Jakub suggested for reload.