| 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: <0f021f89-35d4-4d99-b0b1-451f09636e58@nvidia.com>
Date: Tue, 29 Dec 2020 12:23:04 +0200
From: Vladyslav Tarasiuk <vladyslavt@...dia.com>
To: Andrew Lunn <andrew@...n.ch>, Moshe Shemesh <moshe@...dia.com>
CC: Jakub Kicinski <kuba@...nel.org>,
Moshe Shemesh <moshe@...lanox.com>,
"David S. Miller" <davem@...emloft.net>,
Adrian Pop <pop.adrian61@...il.com>,
Michal Kubecek <mkubecek@...e.cz>, <netdev@...r.kernel.org>,
"Maxim Mikityanskiy" <maximmi@...dia.com>, <vladyslavt@...dia.com>
Subject: Re: [PATCH net-next v2 0/2] Add support for DSFP transceiver type
On 27-Nov-20 17:56, Andrew Lunn wrote:
>> OK, but if the caching system is checking one time netlink and one time
>> ioctl, it means this cache should be in user space, or did you mean to have
>> this cache in kernel ?
> This is all in userspace, in the ethtool code.
>
>>>> What about the global offset that we currently got when user doesn't specify
>>>> a page, do you mean that this global offset goes through the optional and
>>>> non optional pages that exist and skip the ones that are missing according
>>>> to the specific EEPROM ?
>>> ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off] [hex on|off] [offset N] [length N]
>>>
>>> So you mean [offset N] [length N].
>>
>> Yes, that's the current options and we can either try coding new
>> implementation for that or just call the current ioctl implementation. The
>> new code can be triggered once options [bank N] and [Page N] are used.
> You cannot rely on the ioctl being available. New drivers won't
> implement it, if they have the netlink code. Drivers will convert from
> get_module_info to whatever new ndo call you add for netlink.
>
>> OK, if I got it right on current API [offset N] [length N] just call ioctl
>> current implementation, while using the option [raw on] will call new
>> implementation for new SFPs (CMIS 4). Also using [bank N] and [page N] will
>> call new implementation for new SFPs.
> Not just CMIS. All SFPs.
>
> Andrew
Hi Andrew,
Following this conversation, I wrote some pseudocode checking if I'm on
right path here.
Please review:
struct eeprom_page {
u8 page_number;
u8 bank_number;
u16 offset;
u16 data_length;
u8 *data;
}
Check every requested page, offset and length availability in userspace,
depending on module standard and EEPROM data.
================================================================================
cache_fetch_add(page_number, bank_number, offset, length)
{
ethnla_put_u8 (msgbuff, ETHTOOL_A_EEPROM_PAGE_NUMBER, page_number);
ethnla_put_u8 (msgbuff, ETHTOOL_A_EEPROM_BANK_NUMBER, bank_number);
ethnla_put_u16(msgbuff, ETHTOOL_A_EEPROM_PAGE_OFFSET, offset);
ethnla_put_u16(msgbuff, ETHTOOL_A_EEPROM_PAGE_LENGTH, length);
err = nlsock_send_get_request(eeprom_page_reply_cb) // caches a
page
if (err < 0)
return err;
}
// Any call should be a pair of cache_fetch_add() with error check and only
// then a cache_get(), but for pseudocode this will do
cache_get_page(page_number, bank_number, offset, length)
{
if (!cache_contains(page_number, bank_number, offset, length))
cache_fetch_add(page_number, bank_number, offset, length);
return cache_get(page_number, bank_number);
}
print_cmis_y()
{
page_data = cache_get_page(0, 0, 0, 256)->data;
print_page_zero(page_data);
page_data = cache_get_page(1, 0, 128, 128)->data;
print_page_one(page_data);
}
print_human_readable()
{
spec_id = cache_get_page(0, 0, 0, 128)->data[0];
switch (spec_id) {
case sff_xxxx:
print_sff_xxxx();
case cmis_y:
print_cmis_y();
default:
print_hex();
}
}
getmodule_reply_cb()
{
if (offset || hex || bank_number || page number)
print_hex();
else
// if _human_readable() decoder needs more than page
00, it will
// fetch it on demand
print_human_readable();
}
eeprom_page_reply_cb()
{
cache_add(new_page);
}
page_available(page_number)
{
spec_id = cache_get_page(0, 0, 0, 128)->data[0];
// check bits indicating page availability
switch (spec_id) {
case sff_xxxx:
return is_page_available_sff_xxxx(page_number);
case cmis_y:
return is_page_available_cmis_y(page_number, bank_number);
default:
return -EINVAL;
}
}
nl_getmodule()
{
msg_init();
// request first low page
ret = cache_fetch_add(0, 0, 0, 128);
if (ret < 0) // netlink unsupported, fall back to ioctl
return ret;
netlink_cmd_check();
nl_parser()
// check bits indicating page availability according to used spec
if (page_available(page_number, bank_number)) {
if (cache_contains(page_number, bank_number, offset,
length))
print_page(page_number, bank_number, offset,
length)
else {
ret = cache_fetch_add();
if (ret < 0)
return ret;
print_page(page_number, bank_number, offset,
length);
}
} else {
return -EINVAL;
}
}
Or just proceed to request any page and depend on kernel parameter
validation
================================================================================
getmodule_reply_cb()
{
if (offset || hex || bank_number || page_number)
print_hex();
else
// if _human_readable() decoder needs more than page
00, it will
// fetch it on demand
print_human_readable();
}
nl_getmodule()
{
// request page straight away
netlink_cmd_check();
nl_parser();
/*
* nl_parser() sets page_number, page_bank, offset and length and
* prepares to pass them to the kernel. See
eeprom_page_parse_request()
*/
ret = nlsock_sendmsg(getmodule_reply_cb);
if (ret < 0)
retrun ret;
}
Kernel
This part is the same for both userspace variants
================================================================================
kernel_fallback(request, data)
{
struct ethtool_eeprom eeprom;
if (request->bank_number)
return -EINVAL;
// also take into account page 00 size
eeprom.offset = request->offset + page_size * request->page_number;
eeprom.len = request->length;
eeprom.cmd = ETHTOOL_GMODULEEEPROM;
eeprom.data = data;
err = ops->get_module_eeprom(dev, &eeprom, eeprom.data)
if (err < 0)
return err;
// prepare data for response via netlink like for supported ndo
}
eeprom_page_parse_request()
{
struct eeprom_page_req_info *request;
request->offset = nla_get_u16(tb[ETHTOOL_A_EEPROM_PAGE_OFFSET]);
request->length = nla_get_u16(tb[ETHTOOL_A_EEPROM_PAGE_LENGTH]);
request->page_number =
nla_get_u8(tb[ETHTOOL_A_EEPROM_PAGE_NUMBER]);
request->bank_number =
nla_get_u8(tb[ETHTOOL_A_EEPROM_PAGE_BANK_NUMBER]);
}
eeprom_page_prepare_data(request)
{
u8 data[128];
u8 page_zero_data[128];
if (!ops->get_module_eeprom_page) {
if (ops->get_module_info && ops->get_module_eeprom)
return kernel_fallback(request, &data);
else
return -EOPNOTSUPP;
}
// fetch page 00 for further checks
err = ops->get_module_eeprom_page(dev, 0, 128, 0, 0,
&page_zero_data);
if (err < 0)
return err;
// Checks according to spec, similar to page_available() in
userspace.
// May be ommited and passed directly to the driver for it to
validate
if (is_request_valid(request, &page_zero_data))
err = ops->get_module_eeprom_page(dev, request->offset,
request->length,
request->page_number,
request->bank_number, &data);
else
return -EINVAL;
}
Driver
It is required to implement get_module_eeprom_page() ndo, where it queries
its EEPROM and copies to u8 *data array allocated by the kernel previously.
The ndo has the following prototype:
int get_module_eeprom_page(struct net_device *, u16 offset, u16 length,
u8 page_number, u8 bank_number, u8 *data);
The driver may test whatever it may need in this ndo implementation.
Its parameters may also be passed using a special struct.
===============================================================================
driver_get_module_eeprom_page()
{
vendor_query_module_eeprom(page_number, bank_number, offset,
length, data);
}
Thanks,
Vlad
Powered by blists - more mailing lists