netdev - Re: [net-next PATCH v2 5/9] netdev-genl: Add netlink framework functions for napi

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: <0712fad6-bebd-4c9b-9ab7-46c0aa0ea02a@intel.com>
Date: Wed, 23 Aug 2023 16:53:39 -0700
From: "Nambiar, Amritha" <amritha.nambiar@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>,
	<sridhar.samudrala@...el.com>
Subject: Re: [net-next PATCH v2 5/9] netdev-genl: Add netlink framework
 functions for napi

On 8/22/2023 5:51 PM, Jakub Kicinski wrote:
> On Mon, 21 Aug 2023 16:25:36 -0700 Amritha Nambiar wrote:
>> Implement the netdev netlink framework functions for
>> napi support. The netdev structure tracks all the napi
>> instances and napi fields. The napi instances and associated
>> queue[s] can be retrieved this way.
>>
>> Signed-off-by: Amritha Nambiar <amritha.nambiar@...el.com>
> 
>> @@ -119,14 +134,158 @@ int netdev_nl_dev_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>>   	return skb->len;
>>   }
>>   
>> +static int
>> +netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi,
>> +			const struct genl_info *info)
>> +{
>> +	struct netdev_rx_queue *rx_queue, *rxq;
>> +	struct netdev_queue *tx_queue, *txq;
>> +	unsigned int rx_qid, tx_qid;
>> +	void *hdr;
>> +
>> +	if (!napi->dev)
>> +		return -EINVAL;
> 
> WARN_ON_ONCE()? If this can be assumed not to happen.

Okay. Will fix in v3.

> 
>> +	hdr = genlmsg_iput(rsp, info);
>> +	if (!hdr)
>> +		return -EMSGSIZE;
>> +
>> +	if (nla_put_u32(rsp, NETDEV_A_NAPI_NAPI_ID, napi->napi_id))
> 
> napi_id can be zero.

Will fix in v3.

> 
>> +		goto nla_put_failure;
>> +
>> +	if (nla_put_u32(rsp, NETDEV_A_NAPI_IFINDEX, napi->dev->ifindex))
>> +		goto nla_put_failure;
> 
>>   int netdev_nl_napi_get_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
>> -	return -EOPNOTSUPP;
>> +	struct net_device *netdev;
>> +	struct sk_buff *rsp;
>> +	u32 napi_id;
>> +	int err;
>> +
>> +	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_NAPI_NAPI_ID))
>> +		return -EINVAL;
>> +
>> +	napi_id = nla_get_u32(info->attrs[NETDEV_A_NAPI_NAPI_ID]);
>> +
>> +	rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> +	if (!rsp)
>> +		return -ENOMEM;
>> +
>> +	rtnl_lock();
>> +
>> +	netdev = dev_get_by_napi_id(napi_id);
> 
> Why lookup the dev and not the NAPI?

Agree. Will fix in v3.

> 
>> +	if (netdev)
>> +		err  = netdev_nl_napi_fill(netdev, rsp, info, napi_id);
>> +	else
>> +		err = -ENODEV;
>> +
>> +	rtnl_unlock();
>> +
>> +	if (err)
>> +		goto err_free_msg;
>> +
>> +	return genlmsg_reply(rsp, info);
>> +
>> +err_free_msg:
>> +	nlmsg_free(rsp);
>> +	return err;
>> +}
>> +
>> +static int
>> +netdev_nl_napi_dump_one(struct net_device *netdev, struct sk_buff *rsp,
>> +			const struct genl_info *info, int *start)
>> +{
>> +	struct napi_struct *napi, *n;
>> +	int err = 0;
>> +	int i = 0;
>> +
>> +	list_for_each_entry_safe(napi, n, &netdev->napi_list, dev_list) {
> 
> Why _safe()? I think you need _rcu() instead?

Agree. This is called within rtnl_lock. Will fix.

> 
>> +		if (i < *start) {
>> +			i++;
>> +			continue;
>> +		}
>> +		err = netdev_nl_napi_fill_one(rsp, napi, info);
>> +		if (err)
>> +			break;
>> +		*start = ++i;
> 
> Why count them instead of relying on the IDs?

Makes sense. Will fix.

> 
>>   int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>>   {
>> -	return -EOPNOTSUPP;
>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> 
> You can get genl_info_dump(cb) here, you don't use the genl_dumpit_info
> AFAICT, only info->info.

Will fix in v3.