| 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
| ||
|
Date: Thu, 10 Nov 2022 00:26:23 +0000
From: "Mogilappagari, Sudheer" <sudheer.mogilappagari@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"mkubecek@...e.cz" <mkubecek@...e.cz>,
"andrew@...n.ch" <andrew@...n.ch>,
"corbet@....net" <corbet@....net>,
"Samudrala, Sridhar" <sridhar.samudrala@...el.com>,
"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
Subject: RE: [PATCH net-next v2] ethtool: add netlink based get rxfh support
> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Monday, November 7, 2022 6:26 PM
> Subject: Re: [PATCH net-next v2] ethtool: add netlink based get rxfh
> support
>
> On Fri, 4 Nov 2022 16:42:44 -0700 Sudheer Mogilappagari wrote:
> > Implement RXFH_GET request to get RSS table, hash key and hash
> > function of an interface. This is netlink equivalent implementation
> of
> > ETHTOOL_GRSSH ioctl request.
>
> Motivation would be good to have, if any. Are you going to add new
> fields or is it simply to fill in the implementation gap we have in the
> netlink version?
>
Will add more explanation here. Goal was to implement existing
functionality first and then extend by adding new context
specific parameters.
> > +RXFH_GET
> > +========
> > +
> > +Get RSS table, hash key and hash function info like
> ``ETHTOOL_GRSSH``
> > +ioctl request.
>
>
> Can we describe in more detail which commands are reimplemented?
> Otherwise calling the command RXFH makes little sense.
> We may be better of using RSS in the name in the first place.
This is the ethtool command being reimplemented.
ethtool -x|--show-rxfh-indir DEVNAME Show Rx flow hash indirection table and/or RSS hash key
[ context %d ]
Picked RXFH based on existing function names and ethtool_rxfh
structure. If it needs to change, how about RSS_CTX or just RSS ?
> > + ``ETHTOOL_A_RXFH_HEADER`` nested reply header
> > + ``ETHTOOL_A_RXFH_RSS_CONTEXT`` u32 RSS context number
> > + ``ETHTOOL_A_RXFH_INDIR_SIZE`` u32 RSS Indirection table
> size
> > + ``ETHTOOL_A_RXFH_KEY_SIZE`` u32 RSS hash key size
> > + ``ETHTOOL_A_RXFH_HFUNC`` u32 RSS hash func
>
> This is u8 in the implementation, please make the implementation u32 as
> documented.
This should have been u8 instead. Will make it consistent.
>
> > + ``ETHTOOL_A_RXFH_RSS_CONFIG`` u32 Indir table and hkey
> bytes
>
> These should really be separate attributes.
>
> Do we even need the indir_size and key_size given every attribute has a
> length so user can just look at the length of the attrs to see the
> length?
We can split indir table and hkey in netlink implementation and sizes
won't be needed anymore. Above format is based on ethtool_rxfh
structure where indir table and hkey come together as last member of
structure. Will update it in next version.
>
> > +static int rxfh_prepare_data(const struct ethnl_req_info *req_base,
> > + struct ethnl_reply_data *reply_base,
> > + struct genl_info *info)
> > +{
> > + struct rxfh_reply_data *data = RXFH_REPDATA(reply_base);
> > + struct net_device *dev = reply_base->dev;
> > + struct ethtool_rxfh *rxfh = &data->rxfh;
> > + struct ethnl_req_info req_info = {};
> > + struct nlattr **tb = info->attrs;
> > + u32 indir_size = 0, hkey_size = 0;
> > + const struct ethtool_ops *ops;
> > + u32 total_size, indir_bytes;
> > + bool mod = false;
> > + u8 dev_hfunc = 0;
> > + u8 *hkey = NULL;
> > + u8 *rss_config;
> > + int ret;
> > +
> > + ops = dev->ethtool_ops;
> > + if (!ops->get_rxfh)
> > + return -EOPNOTSUPP;
> > +
> > + ret = ethnl_parse_header_dev_get(&req_info,
> > + tb[ETHTOOL_A_RXFH_HEADER],
> > + genl_info_net(info), info->extack,
> > + true);
>
> Why are you parsing again?
>
> You hook in ethnl_default_doit() and ethnl_default_dumpit(), which
> should call the parsing for you already.
>
My bad. Had used other netlink get command implementation as reference
and overlooked request_ops->parse_request.
> > + if (ret < 0)
> > + return ret;
> > +
> > + ethnl_update_u32(&rxfh->rss_context,
> tb[ETHTOOL_A_RXFH_RSS_CONTEXT],
> > + &mod);
>
> ethnl_update_u32() is for when you're updating the config.
> You can use plain netlink helpers to get request arguments.
Ack.
> > + /* Some drivers don't handle rss_context */
> > + if (rxfh->rss_context && !ops->get_rxfh_context) {
> > + ret = -EOPNOTSUPP;
> > + goto out_dev;
> > + }
> > +
> > + ret = ethnl_ops_begin(dev);
> > + if (ret < 0)
> > + goto out_dev;
> > +
> > + if (ops->get_rxfh_indir_size)
> > + indir_size = ops->get_rxfh_indir_size(dev);
> > + if (ops->get_rxfh_key_size)
> > + hkey_size = ops->get_rxfh_key_size(dev);
> > +
> > + indir_bytes = indir_size * sizeof(rxfh->rss_config[0]);
> > + total_size = indir_bytes + hkey_size;
> > + rss_config = kzalloc(total_size, GFP_USER);
>
> GFP_KERNEL is enough here
>
Will fix in next version.
> > + if (!rss_config) {
> > + ret = -ENOMEM;
> > + goto out_ops;
> > + }
> > +
> > + if (indir_size) {
> > + data->rss_config = (u32 *)rss_config;
> > + rxfh->indir_size = indir_size;
> > + }
> > +
> > + if (hkey_size) {
> > + hkey = rss_config + indir_bytes;
> > + rxfh->key_size = hkey_size;
> > + }
> > +
> > + if (rxfh->rss_context)
> > + ret = ops->get_rxfh_context(dev, data->rss_config, hkey,
> > + &dev_hfunc, rxfh->rss_context);
> > + else
> > + ret = ops->get_rxfh(dev, data->rss_config, hkey,
> &dev_hfunc);
>
> This will not be sufficient for dump, I'm afraid.
>
> For dump we need to find a way to dump all contexts in all devices.
> Which may require extending the driver API. Maybe drop the dump
> implementation for now?
>
Agree. Will remove dumpit for this command.
> > + rxfh->hfunc = dev_hfunc;
> > +
> > +out_ops:
> > + ethnl_ops_complete(dev);
> > +out_dev:
> > + ethnl_parse_header_dev_put(&req_info);
> > + return ret;
> > +}
Powered by blists - more mailing lists