netdev - Re: [net-next PATCH v3 06/10] 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: <d80363ac-9487-4b1c-8069-82e0d931aa7c@intel.com>
Date: Thu, 28 Sep 2023 16:34:08 -0700
From: "Nambiar, Amritha" <amritha.nambiar@...el.com>
To: Paolo Abeni <pabeni@...hat.com>, <netdev@...r.kernel.org>,
	<kuba@...nel.org>
CC: <sridhar.samudrala@...el.com>
Subject: Re: [net-next PATCH v3 06/10] netdev-genl: Add netlink framework
 functions for napi

On 9/28/2023 4:19 AM, Paolo Abeni wrote:
> On Tue, 2023-09-19 at 15:27 -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
>> parameters can be retrieved this way.
>>
>> Signed-off-by: Amritha Nambiar <amritha.nambiar@...el.com>
>> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@...el.com>
>> ---
>>   include/linux/netdevice.h |    2 +
>>   net/core/dev.c            |    4 +-
>>   net/core/netdev-genl.c    |  117 ++++++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 119 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 69e363918e4b..e7321178dc1a 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -536,6 +536,8 @@ static inline bool napi_complete(struct napi_struct *n)
>>   	return napi_complete_done(n, 0);
>>   }
>>   
>> +struct napi_struct *napi_by_id(unsigned int napi_id);
>> +
>>   int dev_set_threaded(struct net_device *dev, bool threaded);
>>   
>>   /**
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 508b1d799681..ea6b3115ee8b 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -165,7 +165,6 @@ static int netif_rx_internal(struct sk_buff *skb);
>>   static int call_netdevice_notifiers_extack(unsigned long val,
>>   					   struct net_device *dev,
>>   					   struct netlink_ext_ack *extack);
>> -static struct napi_struct *napi_by_id(unsigned int napi_id);
>>   
>>   /*
>>    * The @dev_base_head list is protected by @dev_base_lock and the rtnl
>> @@ -6133,7 +6132,7 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
>>   EXPORT_SYMBOL(napi_complete_done);
>>   
>>   /* must be called under rcu_read_lock(), as we dont take a reference */
>> -static struct napi_struct *napi_by_id(unsigned int napi_id)
>> +struct napi_struct *napi_by_id(unsigned int napi_id)
>>   {
>>   	unsigned int hash = napi_id % HASH_SIZE(napi_hash);
>>   	struct napi_struct *napi;
>> @@ -6144,6 +6143,7 @@ static struct napi_struct *napi_by_id(unsigned int napi_id)
>>   
>>   	return NULL;
>>   }
>> +EXPORT_SYMBOL(napi_by_id);
>>   
>>   #if defined(CONFIG_NET_RX_BUSY_POLL)
>>   
>> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
>> index 8609884fefe4..6f4ed21ebd15 100644
>> --- a/net/core/netdev-genl.c
>> +++ b/net/core/netdev-genl.c
>> @@ -7,6 +7,7 @@
>>   #include <net/sock.h>
>>   #include <net/xdp.h>
>>   #include <net/netdev_rx_queue.h>
>> +#include <net/busy_poll.h>
>>   
>>   #include "netdev-genl-gen.h"
>>   
>> @@ -14,6 +15,7 @@ struct netdev_nl_dump_ctx {
>>   	unsigned long	ifindex;
>>   	unsigned int	rxq_idx;
>>   	unsigned int	txq_idx;
>> +	unsigned int	napi_id;
> 
> Why you need to add napi_id to the context? don't you need just such
> field to do the traversing? e.g. use directly single cb[0]
> 

Not sure I follow this comment. Could you please clarify.

I introduced the netdev_nl_dump_ctx structure to save the 
states/counters for dump (for ifindex, queue indices, NAPI IDs etc.) if 
the dump was interrupted due to EMSGSIZE error. Also, I decided to use 
cb->ctx instead of cb->args[] due to a comment in the definition of 
struct netlink_callback in include/linux/netlink.h that says "args is 
deprecated. Cast a struct over ctx instead for proper type safety."

The 'napi_id' is saved into netdev_nl_dump_ctx since there is no need 
for a separate counter when we can rely on the NAPI IDs itself (as 
pointed out by Jakub's review comment on v2).

> [...]
>> +static int
>> +netdev_nl_napi_dump_one(struct net_device *netdev, struct sk_buff *rsp,
>> +			const struct genl_info *info, unsigned int *start)
>> +{
>> +	struct napi_struct *napi;
>> +	int err = 0;
>> +
>> +	list_for_each_entry_rcu(napi, &netdev->napi_list, dev_list) {
> 
> I'm sorry for the conflicting feedback here, but I think you don't need
> any special variant, just 'list_for_each_entry'. This is under rtnl
> lock, the list can't change under the hood. Also I think you should get
> a RCU splat here when building with CONFIG_PROVE_RCU_LIST.
> 

Makes sense. Access is protected by the rtnl_lock, so _rcu protection 
isn't needed, I can just use 'list_for_each_entry' for list traversal.

> 
> Cheers,
> 
> Paolo
>