On Thu, Mar 18, 2021 at 02:03:02PM +0100, Andrew Lunn wrote: > On Mon, Mar 15, 2021 at 07:12:39PM +0200, Moshe Shemesh wrote: > > > > +EEPROM_DATA > > +=========== > > + > > +Fetch module EEPROM data dump. > > + > > +Request contents: > > + > > + ===================================== ====== ========================== > > + ``ETHTOOL_A_EEPROM_DATA_HEADER`` nested request header > > + ``ETHTOOL_A_EEPROM_DATA_OFFSET`` u32 offset within a page > > + ``ETHTOOL_A_EEPROM_DATA_LENGTH`` u32 amount of bytes to read > > I wonder if offset and length should be u8. At most, we should only be > returning a 1/2 page, so 128 bytes. We don't need a u32.
There is no actual gain using NLA_U8 due to padding. Out of the interfaces used here, kernel-userspace API is the least flexible so I would generally prefer NLA_U32, except for bools or enumerated values where it's absolutely obvious the number of possible values cannot grow too much. In this case, I can't really say it's impossible we could have devices with bigger pages in something like 20 years. > > Request translation > > =================== > > > > @@ -1357,8 +1387,8 @@ are netlink only. > > ``ETHTOOL_GET_DUMP_FLAG`` n/a > > ``ETHTOOL_GET_DUMP_DATA`` n/a > > ``ETHTOOL_GET_TS_INFO`` ``ETHTOOL_MSG_TSINFO_GET`` > > - ``ETHTOOL_GMODULEINFO`` n/a > > - ``ETHTOOL_GMODULEEEPROM`` n/a > > + ``ETHTOOL_GMODULEINFO`` ``ETHTOOL_MSG_MODULE_EEPROM_GET`` > > + ``ETHTOOL_GMODULEEEPROM`` ``ETHTOOL_MSG_MODULE_EEPROM_GET`` > > ``ETHTOOL_GEEE`` ``ETHTOOL_MSG_EEE_GET`` > > ``ETHTOOL_SEEE`` ``ETHTOOL_MSG_EEE_SET`` > > ``ETHTOOL_GRSSH`` n/a > > We should check with Michal about this. It is not a direct replacement > of the old IOCTL API, it is new API. He may want it documented > differently. This table is meant to give a hint in the sense "for what you used ioctl command in left column, use now netlink request in the right". So IMHO it's appropriate. Perhaps it would deserve a comment explaining this. > > + request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]); > > + request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]); > > + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] && > > + dev->ethtool_ops->get_module_eeprom_data_by_page && > > + request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN) > > + return -EINVAL; > > You need to watch out for overflows here. 0xfffffff0 + 0x20 is less > than ETH_MODULE_EEPROM_PAGE_LEN when it wraps around, but will cause > bad things to happen. BtW, the ioctl code also suffers from this problem and we recently had a report from customer (IIRC the effect was ethtool trying to allocate ~4GB of memory), upstream fix should follow soon. Michal
