netdev - Re: [PATCH net-next v4 14/15] idpf: add ethtool callbacks

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: <ZNNreRbo5jB9CaWc@vergenet.net>
Date: Wed, 9 Aug 2023 12:33:29 +0200
From: Simon Horman <horms@...nel.org>
To: Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	edumazet@...gle.com, netdev@...r.kernel.org,
	Alan Brady <alan.brady@...el.com>, pavan.kumar.linga@...el.com,
	emil.s.tantilov@...el.com, jesse.brandeburg@...el.com,
	sridhar.samudrala@...el.com, shiraz.saleem@...el.com,
	sindhu.devale@...el.com, willemb@...gle.com, decot@...gle.com,
	andrew@...n.ch, leon@...nel.org, mst@...hat.com,
	simon.horman@...igine.com, shannon.nelson@....com,
	stephen@...workplumber.org, Alice Michael <alice.michael@...el.com>,
	Joshua Hay <joshua.a.hay@...el.com>,
	Phani Burra <phani.r.burra@...el.com>
Subject: Re: [PATCH net-next v4 14/15] idpf: add ethtool callbacks

On Mon, Aug 07, 2023 at 05:34:15PM -0700, Tony Nguyen wrote:
> From: Alan Brady <alan.brady@...el.com>
> 
> Initialize all the ethtool ops that are supported by the driver and
> add the necessary support for the ethtool callbacks. Also add
> asynchronous link notification virtchnl support where the device
> Control Plane sends the link status and link speed as an
> asynchronous event message. Driver report the link speed on
> ethtool .idpf_get_link_ksettings query.
> 
> Introduce soft reset function which is used by some of the ethtool
> callbacks such as .set_channels, .set_ringparam etc. to change the
> existing queue configuration. It deletes the existing queues by sending
> delete queues virtchnl message to the CP and calls the 'vport_stop' flow
> which disables the queues, vport etc. New set of queues are requested to
> the CP and reconfigure the queue context by calling the 'vport_open'
> flow. Soft reset flow also adjusts the number of vectors associated to a
> vport if .set_channels is called.
> 
> Signed-off-by: Alan Brady <alan.brady@...el.com>
> Co-developed-by: Alice Michael <alice.michael@...el.com>
> Signed-off-by: Alice Michael <alice.michael@...el.com>
> Co-developed-by: Joshua Hay <joshua.a.hay@...el.com>
> Signed-off-by: Joshua Hay <joshua.a.hay@...el.com>
> Co-developed-by: Phani Burra <phani.r.burra@...el.com>
> Signed-off-by: Phani Burra <phani.r.burra@...el.com>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@...el.com>
> Reviewed-by: Willem de Bruijn <willemb@...gle.com>
> Co-developed-by: Pavan Kumar Linga <pavan.kumar.linga@...el.com>
> Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@...el.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>

...

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c

...

> +/**
> + * idpf_get_ethtool_stats - report device statistics
> + * @netdev: network interface device structure
> + * @stats: ethtool statistics structure
> + * @data: pointer to data buffer
> + *
> + * All statistics are added to the data buffer as an array of u64.
> + */
> +static void idpf_get_ethtool_stats(struct net_device *netdev,
> +				   struct ethtool_stats __always_unused *stats,
> +				   u64 *data)
> +{
> +	struct idpf_vport *vport = idpf_netdev_to_vport(netdev);
> +	struct idpf_vport_config *vport_config;
> +	struct page_pool_stats pp_stats = { };
> +	unsigned int total = 0;
> +	unsigned int i, j;
> +	bool is_splitq;
> +	u16 qtype;
> +
> +	if (!vport || vport->state != __IDPF_VPORT_UP)
> +		return;
> +
> +	rcu_read_lock();
> +
> +	idpf_collect_queue_stats(vport);
> +	idpf_add_port_stats(vport, &data);
> +
> +	for (i = 0; i < vport->num_txq_grp; i++) {
> +		struct idpf_txq_group *txq_grp = &vport->txq_grps[i];
> +
> +		qtype = VIRTCHNL2_QUEUE_TYPE_TX;
> +
> +		for (j = 0; j < txq_grp->num_txq; j++, total++) {
> +			struct idpf_queue *txq = txq_grp->txqs[j];
> +
> +			if (!txq)
> +				idpf_add_empty_queue_stats(&data, qtype);
> +			else
> +				idpf_add_queue_stats(&data, txq);
> +		}
> +	}
> +
> +	vport_config = vport->adapter->vport_config[vport->idx];
> +	/* It is critical we provide a constant number of stats back to
> +	 * userspace regardless of how many queues are actually in use because
> +	 * there is no way to inform userspace the size has changed between
> +	 * ioctl calls. This will fill in any missing stats with zero.
> +	 */
> +	for (; total < vport_config->max_q.max_txq; total++)
> +		idpf_add_empty_queue_stats(&data, VIRTCHNL2_QUEUE_TYPE_TX);
> +	total = 0;
> +
> +	is_splitq = idpf_is_queue_model_split(vport->rxq_model);
> +
> +	for (i = 0; i < vport->num_rxq_grp; i++) {
> +		struct idpf_rxq_group *rxq_grp = &vport->rxq_grps[i];
> +		u16 num_rxq;
> +
> +		qtype = VIRTCHNL2_QUEUE_TYPE_RX;
> +
> +		if (is_splitq)
> +			num_rxq = rxq_grp->splitq.num_rxq_sets;
> +		else
> +			num_rxq = rxq_grp->singleq.num_rxq;
> +
> +		for (j = 0; j < num_rxq; j++, total++) {
> +			struct idpf_queue *rxq;
> +
> +			if (is_splitq)
> +				rxq = &rxq_grp->splitq.rxq_sets[j]->rxq;
> +			else
> +				rxq = rxq_grp->singleq.rxqs[j];
> +			if (!rxq)

Hi Alan, Tony, all,

Here it is assumed that rxq may be NULl...

> +				idpf_add_empty_queue_stats(&data, qtype);
> +			else
> +				idpf_add_queue_stats(&data, rxq);
> +
> +			/* In splitq mode, don't get page pool stats here since
> +			 * the pools are attached to the buffer queues
> +			 */
> +			if (is_splitq)
> +				continue;
> +
> +			page_pool_get_stats(rxq->pp, &pp_stats);

... but here rxq is dereferenced.

Flagged by Smatch.

> +		}
> +	}

...