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

["Nambiar, Amritha" <amritha.nambiar@intel.com>]

On 7/30/2023 10:15 AM, Simon Horman wrote:
> On Fri, Jul 28, 2023 at 05:47:17PM -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@intel.com>
>> ---
>>   net/core/netdev-genl.c |  253 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 251 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> 
> ...
> 
>>   int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>>   {
>> -	return -EOPNOTSUPP;
>> +	struct netdev_nl_dump_ctx *ctx = netdev_dump_ctx(cb);
>> +	struct net *net = sock_net(skb->sk);
>> +	struct net_device *netdev;
>> +	int idx = 0, s_idx, n_idx;
>> +	int h, s_h;
>> +	int err;
>> +
>> +	s_h = ctx->dev_entry_hash;
>> +	s_idx = ctx->dev_entry_idx;
>> +	n_idx = ctx->napi_idx;
>> +
>> +	rtnl_lock();
>> +
>> +	for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
>> +		struct hlist_head *head;
>> +
>> +		idx = 0;
>> +		head = &net->dev_index_head[h];
>> +		hlist_for_each_entry(netdev, head, index_hlist) {
>> +			if (idx < s_idx)
>> +				goto cont;
>> +			err = netdev_nl_napi_dump_entry(netdev, skb, cb, &n_idx);
>> +			if (err == -EMSGSIZE)
>> +				goto out;
>> +			n_idx = 0;
>> +			if (err < 0)
>> +				break;
>> +cont:
>> +			idx++;
>> +		}
>> +	}
>> +
>> +	rtnl_unlock();
>> +
>> +	return err;
> 
> Hi Amritha,
> 
> I'm unsure if this can happen, but if loop iteration occurs zero times
> above in such a way that netdev_nl_napi_dump_entry() isn't called, then err
> will be uninitialised here.
> 
> This is also the case in netdev_nl_dev_get_dumpit
> (both before and after this patch.
> 
> As flagged by Smatch.
> 

Will fix the initialization in the next version.

>> +
>> +out:
>> +	rtnl_unlock();
>> +
>> +	ctx->dev_entry_idx = idx;
>> +	ctx->dev_entry_hash = h;
>> +	ctx->napi_idx = n_idx;
>> +	cb->seq = net->dev_base_seq;
>> +
>> +	return skb->len;
>>   }
> 
> ...