netdev - Re: [PATCH net-next v2 1/4] octeon_ep: add PF-VF mailbox communication

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: <20231210113732.GF5817@kernel.org>
Date: Sun, 10 Dec 2023 11:37:32 +0000
From: Simon Horman <horms@...nel.org>
To: Shinas Rasheed <srasheed@...vell.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, hgani@...vell.com,
	vimleshk@...vell.com, egallen@...hat.com, mschmidt@...hat.com,
	pabeni@...hat.com, kuba@...nel.org, davem@...emloft.net,
	wizhao@...hat.com, kheib@...hat.com, konguyen@...hat.com,
	Veerasenareddy Burru <vburru@...vell.com>,
	Sathesh Edara <sedara@...vell.com>,
	Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next v2 1/4] octeon_ep: add PF-VF mailbox
 communication

On Sat, Dec 09, 2023 at 12:14:47AM -0800, Shinas Rasheed wrote:
> Implement mailbox communication between PF and VFs.
> PF-VF mailbox is used for all control commands from VF to PF and
> asynchronous notification messages from PF to VF.
> 
> Signed-off-by: Shinas Rasheed <srasheed@...vell.com>

...

> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c

...

> @@ -1315,6 +1335,7 @@ static void octep_device_cleanup(struct octep_device *oct)
>  		oct->mbox[i] = NULL;
>  	}
>  
> +	octep_delete_pfvf_mbox(oct);
>  	octep_ctrl_net_uninit(oct);
>  	cancel_delayed_work_sync(&oct->hb_task);
>  
> @@ -1411,6 +1432,13 @@ static int octep_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto err_octep_config;
>  	}
>  
> +	err = octep_setup_pfvf_mbox(octep_dev);
> +	if (err) {
> +		dev_err(&pdev->dev, " pfvf mailbox setup failed\n");
> +		octep_ctrl_net_uninit(octep_dev);
> +		return err;

Hi Shinas,

This seems inconsistent with other error handling in this function, I
suspect it is leaking resources. And even if it does not, it's likely to
lead to such problems as this function is updated in future.  Please
consider consistently handling error unwinding using goto labels in this
function.

I think that means either making sure that octep_delete_pfvf_mbox() can
handle the case whereby octep_setup_pfvf_mbox() fails. Or including) the
steps taken by octep_device_cleanup() in the unwind ladder provided by goto
labels (which could be a separate patch, and I suspect to be the best
approach). But I could well be wrong.

> +	}
> +
>  	octep_ctrl_net_get_info(octep_dev, OCTEP_CTRL_NET_INVALID_VFID,
>  				&octep_dev->conf->fw_info);
>  	dev_info(&octep_dev->pdev->dev, "Heartbeat interval %u msecs Heartbeat miss count %u\n",

...