netdev - Re: [PATCH net-next v2 03/15] idpf: add controlq init and reset checks

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0b6fa05f-d357-2942-d17b-d24d8a5a3321@intel.com>
Date: Wed, 21 Jun 2023 12:05:54 -0700
From: "Linga, Pavan Kumar" <pavan.kumar.linga@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>
CC: <davem@...emloft.net>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<netdev@...r.kernel.org>, Joshua Hay <joshua.a.hay@...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>, Alan Brady <alan.brady@...el.com>, "Madhu
 Chittim" <madhu.chittim@...el.com>, Phani Burra <phani.r.burra@...el.com>,
	Shailendra Bhatnagar <shailendra.bhatnagar@...el.com>
Subject: Re: [PATCH net-next v2 03/15] idpf: add controlq init and reset
 checks



On 6/16/2023 11:42 PM, Jakub Kicinski wrote:
> On Wed, 14 Jun 2023 10:14:16 -0700 Tony Nguyen wrote:
>> +static void idpf_ctlq_init_rxq_bufs(struct idpf_ctlq_info *cq)
>> +{
>> +	int i = 0;
>> +
>> +	for (i = 0; i < cq->ring_size; i++) {
> 
> no need to init i twice
> 

Will fix it.

>> +	if (!qinfo->len || !qinfo->buf_size ||
>> +	    qinfo->len > IDPF_CTLQ_MAX_RING_SIZE ||
>> +	    qinfo->buf_size > IDPF_CTLQ_MAX_BUF_LEN)
>> +		return -EINVAL;
> 
> Looks like defensive programming, it's generally discouraged in
> the kernel.
> 

Thanks for the suggestion, will fix it.

>> +init_free_q:
>> +	kfree(cq);
>> +	cq = NULL;
> 
> no need to clear local variables
> 

Will fix it.

>> +	return err;
>> +}
> 
>> +	int i = 0;
>> +
>> +	INIT_LIST_HEAD(&hw->cq_list_head);
>> +
>> +	for (i = 0; i < num_q; i++) {
> 
> init, again, please fix throughout
> 

Sure, will go through all the instances.

>> +		struct idpf_ctlq_create_info *qinfo = q_info + i;
>> +
>> +		err = idpf_ctlq_add(hw, qinfo, &cq);
>> +		if (err)
>> +			goto init_destroy_qs;
>> +	}
>> +
>> +	return err;
> 
> return 0 is more idiomatic, you can't reach it with an errno
> 
> 

Will fix it.

>> +void idpf_ctlq_deinit(struct idpf_hw *hw)
>> +{
>> +	struct idpf_ctlq_info *cq = NULL, *tmp = NULL;
> 
> You really like to init the stack :S
> 

Will fix it.

>> +	list_for_each_entry_safe(cq, tmp, &hw->cq_list_head, cq_list)
>> +		idpf_ctlq_remove(hw, cq);
>> +}
> 
>> +	if (!cq || !cq->ring_size)
>> +		return -ENOBUFS;
> 
> even worse defensive programming
> 

Will fix it.

>> +	mutex_lock(&cq->cq_lock);
>> +
>> +	/* Ensure there are enough descriptors to send all messages */
>> +	num_desc_avail = IDPF_CTLQ_DESC_UNUSED(cq);
>> +	if (num_desc_avail == 0 || num_desc_avail < num_q_msg) {
>> +		err = -ENOSPC;
>> +		goto sq_send_command_out;
> 
> name labels after what they jump to, err_unlock
> 

Will fix it.

>> +void *idpf_alloc_dma_mem(struct idpf_hw *hw, struct idpf_dma_mem *mem, u64 size)
>> +{
>> +	struct idpf_adapter *adapter = hw->back;
>> +	size_t sz = ALIGN(size, 4096);
>> +
>> +	mem->va = dma_alloc_coherent(&adapter->pdev->dev, sz,
>> +				     &mem->pa, GFP_KERNEL | __GFP_ZERO);
> 
> DMA API always zeros memory, I thought cocci warns about this
> Did you run cocci checks?
> 

I ran cocci check using the command "make -j8 
M=drivers/net/ethernet/intel/idpf/ C=1 CHECK="scripts/coccicheck" 
CONFIG_IDPF=m &>>err.log" but didn't see any hits and not sure if this 
is the right command to see the warning. Will fix it anyways.

Thanks,
Pavan