| Message ID | 20240122084530.32451-10-danieller@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add ability to flash modules' firmware | expand |
On Mon, Jan 22, 2024 at 10:45:30AM +0200, Danielle Ratson wrote: ... > +static int module_flash_fw(struct net_device *dev, struct nlattr **tb, > + struct netlink_ext_ack *extack) > +{ > + struct ethtool_module_fw_flash_params params = {}; > + struct nlattr *attr; > + > + if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) { > + NL_SET_ERR_MSG_ATTR(extack, > + tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME], > + "File name attribute is missing"); > + return -EINVAL; > + } > + > + params.file_name = > + nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]); > + > + attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD]; > + if (attr) { > + params.password = cpu_to_be32(nla_get_u32(attr)); Hi Danielle, The type of password is u32, so perhaps cpu_to_be32() isn't needed here? Flagged by Sparse. > + params.password_valid = true; > + } > + > + return module_flash_fw_schedule(dev, ¶ms, extack); > +} ...
> -----Original Message----- > From: Simon Horman <horms@kernel.org> > Sent: Monday, 22 January 2024 12:38 > To: Danielle Ratson <danieller@nvidia.com> > Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; corbet@lwn.net; > linux@armlinux.org.uk; sdf@google.com; kory.maincent@bootlin.com; > maxime.chevallier@bootlin.com; vladimir.oltean@nxp.com; > przemyslaw.kitszel@intel.com; ahmed.zaki@intel.com; > richardcochran@gmail.com; shayagr@amazon.com; > paul.greenwalt@intel.com; jiri@resnulli.us; linux-doc@vger.kernel.org; linux- > kernel@vger.kernel.org; mlxsw <mlxsw@nvidia.com>; Petr Machata > <petrm@nvidia.com>; Ido Schimmel <idosch@nvidia.com> > Subject: Re: [RFC PATCH net-next 9/9] ethtool: Add ability to flash transceiver > modules' firmware > > On Mon, Jan 22, 2024 at 10:45:30AM +0200, Danielle Ratson wrote: > > ... > > > +static int module_flash_fw(struct net_device *dev, struct nlattr **tb, > > + struct netlink_ext_ack *extack) { > > + struct ethtool_module_fw_flash_params params = {}; > > + struct nlattr *attr; > > + > > + if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) { > > + NL_SET_ERR_MSG_ATTR(extack, > > + > tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME], > > + "File name attribute is missing"); > > + return -EINVAL; > > + } > > + > > + params.file_name = > > + nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]); > > + > > + attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD]; > > + if (attr) { > > + params.password = cpu_to_be32(nla_get_u32(attr)); > > Hi Danielle, > > The type of password is u32, so perhaps cpu_to_be32() isn't needed here? > > Flagged by Sparse. Hi Simon, The cpu_to_be32() is actually needed here, without it the password parameter from user space is passed with wrong endianness. Thanks, Danielle > > > + params.password_valid = true; > > + } > > + > > + return module_flash_fw_schedule(dev, ¶ms, extack); } > > ...
On Mon, 22 Jan 2024 10:45:30 +0200 Danielle Ratson wrote: > #include <linux/ethtool.h> > +#include <linux/sfp.h> > +#include <linux/firmware.h> alphabetical order, please > +static int > +module_flash_fw_schedule(struct net_device *dev, > + struct ethtool_module_fw_flash_params *params, > + struct netlink_ext_ack *extack) > +{ > + const struct ethtool_ops *ops = dev->ethtool_ops; > + struct ethtool_module_fw_flash *module_fw; > + int err; > + > + if (!ops->set_module_eeprom_by_page || > + !ops->get_module_eeprom_by_page) { > + NL_SET_ERR_MSG(extack, > + "Flashing module firmware is not supported by this device"); > + return -EOPNOTSUPP; > + } > + > + if (dev->module_fw_flash_in_progress) { > + NL_SET_ERR_MSG(extack, "Module firmware flashing already in progress"); > + return -EBUSY; > + } > + > + module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL); > + if (!module_fw) > + return -ENOMEM; > + > + module_fw->params = *params; > + err = request_firmware(&module_fw->fw, module_fw->params.file_name, request_firmware_direct() ? I think udev timeout is 30 sec and we're holding rtnl_lock.. I don't remember why we didn't use that in devlink > + &dev->dev); > + if (err) { > + NL_SET_ERR_MSG(extack, > + "Failed to request module firmware image"); > + goto err_request_firmware; > + } > + > + err = module_flash_fw_work_init(module_fw, dev, extack); > + if (err < 0) { > + NL_SET_ERR_MSG(extack, > + "Flashing module firmware is not supported by this device"); > + goto err_work_init; > + } > + > + dev->module_fw_flash_in_progress = true; What does this protect us from? > +static int module_flash_fw(struct net_device *dev, struct nlattr **tb, > + struct netlink_ext_ack *extack) > +{ > + struct ethtool_module_fw_flash_params params = {}; > + struct nlattr *attr; > + > + if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) { > + NL_SET_ERR_MSG_ATTR(extack, GENL_REQ_ATTR_CHECK, and you can check it in the caller, before taking rtnl_lock. > + tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME], > + "File name attribute is missing"); > + return -EINVAL; > + } > + > + params.file_name = > + nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]); Hm. I think you copy the param struct by value to the work container. nla_data() is in the skb which is going to get freed after _ACT returns. So if anyone tries to access the name from the work it's going to UAF? > + > + attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD]; > + if (attr) { > + params.password = cpu_to_be32(nla_get_u32(attr)); > + params.password_valid = true; > + } > + > + return module_flash_fw_schedule(dev, ¶ms, extack); > +}
> > #include <linux/ethtool.h> > > +#include <linux/sfp.h> > > +#include <linux/firmware.h> > > alphabetical order, please Ok. > > > +static int > > +module_flash_fw_schedule(struct net_device *dev, > > + struct ethtool_module_fw_flash_params *params, > > + struct netlink_ext_ack *extack) > > +{ > > + const struct ethtool_ops *ops = dev->ethtool_ops; > > + struct ethtool_module_fw_flash *module_fw; > > + int err; > > + > > + if (!ops->set_module_eeprom_by_page || > > + !ops->get_module_eeprom_by_page) { > > + NL_SET_ERR_MSG(extack, > > + "Flashing module firmware is not supported by > this device"); > > + return -EOPNOTSUPP; > > + } > > + > > + if (dev->module_fw_flash_in_progress) { > > + NL_SET_ERR_MSG(extack, "Module firmware flashing already > in progress"); > > + return -EBUSY; > > + } > > + > > + module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL); > > + if (!module_fw) > > + return -ENOMEM; > > + > > + module_fw->params = *params; > > + err = request_firmware(&module_fw->fw, module_fw- > >params.file_name, > > request_firmware_direct() ? I think udev timeout is 30 sec and we're holding > rtnl_lock.. I don't remember why we didn't use that in devlink Ok will change. > > > + &dev->dev); > > + if (err) { > > + NL_SET_ERR_MSG(extack, > > + "Failed to request module firmware image"); > > + goto err_request_firmware; > > + } > > + > > + err = module_flash_fw_work_init(module_fw, dev, extack); > > + if (err < 0) { > > + NL_SET_ERR_MSG(extack, > > + "Flashing module firmware is not supported by > this device"); > > + goto err_work_init; > > + } > > + > > + dev->module_fw_flash_in_progress = true; > > What does this protect us from? Currently, it protects us from flashing an in-progress-flashing-module. > > > +static int module_flash_fw(struct net_device *dev, struct nlattr **tb, > > + struct netlink_ext_ack *extack) { > > + struct ethtool_module_fw_flash_params params = {}; > > + struct nlattr *attr; > > + > > + if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) { > > + NL_SET_ERR_MSG_ATTR(extack, > > GENL_REQ_ATTR_CHECK, and you can check it in the caller, before taking > rtnl_lock. > OK, np. The idea was to have module_flash_fw() that checks the attrs and extract them into params and ethnl_act_module_fw_flash() should be free from those checks. But if so, maybe this separation is redundant and should combine the two? > > + > tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME], > > + "File name attribute is missing"); > > + return -EINVAL; > > + } > > + > > + params.file_name = > > + nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]); > > Hm. I think you copy the param struct by value to the work container. > nla_data() is in the skb which is going to get freed after _ACT returns. > So if anyone tries to access the name from the work it's going to UAF? The file_name parameter is not really needed inside the work. Once we called request_firmware_direct(), we have all that we need in module_fw->fw. Do we still need to avoid that situation? If so, can you please suggest how? > > > + > > + attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD]; > > + if (attr) { > > + params.password = cpu_to_be32(nla_get_u32(attr)); > > + params.password_valid = true; > > + } > > + > > + return module_flash_fw_schedule(dev, ¶ms, extack); } >
On Tue, 23 Jan 2024 13:05:16 +0000 Danielle Ratson wrote: > > GENL_REQ_ATTR_CHECK, and you can check it in the caller, before taking > > rtnl_lock. > > > > OK, np. The idea was to have module_flash_fw() that checks the attrs > and extract them into params and ethnl_act_module_fw_flash() should > be free from those checks. But if so, maybe this separation is > redundant and should combine the two? No strong preference, whatever looks better :) To use GENL_REQ_ATTR_CHECK() I think you'll need to pass genl_info here. You can either to that or move the validation. > > > + > > tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME], > > > + "File name attribute is missing"); > > > + return -EINVAL; > > > + } > > > + > > > + params.file_name = > > > + nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]); > > > > Hm. I think you copy the param struct by value to the work container. > > nla_data() is in the skb which is going to get freed after _ACT returns. > > So if anyone tries to access the name from the work it's going to UAF? > > The file_name parameter is not really needed inside the work. Once we > called request_firmware_direct(), we have all that we need in > module_fw->fw. Do we still need to avoid that situation? If so, can > you please suggest how? I'd pass it to module_flash_fw_schedule() as a separate argument, if it doesn't have to be saved.
On Mon, Jan 22, 2024 at 10:45:30AM +0200, Danielle Ratson wrote: > +#define MODULE_EEPROM_PAGE 0 > +#define MODULE_EEPROM_OFFSET 0 > +#define MODULE_EEPROM_LENGTH 1 > +#define MODULE_EEPROM_I2C_ADDR 0x50 > + > +static int module_flash_fw_work_init(struct ethtool_module_fw_flash *module_fw, > + struct net_device *dev, > + struct netlink_ext_ack *extack) > +{ > + const struct ethtool_ops *ops = dev->ethtool_ops; > + struct ethtool_module_eeprom page_data = {}; > + struct module_sff8024_id_rpl *rpl; > + int err; > + > + /* Fetch the SFF-8024 Identifier Value. For all supported standards, it > + * is located at I2C address 0x50, byte 0. See section 4.1 in SFF-8024, > + * revision 4.9. > + */ > + page_data.page = MODULE_EEPROM_PAGE; > + page_data.offset = MODULE_EEPROM_OFFSET; > + page_data.length = MODULE_EEPROM_LENGTH; > + page_data.i2c_address = MODULE_EEPROM_I2C_ADDR; Please use better names - these aren't any better than using integers. Maybe use SFP_PHYS_ID for the offset? > + page_data.data = kmalloc(page_data.length, GFP_KERNEL); > + if (!page_data.data) > + return -ENOMEM; > + > + err = ops->get_module_eeprom_by_page(dev, &page_data, extack); > + if (err < 0) > + goto out; > + > + rpl = (struct module_sff8024_id_rpl *)page_data.data; What purpose does this structure of a single byte serve? To me, it just obfuscates the code. u8 phys_id; ... page_data.offset = SFP_PHYS_ID; page_data.length = sizeof(phys_id); page_data.data = &phys_id; ... switch (phys_id) { will work just as well, and be more explicit about what's actually going on here. It doesn't mean that I have to understand what this new module_sff8024_id_rpl structure is. I can see that we're just getting one byte which is the module physical ID. You also then don't need to care about kfree()ing one byte of data structure.
> > +#define MODULE_EEPROM_PAGE 0 > > +#define MODULE_EEPROM_OFFSET 0 > > +#define MODULE_EEPROM_LENGTH 1 > > +#define MODULE_EEPROM_I2C_ADDR 0x50 > > + > > +static int module_flash_fw_work_init(struct ethtool_module_fw_flash > *module_fw, > > + struct net_device *dev, > > + struct netlink_ext_ack *extack) { > > + const struct ethtool_ops *ops = dev->ethtool_ops; > > + struct ethtool_module_eeprom page_data = {}; > > + struct module_sff8024_id_rpl *rpl; > > + int err; > > + > > + /* Fetch the SFF-8024 Identifier Value. For all supported standards, it > > + * is located at I2C address 0x50, byte 0. See section 4.1 in SFF-8024, > > + * revision 4.9. > > + */ > > + page_data.page = MODULE_EEPROM_PAGE; > > + page_data.offset = MODULE_EEPROM_OFFSET; > > + page_data.length = MODULE_EEPROM_LENGTH; > > + page_data.i2c_address = MODULE_EEPROM_I2C_ADDR; > > Please use better names - these aren't any better than using integers. > > Maybe use SFP_PHYS_ID for the offset? > > > + page_data.data = kmalloc(page_data.length, GFP_KERNEL); > > + if (!page_data.data) > > + return -ENOMEM; > > + > > + err = ops->get_module_eeprom_by_page(dev, &page_data, extack); > > + if (err < 0) > > + goto out; > > + > > + rpl = (struct module_sff8024_id_rpl *)page_data.data; > > What purpose does this structure of a single byte serve? To me, it just > obfuscates the code. > > u8 phys_id; > > ... > page_data.offset = SFP_PHYS_ID; > page_data.length = sizeof(phys_id); > page_data.data = &phys_id; > ... > switch (phys_id) { > > will work just as well, and be more explicit about what's actually going on > here. It doesn't mean that I have to understand what this new > module_sff8024_id_rpl structure is. I can see that we're just getting one byte > which is the module physical ID. > > You also then don't need to care about kfree()ing one byte of data structure. OK, will change, thanks! > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> > > GENL_REQ_ATTR_CHECK, and you can check it in the caller, before > > > taking rtnl_lock. > > > > > > > OK, np. The idea was to have module_flash_fw() that checks the attrs > > and extract them into params and ethnl_act_module_fw_flash() should be > > free from those checks. But if so, maybe this separation is redundant > > and should combine the two? > > No strong preference, whatever looks better :) To use > GENL_REQ_ATTR_CHECK() I think you'll need to pass genl_info here. > You can either to that or move the validation. > > > > > + > > > tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME], > > > > + "File name attribute is missing"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + params.file_name = > > > > + nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]); > > > > > > Hm. I think you copy the param struct by value to the work container. > > > nla_data() is in the skb which is going to get freed after _ACT returns. > > > So if anyone tries to access the name from the work it's going to UAF? > > > > The file_name parameter is not really needed inside the work. Once we > > called request_firmware_direct(), we have all that we need in > > module_fw->fw. Do we still need to avoid that situation? If so, can > > you please suggest how? > > I'd pass it to module_flash_fw_schedule() as a separate argument, if it doesn't > have to be saved. It doesn’t make the module_fw->file_name attribute redundant?
On Wed, 24 Jan 2024 15:46:55 +0000 Danielle Ratson wrote: > > > The file_name parameter is not really needed inside the work. Once we > > > called request_firmware_direct(), we have all that we need in > > > module_fw->fw. Do we still need to avoid that situation? If so, can > > > you please suggest how? > > > > I'd pass it to module_flash_fw_schedule() as a separate argument, if it doesn't > > have to be saved. > > It doesn’t make the module_fw->file_name attribute redundant? This is what I mean: diff --git a/net/ethtool/module.c b/net/ethtool/module.c index 69cedb3ede6d..ea048a7089d9 100644 --- a/net/ethtool/module.c +++ b/net/ethtool/module.c @@ -229,7 +229,7 @@ static int module_flash_fw_work_init(struct ethtool_module_fw_flash *module_fw, } static int -module_flash_fw_schedule(struct net_device *dev, +module_flash_fw_schedule(struct net_device *dev, const char *file_name, struct ethtool_module_fw_flash_params *params, struct netlink_ext_ack *extack) { @@ -254,8 +254,7 @@ module_flash_fw_schedule(struct net_device *dev, return -ENOMEM; module_fw->params = *params; - err = request_firmware(&module_fw->fw, module_fw->params.file_name, - &dev->dev); + err = request_firmware(&module_fw->fw, file_name, &dev->dev); if (err) { NL_SET_ERR_MSG(extack, "Failed to request module firmware image"); @@ -288,6 +287,7 @@ static int module_flash_fw(struct net_device *dev, struct nlattr **tb, struct netlink_ext_ack *extack) { struct ethtool_module_fw_flash_params params = {}; + const char *file_name; struct nlattr *attr; if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) { @@ -297,8 +297,7 @@ static int module_flash_fw(struct net_device *dev, struct nlattr **tb, return -EINVAL; } - params.file_name = - nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]); + file_name = nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]); attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD]; if (attr) { @@ -306,7 +305,7 @@ static int module_flash_fw(struct net_device *dev, struct nlattr **tb, params.password_valid = true; } - return module_flash_fw_schedule(dev, ¶ms, extack); + return module_flash_fw_schedule(dev, file_name, ¶ms, extack); } int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info) diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h index 969de116f995..888f082f8daf 100644 --- a/net/ethtool/module_fw.h +++ b/net/ethtool/module_fw.h @@ -13,12 +13,10 @@ void ethtool_cmis_fw_update(struct work_struct *work); /** * struct ethtool_module_fw_flash_params - module firmware flashing parameters - * @file_name: Firmware image file name. * @password: Module password. Only valid when @pass_valid is set. * @password_valid: Whether the module password is valid or not. */ struct ethtool_module_fw_flash_params { - const char *file_name; u32 password; u8 password_valid:1; };
> +static int > +module_flash_fw_schedule(struct net_device *dev, > + struct ethtool_module_fw_flash_params *params, > + struct netlink_ext_ack *extack) > +{ > + const struct ethtool_ops *ops = dev->ethtool_ops; > + struct ethtool_module_fw_flash *module_fw; > + int err; > + > + if (!ops->set_module_eeprom_by_page || > + !ops->get_module_eeprom_by_page) { > + NL_SET_ERR_MSG(extack, > + "Flashing module firmware is not supported by this device"); > + return -EOPNOTSUPP; > + } > + > + if (dev->module_fw_flash_in_progress) { > + NL_SET_ERR_MSG(extack, "Module firmware flashing already in progress"); > + return -EBUSY; > + } > + > + module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL); > + if (!module_fw) > + return -ENOMEM; > + > + module_fw->params = *params; > + err = request_firmware(&module_fw->fw, module_fw->params.file_name, > + &dev->dev); How big are these firmware blobs? Ideally we want to be able to use the same API to upgrade things like GPON modules, which often run an openwrt image, and they are plugged into a cable modem which does not have too much RAM. Given that the interface to the EEPROM is using 128 byte 1/2 pages, would it be possible to use request_partial_firmware_into_buf() to read it on demand, rather than all at once? Andrew
> Subject: Re: [RFC PATCH net-next 9/9] ethtool: Add ability to flash transceiver > modules' firmware > > > +static int > > +module_flash_fw_schedule(struct net_device *dev, > > + struct ethtool_module_fw_flash_params *params, > > + struct netlink_ext_ack *extack) > > +{ > > + const struct ethtool_ops *ops = dev->ethtool_ops; > > + struct ethtool_module_fw_flash *module_fw; > > + int err; > > + > > + if (!ops->set_module_eeprom_by_page || > > + !ops->get_module_eeprom_by_page) { > > + NL_SET_ERR_MSG(extack, > > + "Flashing module firmware is not supported by > this device"); > > + return -EOPNOTSUPP; > > + } > > + > > + if (dev->module_fw_flash_in_progress) { > > + NL_SET_ERR_MSG(extack, "Module firmware flashing already > in progress"); > > + return -EBUSY; > > + } > > + > > + module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL); > > + if (!module_fw) > > + return -ENOMEM; > > + > > + module_fw->params = *params; > > + err = request_firmware(&module_fw->fw, module_fw- > >params.file_name, > > + &dev->dev); > > How big are these firmware blobs? > > Ideally we want to be able to use the same API to upgrade things like GPON > modules, which often run an openwrt image, and they are plugged into a cable > modem which does not have too much RAM. > > Given that the interface to the EEPROM is using 128 byte 1/2 pages, would it > be possible to use request_partial_firmware_into_buf() to read it on demand, > rather than all at once? > > Andrew OK, Ill handle that in the actual version. Thanks.
> > Subject: Re: [RFC PATCH net-next 9/9] ethtool: Add ability to flash > > transceiver modules' firmware > > > > > +static int > > > +module_flash_fw_schedule(struct net_device *dev, > > > + struct ethtool_module_fw_flash_params *params, > > > + struct netlink_ext_ack *extack) { > > > + const struct ethtool_ops *ops = dev->ethtool_ops; > > > + struct ethtool_module_fw_flash *module_fw; > > > + int err; > > > + > > > + if (!ops->set_module_eeprom_by_page || > > > + !ops->get_module_eeprom_by_page) { > > > + NL_SET_ERR_MSG(extack, > > > + "Flashing module firmware is not supported by > > this device"); > > > + return -EOPNOTSUPP; > > > + } > > > + > > > + if (dev->module_fw_flash_in_progress) { > > > + NL_SET_ERR_MSG(extack, "Module firmware flashing already > > in progress"); > > > + return -EBUSY; > > > + } > > > + > > > + module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL); > > > + if (!module_fw) > > > + return -ENOMEM; > > > + > > > + module_fw->params = *params; > > > + err = request_firmware(&module_fw->fw, module_fw- > > >params.file_name, > > > + &dev->dev); > > > > How big are these firmware blobs? > > The largest file I came across is 400K. > > Ideally we want to be able to use the same API to upgrade things like > > GPON modules, which often run an openwrt image, and they are plugged > > into a cable modem which does not have too much RAM. > > > > Given that the interface to the EEPROM is using 128 byte 1/2 pages, > > would it be possible to use request_partial_firmware_into_buf() to > > read it on demand, rather than all at once? > > > > Andrew > > OK, Ill handle that in the actual version. > Thanks.
> > > How big are these firmware blobs? > > > > > The largest file I came across is 400K. https://hack-gpon.org/ont-fs-com-gpon-onu-stick-with-mac/ Suggests that some GPON devices have 16MB of flash. Ideally we don't want to map a 16MB firmware image into the kernel address space. A high end switch could do it, but a typical OpenWRT 'cable modem' is likely to have trouble. Andrew
> -----Original Message----- > From: Danielle Ratson > Sent: Wednesday, 31 January 2024 17:53 > To: Andrew Lunn <andrew@lunn.ch> > Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; corbet@lwn.net; > linux@armlinux.org.uk; sdf@google.com; kory.maincent@bootlin.com; > maxime.chevallier@bootlin.com; vladimir.oltean@nxp.com; > przemyslaw.kitszel@intel.com; ahmed.zaki@intel.com; > richardcochran@gmail.com; shayagr@amazon.com; > paul.greenwalt@intel.com; jiri@resnulli.us; linux-doc@vger.kernel.org; linux- > kernel@vger.kernel.org; mlxsw <mlxsw@nvidia.com>; Petr Machata > <petrm@nvidia.com>; Ido Schimmel <idosch@nvidia.com> > Subject: RE: [RFC PATCH net-next 9/9] ethtool: Add ability to flash transceiver > modules' firmware > > > > Subject: Re: [RFC PATCH net-next 9/9] ethtool: Add ability to flash > > > transceiver modules' firmware > > > > > > > +static int > > > > +module_flash_fw_schedule(struct net_device *dev, > > > > + struct ethtool_module_fw_flash_params *params, > > > > + struct netlink_ext_ack *extack) { > > > > + const struct ethtool_ops *ops = dev->ethtool_ops; > > > > + struct ethtool_module_fw_flash *module_fw; > > > > + int err; > > > > + > > > > + if (!ops->set_module_eeprom_by_page || > > > > + !ops->get_module_eeprom_by_page) { > > > > + NL_SET_ERR_MSG(extack, > > > > + "Flashing module firmware is not supported by > > > this device"); > > > > + return -EOPNOTSUPP; > > > > + } > > > > + > > > > + if (dev->module_fw_flash_in_progress) { > > > > + NL_SET_ERR_MSG(extack, "Module firmware flashing already > > > in progress"); > > > > + return -EBUSY; > > > > + } > > > > + > > > > + module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL); > > > > + if (!module_fw) > > > > + return -ENOMEM; > > > > + > > > > + module_fw->params = *params; > > > > + err = request_firmware(&module_fw->fw, module_fw- > > > >params.file_name, > > > > + &dev->dev); > > > > > > How big are these firmware blobs? > > > > > The largest file I came across is 400K. > > > > Ideally we want to be able to use the same API to upgrade things > > > like GPON modules, which often run an openwrt image, and they are > > > plugged into a cable modem which does not have too much RAM. > > > > > > Given that the interface to the EEPROM is using 128 byte 1/2 pages, > > > would it be possible to use request_partial_firmware_into_buf() to > > > read it on demand, rather than all at once? > > > > > > Andrew > > > > OK, Ill handle that in the actual version. > > Thanks. Hi Andrew, Thanks again for your feedback. I thought again about you comment, and this patchset adds support for flashing CMIS compliant modules only. Later on, if it will be expanded to more modules, it will be more reasonable to add support like you have suggested to fit the new supported standard. So, currently I think it is better to not add it to this specific patchset. Thanks, Danielle
> Thanks again for your feedback. > I thought again about you comment, and this patchset adds support for flashing CMIS compliant modules only. > Later on, if it will be expanded to more modules, it will be more reasonable to add support like you have suggested to fit the new supported standard. > So, currently I think it is better to not add it to this specific patchset. O.K. It should not be a big change, and i doubt it has any major performance impacts. I2C is not very fast, so we can probably get it off the disk faster than it can be written to the device. Andrew
diff --git a/net/ethtool/module.c b/net/ethtool/module.c index 09cf11564840..69cedb3ede6d 100644 --- a/net/ethtool/module.c +++ b/net/ethtool/module.c @@ -1,6 +1,8 @@ // SPDX-License-Identifier: GPL-2.0-only #include <linux/ethtool.h> +#include <linux/sfp.h> +#include <linux/firmware.h> #include "netlink.h" #include "common.h" @@ -160,6 +162,183 @@ const struct ethnl_request_ops ethnl_module_request_ops = { .set_ntf_cmd = ETHTOOL_MSG_MODULE_NTF, }; +/* MODULE_FW_FLASH_ACT */ + +const struct nla_policy +ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD + 1] = { + [ETHTOOL_A_MODULE_FW_FLASH_HEADER] = + NLA_POLICY_NESTED(ethnl_header_policy), + [ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME] = { .type = NLA_NUL_STRING }, + [ETHTOOL_A_MODULE_FW_FLASH_PASSWORD] = { .type = NLA_U32 }, +}; + +struct module_sff8024_id_rpl { + u8 id; +}; + +#define MODULE_EEPROM_PAGE 0 +#define MODULE_EEPROM_OFFSET 0 +#define MODULE_EEPROM_LENGTH 1 +#define MODULE_EEPROM_I2C_ADDR 0x50 + +static int module_flash_fw_work_init(struct ethtool_module_fw_flash *module_fw, + struct net_device *dev, + struct netlink_ext_ack *extack) +{ + const struct ethtool_ops *ops = dev->ethtool_ops; + struct ethtool_module_eeprom page_data = {}; + struct module_sff8024_id_rpl *rpl; + int err; + + /* Fetch the SFF-8024 Identifier Value. For all supported standards, it + * is located at I2C address 0x50, byte 0. See section 4.1 in SFF-8024, + * revision 4.9. + */ + page_data.page = MODULE_EEPROM_PAGE; + page_data.offset = MODULE_EEPROM_OFFSET; + page_data.length = MODULE_EEPROM_LENGTH; + page_data.i2c_address = MODULE_EEPROM_I2C_ADDR; + page_data.data = kmalloc(page_data.length, GFP_KERNEL); + if (!page_data.data) + return -ENOMEM; + + err = ops->get_module_eeprom_by_page(dev, &page_data, extack); + if (err < 0) + goto out; + + rpl = (struct module_sff8024_id_rpl *)page_data.data; + switch (rpl->id) { + case SFF8024_ID_QSFP_DD: + case SFF8024_ID_OSFP: + case SFF8024_ID_DSFP: + case SFF8024_ID_QSFP_PLUS_CMIS: + case SFF8024_ID_SFP_DD_CMIS: + case SFF8024_ID_SFP_PLUS_CMIS: + INIT_WORK(&module_fw->work, ethtool_cmis_fw_update); + goto out; + default: + NL_SET_ERR_MSG(extack, + "Module type does not support firmware flashing"); + err = -EOPNOTSUPP; + goto out; + } + +out: + kfree(page_data.data); + return err; +} + +static int +module_flash_fw_schedule(struct net_device *dev, + struct ethtool_module_fw_flash_params *params, + struct netlink_ext_ack *extack) +{ + const struct ethtool_ops *ops = dev->ethtool_ops; + struct ethtool_module_fw_flash *module_fw; + int err; + + if (!ops->set_module_eeprom_by_page || + !ops->get_module_eeprom_by_page) { + NL_SET_ERR_MSG(extack, + "Flashing module firmware is not supported by this device"); + return -EOPNOTSUPP; + } + + if (dev->module_fw_flash_in_progress) { + NL_SET_ERR_MSG(extack, "Module firmware flashing already in progress"); + return -EBUSY; + } + + module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL); + if (!module_fw) + return -ENOMEM; + + module_fw->params = *params; + err = request_firmware(&module_fw->fw, module_fw->params.file_name, + &dev->dev); + if (err) { + NL_SET_ERR_MSG(extack, + "Failed to request module firmware image"); + goto err_request_firmware; + } + + err = module_flash_fw_work_init(module_fw, dev, extack); + if (err < 0) { + NL_SET_ERR_MSG(extack, + "Flashing module firmware is not supported by this device"); + goto err_work_init; + } + + dev->module_fw_flash_in_progress = true; + netdev_hold(dev, &module_fw->dev_tracker, GFP_KERNEL); + module_fw->dev = dev; + + schedule_work(&module_fw->work); + + return 0; + +err_work_init: + release_firmware(module_fw->fw); +err_request_firmware: + kfree(module_fw); + return err; +} + +static int module_flash_fw(struct net_device *dev, struct nlattr **tb, + struct netlink_ext_ack *extack) +{ + struct ethtool_module_fw_flash_params params = {}; + struct nlattr *attr; + + if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) { + NL_SET_ERR_MSG_ATTR(extack, + tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME], + "File name attribute is missing"); + return -EINVAL; + } + + params.file_name = + nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]); + + attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD]; + if (attr) { + params.password = cpu_to_be32(nla_get_u32(attr)); + params.password_valid = true; + } + + return module_flash_fw_schedule(dev, ¶ms, extack); +} + +int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info) +{ + struct ethnl_req_info req_info = {}; + struct nlattr **tb = info->attrs; + struct net_device *dev; + int ret; + + ret = ethnl_parse_header_dev_get(&req_info, + tb[ETHTOOL_A_MODULE_FW_FLASH_HEADER], + genl_info_net(info), info->extack, + true); + if (ret < 0) + return ret; + dev = req_info.dev; + + rtnl_lock(); + ret = ethnl_ops_begin(dev); + if (ret < 0) + goto out_rtnl; + + ret = module_flash_fw(dev, tb, info->extack); + + ethnl_ops_complete(dev); + +out_rtnl: + rtnl_unlock(); + ethnl_parse_header_dev_put(&req_info); + return ret; +} + /* MODULE_FW_FLASH_NTF */ static void diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index fe3553f60bf3..85e27bdb1f73 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -1129,6 +1129,13 @@ static const struct genl_ops ethtool_genl_ops[] = { .policy = ethnl_mm_set_policy, .maxattr = ARRAY_SIZE(ethnl_mm_set_policy) - 1, }, + { + .cmd = ETHTOOL_MSG_MODULE_FW_FLASH_ACT, + .flags = GENL_UNS_ADMIN_PERM, + .doit = ethnl_act_module_fw_flash, + .policy = ethnl_module_fw_flash_act_policy, + .maxattr = ARRAY_SIZE(ethnl_module_fw_flash_act_policy) - 1, + }, }; static const struct genl_multicast_group ethtool_nl_mcgrps[] = { diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index 9a333a8d04c1..46712c9531ae 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -441,6 +441,7 @@ extern const struct nla_policy ethnl_plca_set_cfg_policy[ETHTOOL_A_PLCA_MAX + 1] extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADER + 1]; extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1]; extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1]; +extern const struct nla_policy ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD + 1]; int ethnl_set_features(struct sk_buff *skb, struct genl_info *info); int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info); @@ -448,6 +449,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info); int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info); int ethnl_tunnel_info_start(struct netlink_callback *cb); int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb); +int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info); extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN]; extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
Add the ability to flash the modules' firmware by implementing the interface between the user space and the kernel. Example from a succeeding implementation: # ethtool --flash-module-firmware swp40 file test.bin Transceiver module firmware flashing started for device eth0 Transceiver module firmware flashing in progress for device eth0 Status message: Downloading firmware image Progress: 0% [...] Transceiver module firmware flashing in progress for device eth0 Status message: Downloading firmware image Progress: 50% [...] Transceiver module firmware flashing in progress for device eth0 Status message: Downloading firmware image Progress: 100% Transceiver module firmware flashing completed for device eth0 Signed-off-by: Danielle Ratson <danieller@nvidia.com> --- net/ethtool/module.c | 179 ++++++++++++++++++++++++++++++++++++++++++ net/ethtool/netlink.c | 7 ++ net/ethtool/netlink.h | 2 + 3 files changed, 188 insertions(+)