| 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
| ||
|
Message-ID: <20260207022504.4018024-1-kuba@kernel.org>
Date: Fri, 6 Feb 2026 18:25:03 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: illusion.wang@...ula-matrix.com
Cc: Jakub Kicinski <kuba@...nel.org>,
sam.chen@...ula-matrix.com,
lukas.bulwahn@...hat.com,
dimon.zhao@...ula-matrix.com,
netdev@...r.kernel.org,
vadim.fedorenko@...ux.dev,
linux-kernel@...r.kernel.org (open list),
corbet@....net,
lorenzo@...nel.org,
alvin.wang@...ula-matrix.com,
andrew+netdev@...n.ch,
linux-doc@...r.kernel.org,
pabeni@...hat.com,
edumazet@...gle.com,
horms@...nel.org
Subject: Re: [v4,net-next,11/11] net/nebula-matrix: add common dev start/stop operation
This is an AI-generated review of your patch. The human sending this
email says "This is an automated AI code reviewer, please treat what
it says with a grain of salt."
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net/nebula-matrix: add common dev start/stop operation
This commit adds MSI-X interrupt initialization for the common device
layer, including configuring MSI-X vector mappings, allocating interrupt
resources, and registering mailbox interrupt handlers.
> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> index 9927dd3a1791..59d23e93e923 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
[ ... ]
> @@ -14,6 +25,179 @@ static void nbl_dev_register_common_irq(struct nbl_dev_mgt *dev_mgt)
> msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].num = 1;
> }
>
> +static int nbl_dev_request_mailbox_irq(struct nbl_dev_mgt *dev_mgt)
[ ... ]
> +static int nbl_dev_configure_msix_map(struct nbl_dev_mgt *dev_mgt)
> +{
> + struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
> + struct nbl_dev_common *dev_common = dev_mgt->common_dev;
> + struct nbl_msix_info *msix_info = &dev_common->msix_info;
> + bool mask_en = msix_info->serv_info[NBL_MSIX_NET_TYPE].hw_self_mask_en;
> + u16 msix_net_num = msix_info->serv_info[NBL_MSIX_NET_TYPE].num;
> + u16 msix_not_net_num = 0;
> + int err, i;
> +
> + for (i = NBL_MSIX_NET_TYPE; i < NBL_MSIX_TYPE_MAX; i++)
> + msix_info->serv_info[i].base_vector_id =
> + msix_info->serv_info[i - 1].base_vector_id +
> + msix_info->serv_info[i - 1].num;
^^^^^^
Does this loop cause an array underflow? NBL_MSIX_NET_TYPE equals 0 (from
the enum in nbl_dev.h), so on the first iteration i=0 and the code accesses
serv_info[-1].
The serv_info array is declared with NBL_MSIX_TYPE_MAX (2) elements in
struct nbl_msix_info, giving valid indices [0,1]. Accessing index -1 reads
uninitialized memory before the array start.
Call chain: nbl_core_start()->nbl_dev_start()->nbl_dev_start_common_dev()
->nbl_dev_configure_msix_map() guarantees this occurs during device start.
Should the loop start at i=1 (NBL_MSIX_MAILBOX_TYPE) instead, or should
serv_info[0].base_vector_id be initialized before the loop?
> +
> + for (i = NBL_MSIX_MAILBOX_TYPE; i < NBL_MSIX_TYPE_MAX; i++) {
[ ... ]
> +static int nbl_dev_alloc_msix_intr(struct nbl_dev_mgt *dev_mgt)
> +{
> + struct nbl_dev_common *dev_common = dev_mgt->common_dev;
> + struct nbl_msix_info *msix_info = &dev_common->msix_info;
> + struct nbl_common_info *common = dev_mgt->common;
> + int needed = 0;
> + int err;
> + int i;
> +
> + for (i = 0; i < NBL_MSIX_TYPE_MAX; i++)
> + needed += msix_info->serv_info[i].num;
> +
> + err = pci_alloc_irq_vectors(common->pdev, needed, needed,
> + PCI_IRQ_MSIX | PCI_IRQ_AFFINITY);
> + if (err < 0) {
> + pr_err("pci_enable_msix_range failed, err = %d.\n", err);
^^^^^^^^
This isn't a bug, but the error message says "pci_enable_msix_range failed"
while the actual function being called is pci_alloc_irq_vectors.
> + goto enable_msix_failed;
> + }
[ ... ]
> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> index a16a685e4f29..55ed560624d3 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> @@ -283,7 +283,38 @@ static struct pci_driver nbl_driver = {
> .remove = nbl_remove,
> };
>
> -module_pci_driver(nbl_driver);
> +static int __init nbl_module_init(void)
> +{
> + int status;
> +
> + status = nbl_common_create_wq();
> + if (status) {
> + pr_err("Failed to create wq, err = %d\n", status);
> + goto wq_create_failed;
> + }
> + status = pci_register_driver(&nbl_driver);
> + if (status) {
> + pr_err("Failed to register PCI driver, err = %d\n", status);
> + goto pci_register_driver_failed;
> + }
> + pr_info("nbl module loaded\n");
> + return 0;
Previous review feedback from Andrew Lunn on v3 noted that the config is
named 'NBL_CORE' and the module is named 'nbl_core'. He pointed out that
'_CORE' naming is unusual for a driver - CORE is typically a library shared
by multiple drivers. The Kconfig still shows:
config NBL_CORE
tristate "Nebula-matrix Ethernet Controller m18100 Family support"
Should this be renamed to follow the naming convention for standalone
drivers?
Link: https://lore.kernel.org/netdev/88c7e349-f941-4216-89ca-70a49e82b378@lunn.ch/
Andrew Lunn also noted in v3 that variable declarations should follow
reverse Christmas tree ordering (longest line first). Looking at functions
in this commit like nbl_dev_request_mailbox_irq():
struct nbl_dev_common *dev_common = dev_mgt->common_dev;
struct nbl_msix_info *msix_info = &dev_common->msix_info;
struct nbl_common_info *common = dev_mgt->common;
the declarations are not in reverse Christmas tree order.
Link: https://lore.kernel.org/netdev/c6d37ecd-7dc6-4f83-b76c-2735a5f116fd@lunn.ch/
Powered by blists - more mailing lists