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 PHC | |
Open Source and information security mailing list archives
| ||
|
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