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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20230616234218.58760587@kernel.org>
Date: Fri, 16 Jun 2023 23:42:18 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: 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>,
 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, 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 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

> +	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.

> +init_free_q:
> +	kfree(cq);
> +	cq = NULL;

no need to clear local variables

> +	return err;
> +}

> +	int i = 0;
> +
> +	INIT_LIST_HEAD(&hw->cq_list_head);
> +
> +	for (i = 0; i < num_q; i++) {

init, again, please fix throughout

> +		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


> +void idpf_ctlq_deinit(struct idpf_hw *hw)
> +{
> +	struct idpf_ctlq_info *cq = NULL, *tmp = NULL;

You really like to init the stack :S

> +	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

> +	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

> +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?