Re: [PATCH ethtool-next v4 2/2] netlink: add netlink handler for get rss (-x)

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 6 Jan 2023 17:41:39 +0000 Mogilappagari, Sudheer wrote:
> > > +	rss_hfunc = mnl_attr_get_u32(tb[ETHTOOL_A_RSS_HFUNC]);
> > > +
> > > +	indir_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_INDIR]);
> > > +	indir_table = mnl_attr_get_str(tb[ETHTOOL_A_RSS_INDIR]);
> > > +
> > > +	hkey_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_HKEY]);
> > > +	hkey = mnl_attr_get_str(tb[ETHTOOL_A_RSS_HKEY]);  
> > 
> > All elements of tb can be NULL, AFAIU.  
> 
> Didn't get this. Do you mean the variables need to be checked 
> for NULL here? If yes, am checking before printing later on.  

tb[ETHTOOL_A_RSS_HKEY] can be NULL. I just realized now that the kernel
always fills in the attrs:

	if (nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc) ||
	    nla_put(skb, ETHTOOL_A_RSS_INDIR,
		    sizeof(u32) * data->indir_size, data->indir_table) ||
	    nla_put(skb, ETHTOOL_A_RSS_HKEY, data->hkey_size, data->hkey))
		return -EMSGSIZE;

but that's not a great use of netlink. If there is nothing to report
the attribute should simply be skipped, like this:

	if (nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc)) ||
	    (data->indir_size && 
	     nla_put(skb, ETHTOOL_A_RSS_INDIR,
		     sizeof(u32) * data->indir_size, data->indir_table)) ||
	    (data->hkey_size &&
	     nla_put(skb, ETHTOOL_A_RSS_HKEY, data->hkey_size, data->hkey)))
		return -EMSGSIZE;

I think we should fix the kernel side.

> > > +int get_channels_cb(const struct nlmsghdr *nlhdr, void *data) {  
> 
> > > +	silent = nlctx->is_dump || nlctx->is_monitor;
> > > +	err_ret = silent ? MNL_CB_OK : MNL_CB_ERROR;
> > > +	ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
> > > +	if (ret < 0)
> > > +		return err_ret;
> > > +	nlctx->devname = get_dev_name(tb[ETHTOOL_A_CHANNELS_HEADER]);  
> > 
> > We need to check that the kernel has filled in the attrs before
> > accessing them, no?  
> 
> Didn't get this one either. similar code isn't doing any checks 
> like you suggested.

Same point, really. Even if the kernel always populates the attribute
today, according to netlink rules it's not guaranteed to do so in the
future, so any tb[] access should be NULL-checked.

> > The correct value is combined + rx, I think I mentioned this before.  
> 
> Have changed it to include rx too like below. 
> args->num_rings = mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]);
> args->num_rings += mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_RX_COUNT]);

Related to previous comments - take a look at channels_fill_reply().
tb[ETHTOOL_A_CHANNELS_RX_COUNT] will be NULL for most devices.
mnl_attr_get_u32(NULL) will crash.

> Slightly unrelated, where can I find the reason behind using combined + rx?
> Guess it was discussed in mailing list but not able to find it.

Yes, it's been discussed on the list multiple times but man ethtool 
is the only source of documentation I know of.

The reason is that the channels API refers to interrupts more than
queues. So rx means an _irq_ dedicated to Rx processing, not an Rx
queue. Unfortunately the author came up with the term "channel" which
most people take to mean "queue" :(

> > +	return MNL_CB_OK;
> > 
> > I'm also not sure if fetching the channel info shouldn't be done over
> > the nl2 socket, like the string set. If we are in monitor mode we may
> > confuse ourselves trying to parse the wrong messages.  
> 
> Are you suggesting we need to use ioctl for fetching ring info to avoid
> mix-up. Is there alternative way to do it ?  

No no, look how the strings for hfunc names are fetched - they are
fetched over a different socket, right?