netdev - Re: [PATCH net-next 5/7] ice: Accumulate ring statistics over reset

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] [day] [month] [year] [list]

Message-ID: <8d792d9e-3f58-51c4-0501-325b0f07e716@intel.com>
Date:   Wed, 16 Nov 2022 15:34:15 -0800
From:   Benjamin Mikailenko <benjamin.mikailenko@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     <davem@...emloft.net>, <pabeni@...hat.com>, <edumazet@...gle.com>,
        <netdev@...r.kernel.org>, Gurucharan G <gurucharanx.g@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
Subject: Re: [PATCH net-next 5/7] ice: Accumulate ring statistics over reset



On 11/15/2022 9:02 PM, Jakub Kicinski wrote:
> On Mon, 14 Nov 2022 15:42:48 -0800 Tony Nguyen wrote:
>> +static int ice_vsi_alloc_stat_arrays(struct ice_vsi *vsi)
>> +{
>> +	struct ice_vsi_stats *vsi_stat;
>> +	struct ice_pf *pf = vsi->back;
>> +	struct device *dev;
>> +
>> +	dev = ice_pf_to_dev(pf);
>> +
>> +	if (vsi->type == ICE_VSI_CHNL)
>> +		return 0;
>> +	if (!pf->vsi_stats)
>> +		return -ENOENT;
>> +
>> +	vsi_stat = devm_kzalloc(dev, sizeof(*vsi_stat), GFP_KERNEL);
> 
> Don't use managed allocations if the structure's lifetime does not
> match that of the driver instance. It's not generic garbage collection.
> 
>> +	if (!vsi_stat)
>> +		return -ENOMEM;
>> +
>> +	vsi_stat->tx_ring_stats =
>> +		devm_kcalloc(dev, vsi->alloc_txq,
>> +			     sizeof(*vsi_stat->tx_ring_stats), GFP_KERNEL);
>> +
>> +	vsi_stat->rx_ring_stats =
>> +		devm_kcalloc(dev, vsi->alloc_rxq,
>> +			     sizeof(*vsi_stat->rx_ring_stats), GFP_KERNEL);
>> +
>> +	if (!vsi_stat->tx_ring_stats || !vsi_stat->rx_ring_stats)
>> +		goto err_alloc;
>> +
>> +	pf->vsi_stats[vsi->idx] = vsi_stat;
>> +
>> +	return 0;
>> +
>> +err_alloc:
> 
> Don't do combined error checking, add appropriate labels for each case.
> 
>> +	devm_kfree(dev, vsi_stat->tx_ring_stats);
>> +	vsi_stat->tx_ring_stats = NULL;
> 
> No need to clear the pointers, vsi_stats is freed few lines down.
> 
>> +	devm_kfree(dev, vsi_stat->rx_ring_stats);
>> +	vsi_stat->rx_ring_stats = NULL;
>> +	devm_kfree(dev, vsi_stat);
>> +	pf->vsi_stats[vsi->idx] = NULL;
>> +	return -ENOMEM;
>> +}
>> +
>>  /**
>>   * ice_vsi_alloc - Allocates the next available struct VSI in the PF
>>   * @pf: board private structure
>> @@ -560,6 +606,11 @@ ice_vsi_alloc(struct ice_pf *pf, enum ice_vsi_type vsi_type,
>>  
>>  	if (vsi->type == ICE_VSI_CTRL && vf)
>>  		vf->ctrl_vsi_idx = vsi->idx;
>> +
>> +	/* allocate memory for Tx/Rx ring stat pointers */
>> +	if (ice_vsi_alloc_stat_arrays(vsi))
>> +		goto err_rings;
>> +
>>  	goto unlock_pf;
>>  
>>  err_rings:
>> @@ -1535,6 +1586,122 @@ static int ice_vsi_alloc_rings(struct ice_vsi *vsi)
>>  	return -ENOMEM;
>>  }
>>  
>> +/**
>> + * ice_vsi_free_stats - Free the ring statistics structures
>> + * @vsi: VSI pointer
>> + */
>> +static void ice_vsi_free_stats(struct ice_vsi *vsi)
>> +{
>> +	struct ice_vsi_stats *vsi_stat;
>> +	struct ice_pf *pf = vsi->back;
>> +	struct device *dev;
>> +	int i;
>> +
>> +	dev = ice_pf_to_dev(pf);
>> +
>> +	if (vsi->type == ICE_VSI_CHNL)
>> +		return;
>> +	if (!pf->vsi_stats)
>> +		return;
>> +
>> +	vsi_stat = pf->vsi_stats[vsi->idx];
>> +	if (!vsi_stat)
>> +		return;
>> +
>> +	ice_for_each_alloc_txq(vsi, i) {
>> +		if (vsi_stat->tx_ring_stats[i]) {
>> +			kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
>> +			WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
>> +		}
>> +	}
>> +
>> +	ice_for_each_alloc_rxq(vsi, i) {
>> +		if (vsi_stat->rx_ring_stats[i]) {
>> +			kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
>> +			WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
>> +		}
>> +	}
>> +
>> +	devm_kfree(dev, vsi_stat->tx_ring_stats);
>> +	vsi_stat->tx_ring_stats = NULL;
>> +	devm_kfree(dev, vsi_stat->rx_ring_stats);
>> +	vsi_stat->rx_ring_stats = NULL;
>> +	devm_kfree(dev, vsi_stat);
>> +	pf->vsi_stats[vsi->idx] = NULL;
>> +}
>> +
>> +/**
>> + * ice_vsi_alloc_ring_stats - Allocates Tx and Rx ring stats for the VSI
>> + * @vsi: VSI which is having stats allocated
>> + */
>> +static int ice_vsi_alloc_ring_stats(struct ice_vsi *vsi)
>> +{
>> +	struct ice_ring_stats **tx_ring_stats;
>> +	struct ice_ring_stats **rx_ring_stats;
>> +	struct ice_vsi_stats *vsi_stats;
>> +	struct ice_pf *pf = vsi->back;
>> +	u16 i;
>> +
>> +	if (!pf->vsi_stats)
>> +		return -ENOENT;
>> +
>> +	vsi_stats = pf->vsi_stats[vsi->idx];
>> +	if (!vsi_stats)
>> +		return -ENOENT;
>> +
>> +	tx_ring_stats = vsi_stats->tx_ring_stats;
>> +	if (!tx_ring_stats)
>> +		return -ENOENT;
>> +
>> +	rx_ring_stats = vsi_stats->rx_ring_stats;
>> +	if (!rx_ring_stats)
>> +		return -ENOENT;
>> +
> 
> Why all this NULL-checking? The init should have failed if any of these
> are NULL. Please avoid defensive programming.

Hi Jakub!

Thanks for the review.
I fixed the unnecessary managed allocations and null checks.

Sending v6.

Thank you,
Ben Mikailenko