| 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: <4F85B639.5090900@solarflare.com>
Date: Wed, 11 Apr 2012 17:50:01 +0100
From: Stuart Hodgson <smhodgson@...arflare.com>
To: Ben Hutchings <bhutchings@...arflare.com>
CC: <netdev@...r.kernel.org>, <bruce.w.allan@...el.com>,
<mirq-linux@...e.qmqm.pl>, <decot@...gle.com>,
<amit.salecha@...gic.com>, <alexander.h.duyck@...el.com>,
<davem@...emloft.net>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/2] net: ethtool: Add capability to retrieve plug-in
module EEPROM
On 02/04/12 18:52, Ben Hutchings wrote:
> We previously discussed the need for this in person, so I'm going to
> review this as a submission rather than an RFC.
>
> On Tue, 2012-03-27 at 18:51 +0100, Stuart Hodgson wrote:
>> Added extensions to the ethtool API to obtain plugin module eeprom data
>> This is useful for end users to be able to determine what the capabilities
>> of the in use module are.
>>
>> The first provides a new struct ethtool_modinfo that will return the
>> type and size of plug-in module eeprom (such as SFP+) for parsing
>> by userland program.
>>
>> The second provides the API to get the raw eeprom information
>> using the existing ethtool_eeprom structture to return the data
>>
>> Signed-off-by: Stuart Hodgson<smhodgson@...arflare.com>
>> ---
>> include/linux/ethtool.h | 20 ++++++++++++
>> net/core/ethtool.c | 79
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 99 insertions(+), 0 deletions(-)
>
> This is line-wrapped; you'll need to take care to avoid that when
> submitting the patch for real. Tabs have been converted to spaces,
> which you also need to avoid. See Documentation/email-clients.txt.
>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index da5b2de..50eaf35 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -117,6 +117,14 @@ struct ethtool_eeprom {
>> __u8 data[0];
>> };
>>
>> +/* for passing plug-in module information */
>> +struct ethtool_modinfo {
>> + __u32 cmd;
>> + __u32 type;
>> + __u32 eeprom_len;
>> + __u32 reserved[8];
>> +};
>> +
>
> This needs a proper kernel-doc comment with a description of each of the
> fields (except reserved).
>
>> /**
>> * struct ethtool_coalesce - coalescing parameters for IRQs and stats
>> updates
>> * @cmd: ETHTOOL_{G,S}COALESCE
>> @@ -936,6 +944,10 @@ struct ethtool_ops {
>> int (*get_dump_data)(struct net_device *,
>> struct ethtool_dump *, void *);
>> int (*set_dump)(struct net_device *, struct ethtool_dump *);
>> + int (*get_module_info)(struct net_device *,
>> + struct ethtool_modinfo *);
>> + int (*get_module_eeprom)(struct net_device *,
>> + struct ethtool_eeprom *, u8 *);
>
> These need to be described in the kernel-doc comment for this structure.
>
>> };
>> #endif /* __KERNEL__ */
>> @@ -1010,6 +1022,8 @@ struct ethtool_ops {
>> #define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */
>> #define ETHTOOL_GET_DUMP_FLAG 0x0000003f /* Get dump settings */
>> #define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */
>> +#define ETHTOOL_GMODULEINFO 0x00000041 /* Get plug-in module
>> information */
>> +#define ETHTOOL_GMODULEEEPROM 0x00000042 /* Get plug-in module eeprom */
>>
>> /* compatibility with older code */
>> #define SPARC_ETH_GSET ETHTOOL_GSET
>> @@ -1159,6 +1173,12 @@ struct ethtool_ops {
>> #define RX_CLS_LOC_FIRST 0xfffffffe
>> #define RX_CLS_LOC_LAST 0xfffffffd
>>
>> +/* EEPROM Standards for plug in modules */
>> +#define SFF_8079 0x1
>> +#define SFF_8079_LEN 256
>> +#define SFF_8472 0x2
>> +#define SFF_8472_LEN 512
>> +
>
> I think it's best to include a prefix of 'ethtool_' 'ETHTOOL_' or 'ETH_'
> in new definitions in ethtool.h. Since these are specific to modules I
> would suggest a prefix of 'ETH_MODULE_'.
>
>> /* Reset flags */
>> /* The reset() operation must clear the flags for the components which
>> * were actually reset. On successful return, the flags indicate the
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index 3f79db1..1e86bd9 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
> [...]
>> +static int ethtool_get_module_eeprom(struct net_device *dev,
>> + void __user *useraddr)
>> +{
>> + int ret;
>> + struct ethtool_eeprom eeprom;
>> + struct ethtool_modinfo modinfo;
>> + const struct ethtool_ops *ops = dev->ethtool_ops;
>> + void __user *userbuf = useraddr + sizeof(eeprom);
>> + u8 *data;
>> +
>> + if (!ops->get_module_info || !ops->get_module_eeprom)
>> + return -EOPNOTSUPP;
>> +
>> + if (copy_from_user(&eeprom, useraddr, sizeof(eeprom)))
>> + return -EFAULT;
>> +
>> + /* Check for wrap and zero */
>> + if (eeprom.offset + eeprom.len<= eeprom.offset)
>> + return -EINVAL;
>> +
>> + /* Get the modinfo to get the length */
>> + ret = ops->get_module_info(dev,&modinfo);
>> + if (ret)
>> + return ret;
>> +
>> + if (eeprom.offset + eeprom.len> modinfo.eeprom_len)
>> + return -EINVAL;
>> +
>> + data = kmalloc(PAGE_SIZE, GFP_USER);
>> + if (!data)
>> + return -ENOMEM;
>
> What if some device has a larger EEPROM? Surely this length should be
> eeprom.len.
>
Do you mean what if the eeprom length in te device is larger than
PAGE_SIZE? If so then it should really use modinfo.eeprom_len since
this the size of the data. eeprom.len could be arbitary.
>> + ret = ops->get_module_eeprom(dev,&eeprom, data);
>> + if (ret)
>> + goto out;
>> +
>> +
>> + if (copy_to_user(userbuf, data, eeprom.len)) {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>> +
>> + if (copy_to_user(useraddr,&eeprom, sizeof(eeprom)))
>> + ret = -EFAULT;
> [...]
>
> I think you can drop this last copy as there's no information to return
> in the eeprom structure itself.
>
> Ben.
>
Other comments have been addressed and will be re-submitted.
Stu
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists