| Message ID | 20230605-txgbe-wake-v1-1-ea6c441780f9@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: txgbe: Avoid passing uninitialised parameter to pci_wake_from_d3() | expand |
On Mon, Jun 05, 2023 at 04:20:28PM +0200, Simon Horman wrote: > txgbe_shutdown() relies on txgbe_dev_shutdown() to initialise > wake by passing it by reference. However, txgbe_dev_shutdown() > doesn't use this parameter at all. > > wake is then passed uninitialised by txgbe_dev_shutdown() > to pci_wake_from_d3(). > > Resolve this problem by: > * Removing the unused parameter from txgbe_dev_shutdown() > * Removing the uninitialised variable wake from txgbe_dev_shutdown() > * Passing false to pci_wake_from_d3() - this assumes that > although uninitialised wake was in practice false (0). > > I'm not sure that this counts as a bug, as I'm not sure that > it manifests in any unwanted behaviour. But in any case, the issue > was introduced by: > > bbd22f34b47c ("net: txgbe: Avoid passing uninitialised parameter to pci_wake_from_d3()") > > Flagged by Smatch as: > > .../txgbe_main.c:486 txgbe_shutdown() error: uninitialized symbol 'wake'. > > No functional change intended. > Compile tested only. > Almost everyone turns on the AUTO_VAR_INIT stuff these days so that's why we don't see these bugs in testing but they are still bugs and they will affect people who have that turned off. CONFIG_CC_HAS_AUTO_VAR_INIT_PATTERN=y CONFIG_CC_HAS_AUTO_VAR_INIT_ZERO_BARE=y CONFIG_CC_HAS_AUTO_VAR_INIT_ZERO=y # CONFIG_INIT_STACK_NONE is not set CONFIG_INIT_STACK_ALL_PATTERN=y # CONFIG_INIT_STACK_ALL_ZERO is not set CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y CONFIG_INIT_ON_FREE_DEFAULT_ON=y I did report this bug, but I only sent it to the original author and I CC'd kernel-janitors. In 2009 when Smatch was quite new the netdev list was annoyed by static analysis bug reports so I stopped CCing netdev. https://lore.kernel.org/all/YsWWXYal9ZwmIo2G@kili/ regards, dan carpenter
On Mon, Jun 05, 2023 at 04:20:28PM +0200, Simon Horman wrote: Hey Simon, > txgbe_shutdown() relies on txgbe_dev_shutdown() to initialise > wake by passing it by reference. However, txgbe_dev_shutdown() > doesn't use this parameter at all. > > wake is then passed uninitialised by txgbe_dev_shutdown() > to pci_wake_from_d3(). > > Resolve this problem by: > * Removing the unused parameter from txgbe_dev_shutdown() > * Removing the uninitialised variable wake from txgbe_dev_shutdown() > * Passing false to pci_wake_from_d3() - this assumes that > although uninitialised wake was in practice false (0). > > I'm not sure that this counts as a bug, as I'm not sure that > it manifests in any unwanted behaviour. But in any case, the issue > was introduced by: > > bbd22f34b47c ("net: txgbe: Avoid passing uninitialised parameter to pci_wake_from_d3()") wait, you are pointing to your own commit here? this supposed to be: 3ce7547e5b71 net: txgbe: Add build support for txgbe no? > > Flagged by Smatch as: > > .../txgbe_main.c:486 txgbe_shutdown() error: uninitialized symbol 'wake'. > > No functional change intended. > Compile tested only. > > Signed-off-by: Simon Horman <horms@kernel.org> > --- > drivers/net/ethernet/wangxun/txgbe/txgbe_main.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c > index 0f0d9fa1cde1..cfe47f3d2503 100644 > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c > @@ -457,7 +457,7 @@ static int txgbe_close(struct net_device *netdev) > return 0; > } > > -static void txgbe_dev_shutdown(struct pci_dev *pdev, bool *enable_wake) > +static void txgbe_dev_shutdown(struct pci_dev *pdev) > { > struct wx *wx = pci_get_drvdata(pdev); > struct net_device *netdev; > @@ -477,12 +477,10 @@ static void txgbe_dev_shutdown(struct pci_dev *pdev, bool *enable_wake) > > static void txgbe_shutdown(struct pci_dev *pdev) > { > - bool wake; > - > - txgbe_dev_shutdown(pdev, &wake); > + txgbe_dev_shutdown(pdev); > > if (system_state == SYSTEM_POWER_OFF) { > - pci_wake_from_d3(pdev, wake); > + pci_wake_from_d3(pdev, false); > pci_set_power_state(pdev, PCI_D3hot); > } > } > >
On Mon, Jun 05, 2023 at 08:22:38PM +0200, Maciej Fijalkowski wrote: > On Mon, Jun 05, 2023 at 04:20:28PM +0200, Simon Horman wrote: > > Hey Simon, > > > txgbe_shutdown() relies on txgbe_dev_shutdown() to initialise > > wake by passing it by reference. However, txgbe_dev_shutdown() > > doesn't use this parameter at all. > > > > wake is then passed uninitialised by txgbe_dev_shutdown() > > to pci_wake_from_d3(). > > > > Resolve this problem by: > > * Removing the unused parameter from txgbe_dev_shutdown() > > * Removing the uninitialised variable wake from txgbe_dev_shutdown() > > * Passing false to pci_wake_from_d3() - this assumes that > > although uninitialised wake was in practice false (0). > > > > I'm not sure that this counts as a bug, as I'm not sure that > > it manifests in any unwanted behaviour. But in any case, the issue > > was introduced by: > > > > bbd22f34b47c ("net: txgbe: Avoid passing uninitialised parameter to pci_wake_from_d3()") > > wait, you are pointing to your own commit here? > > this supposed to be: > 3ce7547e5b71 net: txgbe: Add build support for txgbe > > no? Yes, sorry about that. Will fix in a v2. ...
On Monday, June 5, 2023 10:20 PM, Simon Horman wrote: > txgbe_shutdown() relies on txgbe_dev_shutdown() to initialise > wake by passing it by reference. However, txgbe_dev_shutdown() > doesn't use this parameter at all. > > wake is then passed uninitialised by txgbe_dev_shutdown() > to pci_wake_from_d3(). > > Resolve this problem by: > * Removing the unused parameter from txgbe_dev_shutdown() > * Removing the uninitialised variable wake from txgbe_dev_shutdown() > * Passing false to pci_wake_from_d3() - this assumes that > although uninitialised wake was in practice false (0). > > I'm not sure that this counts as a bug, as I'm not sure that > it manifests in any unwanted behaviour. But in any case, the issue > was introduced by: Thanks for the fix. These are some redundant codes for a feature that has not yet been implemented. Reviewed-by: Jiawen Wu <jiawenwu@trustnetic.com> > > bbd22f34b47c ("net: txgbe: Avoid passing uninitialised parameter to pci_wake_from_d3()") > > Flagged by Smatch as: > > .../txgbe_main.c:486 txgbe_shutdown() error: uninitialized symbol 'wake'. > > No functional change intended. > Compile tested only. > > Signed-off-by: Simon Horman <horms@kernel.org> > --- > drivers/net/ethernet/wangxun/txgbe/txgbe_main.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c > index 0f0d9fa1cde1..cfe47f3d2503 100644 > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c > @@ -457,7 +457,7 @@ static int txgbe_close(struct net_device *netdev) > return 0; > } > > -static void txgbe_dev_shutdown(struct pci_dev *pdev, bool *enable_wake) > +static void txgbe_dev_shutdown(struct pci_dev *pdev) > { > struct wx *wx = pci_get_drvdata(pdev); > struct net_device *netdev; > @@ -477,12 +477,10 @@ static void txgbe_dev_shutdown(struct pci_dev *pdev, bool *enable_wake) > > static void txgbe_shutdown(struct pci_dev *pdev) > { > - bool wake; > - > - txgbe_dev_shutdown(pdev, &wake); > + txgbe_dev_shutdown(pdev); > > if (system_state == SYSTEM_POWER_OFF) { > - pci_wake_from_d3(pdev, wake); > + pci_wake_from_d3(pdev, false); > pci_set_power_state(pdev, PCI_D3hot); > } > }
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c index 0f0d9fa1cde1..cfe47f3d2503 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c @@ -457,7 +457,7 @@ static int txgbe_close(struct net_device *netdev) return 0; } -static void txgbe_dev_shutdown(struct pci_dev *pdev, bool *enable_wake) +static void txgbe_dev_shutdown(struct pci_dev *pdev) { struct wx *wx = pci_get_drvdata(pdev); struct net_device *netdev; @@ -477,12 +477,10 @@ static void txgbe_dev_shutdown(struct pci_dev *pdev, bool *enable_wake) static void txgbe_shutdown(struct pci_dev *pdev) { - bool wake; - - txgbe_dev_shutdown(pdev, &wake); + txgbe_dev_shutdown(pdev); if (system_state == SYSTEM_POWER_OFF) { - pci_wake_from_d3(pdev, wake); + pci_wake_from_d3(pdev, false); pci_set_power_state(pdev, PCI_D3hot); } }
txgbe_shutdown() relies on txgbe_dev_shutdown() to initialise wake by passing it by reference. However, txgbe_dev_shutdown() doesn't use this parameter at all. wake is then passed uninitialised by txgbe_dev_shutdown() to pci_wake_from_d3(). Resolve this problem by: * Removing the unused parameter from txgbe_dev_shutdown() * Removing the uninitialised variable wake from txgbe_dev_shutdown() * Passing false to pci_wake_from_d3() - this assumes that although uninitialised wake was in practice false (0). I'm not sure that this counts as a bug, as I'm not sure that it manifests in any unwanted behaviour. But in any case, the issue was introduced by: bbd22f34b47c ("net: txgbe: Avoid passing uninitialised parameter to pci_wake_from_d3()") Flagged by Smatch as: .../txgbe_main.c:486 txgbe_shutdown() error: uninitialized symbol 'wake'. No functional change intended. Compile tested only. Signed-off-by: Simon Horman <horms@kernel.org> --- drivers/net/ethernet/wangxun/txgbe/txgbe_main.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)