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

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: <IA1PR11MB62668007BA5BA017F5B46708E4FB9@IA1PR11MB6266.namprd11.prod.outlook.com>
Date:   Fri, 6 Jan 2023 17:41:39 +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>,
        "Samudrala, Sridhar" <sridhar.samudrala@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
Subject: RE: [PATCH ethtool-next v4 2/2] netlink: add netlink handler for get
 rss (-x)



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>


> I believe there can only be a single bit set here, so why not print:
> 
> 	 "rss-hash-function": "toeplitz"
>
> rather than:
> 
> 	  "rss-hash-function": {
>             "toeplitz": true,
>             "xor": false,
>             "crc32": false
>           }
> 

Was following similar output format as current one. Changed in next version. 

> > +	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.  
 
> > +	rss->indir_size = indir_bytes / sizeof(rss->rss_config[0]);
> > +	rss->key_size = hkey_bytes;
> > +	rss->hfunc = rss_hfunc;
> > +
> > +	memcpy(rss->rss_config, indir_table, indir_bytes);
> > +	memcpy(rss->rss_config + rss->indir_size, hkey, hkey_bytes);
> 
> Do you only perform this coalescing to reuse the existing print
> helpers? With a bit of extra refactoring this seems avoidable...

Yes. Was reusing print helpers. Have refactored to avoid need for
coalescing.

> > +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.

> > +	if (!dev_ok(nlctx))
> > +		return err_ret;
> > +
> > +	args->num_rings =
> > +mnl_attr_get_u8(tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]);
> 
> u32, not u8

Fixed.

> 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]);

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.

> +	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 ?