netdev - Re: [PATCH net-next 01/11] ethtool: rss: initial RSS

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250714091548.1a7c999b@kernel.org>
Date: Mon, 14 Jul 2025 09:15:48 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Gal Pressman <gal@...dia.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
 donald.hunter@...il.com, shuah@...nel.org, kory.maincent@...tlin.com,
 maxime.chevallier@...tlin.com, sdf@...ichev.me, ecree.xilinx@...il.com
Subject: Re: [PATCH net-next 01/11] ethtool: rss: initial RSS_SET
 (indirection table handling)

On Sun, 13 Jul 2025 14:08:48 +0300 Gal Pressman wrote:
> >  static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
> > diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
> > index 41ab9fc67652..7167fc3c27a0 100644
> > --- a/net/ethtool/rss.c
> > +++ b/net/ethtool/rss.c
> > @@ -218,6 +218,8 @@ rss_prepare(const struct rss_req_info *request, struct net_device *dev,
> >  {
> >  	rss_prepare_flow_hash(request, dev, data, info);
> >  
> > +	if (!dev->ethtool_ops->get_rxfh)
> > +		return 0;  
> 
> What is this for?

Silly drivers which support selecting which fields are hashed, 
but don't have a callback for basic RSS config. I'll add a comment.

> >  	if (request->rss_context)
> >  		return rss_prepare_ctx(request, dev, data, info);
> >  	return rss_prepare_get(request, dev, data, info);
> > @@ -466,6 +468,192 @@ void ethtool_rss_notify(struct net_device *dev, u32 rss_context)
> >  	ethnl_notify(dev, ETHTOOL_MSG_RSS_NTF, &req_info.base);
> >  }
> >  
> > +static int
> > +ethnl_rss_set_validate(struct ethnl_req_info *req_info, struct genl_info *info)
> > +{
> > +	const struct ethtool_ops *ops = req_info->dev->ethtool_ops;
> > +	struct rss_req_info *request = RSS_REQINFO(req_info);
> > +	struct nlattr **tb = info->attrs;
> > +	struct nlattr *bad_attr = NULL;
> > +
> > +	if (request->rss_context && !ops->create_rxfh_context)
> > +		bad_attr = bad_attr ?: tb[ETHTOOL_A_RSS_CONTEXT];  
> 
> If we wish to be consistent with the ioctl flow, we should also check
> that "at least one change was requested".
> 
> i.e., if (!tb[ETHTOOL_A_RSS_INDIR]) return err?

I was wondering about that, netlink ethtool doesn't have such checks
for other ops. I think consistency within the netlink family trumps 
the random check in the ioctl code.

> > +	rxfh->indir_size = data->indir_size;
> > +	alloc_size = array_size(data->indir_size, sizeof(rxfh->indir[0]));
> > +	rxfh->indir = kzalloc(alloc_size, GFP_KERNEL);
> > +	if (!rxfh->indir)
> > +		return -ENOMEM;
> > +
> > +	nla_memcpy(rxfh->indir, tb[ETHTOOL_A_RSS_INDIR], alloc_size);  
> 
> ethnl_update_binary() will take care of the explicit memcmp down the line?
> 
> > +	for (i = 0; i < user_size; i++) {
> > +		if (rxfh->indir[i] < rx_rings.data)
> > +			continue;
> > +
> > +		NL_SET_ERR_MSG_ATTR_FMT(extack, tb[ETHTOOL_A_RSS_INDIR],
> > +					"entry %d: queue out of range (%d)",
> > +					i, rxfh->indir[i]);
> > +		err = -EINVAL;
> > +		goto err_free;
> > +	}
> > +
> > +	if (user_size) {
> > +		/* Replicate the user-provided table to fill the device table */
> > +		for (i = user_size; i < data->indir_size; i++)
> > +			rxfh->indir[i] = rxfh->indir[i % user_size];
> > +	} else {
> > +		for (i = 0; i < data->indir_size; i++)
> > +			rxfh->indir[i] =
> > +				ethtool_rxfh_indir_default(i, rx_rings.data);  
> 
> Unless you wanted the mcmp to also take care of this case?

Yes, and I think the case of upsizing could also result in false
negatives if we don't memcmp() the whole array.

> > +	if (ctx)
> > +		rss_set_ctx_update(ctx, tb, &data, &rxfh);
> > +	else if (indir_reset)
> > +		dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
> > +	else if (indir_mod)
> > +		dev->priv_flags |= IFF_RXFH_CONFIGURED;  
> 
> One can argue that IFF_RXFH_CONFIGURED should be set even if the
> requested table is equal to the default one.

Good catch.