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: <4e2ad62b-b11e-40db-9cd9-a26f7642c735@kernel.org>
Date: Fri, 30 Aug 2024 19:10:20 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>,
 Geert Uytterhoeven <geert@...ux-m68k.org>,
 "David S. Miller" <davem@...emloft.net>, Mark Brown <broonie@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Paolo Abeni <pabeni@...hat.com>
Cc: Daniel Mack <daniel@...que.org>, Haojian Zhuang
 <haojian.zhuang@...il.com>, Robert Jarzmik <robert.jarzmik@...e.fr>,
 linux-arm-kernel@...ts.infradead.org, linux-spi@...r.kernel.org,
 linux-kernel@...r.kernel.org, opensource.kernel@...o.com,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Yang Ruibin <11162571@...o.com>
Subject: Re: [PATCH v2] drivers: spi: Insert the missing pci_dev_put()before
 return

On 30/08/2024 10:55, Geert Uytterhoeven wrote:
> Hi Yang,
> 
> On Thu, Aug 29, 2024 at 5:35 AM Yang Ruibin <11162571@...o.com> wrote:
>> Increase the reference count by calling pci_get_slot(), and remember to
>> decrement the reference count by calling pci_dev_put().
>>
>> Signed-off-by: Yang Ruibin <11162571@...o.com>
> 
> Thanks for your patch, which is now commit 8a0ec8c2d736961f ("spi:
> Insert the missing pci_dev_put()before return") in spi/for-next.
> 
>> --- a/drivers/spi/spi-pxa2xx-pci.c
>> +++ b/drivers/spi/spi-pxa2xx-pci.c
>> @@ -146,8 +146,10 @@ static int lpss_spi_setup(struct pci_dev *dev, struct pxa2xx_spi_controller *c)
>>         c->num_chipselect = 1;
>>
>>         ret = pxa2xx_spi_pci_clk_register(dev, ssp, 50000000);
>> -       if (ret)
>> +       if (ret) {
>> +               pci_dev_put(dma_dev);
> 
> dma_dev is still uninitialized at this point.
> 
>>                 return ret;
>> +       }
>>
>>         dma_dev = pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> 
> dma_dev is initialized only here...
> 
>>         ret = devm_add_action_or_reset(&dev->dev, lpss_dma_put_device, dma_dev);
> 
> ... and freed automatically by lpss_dma_put_device() in case of
> any later failures since commit 609d7ffdc42199a0 ("spi: pxa2xx-pci:
> Balance reference count for PCI DMA device") in v5.18.
> 
>> @@ -222,8 +224,10 @@ static int mrfld_spi_setup(struct pci_dev *dev, struct pxa2xx_spi_controller *c)
>>         }
>>
>>         ret = pxa2xx_spi_pci_clk_register(dev, ssp, 25000000);
>> -       if (ret)
>> +       if (ret) {
>> +               pci_dev_put(dma_dev);
>>                 return ret;
>> +       }
>>
>>         dma_dev = pci_get_slot(dev->bus, PCI_DEVFN(21, 0));
>>         ret = devm_add_action_or_reset(&dev->dev, lpss_dma_put_device, dma_dev);
> 
> Likewise.
> 
> Hence this patch is not needed, and introduced two bugs.

Cc Greg, Jakub, David and Paolo,

It seems Vivo (at least two persons from vivo.com) is sending patches
generated through some sort of automation without really knowing what
they were doing. All of the patches look like innocent
cleanups/simplifications/fixes, but they do more.

This patch here looks like introducing two bugs.

These patches:
1. https://lore.kernel.org/all/20240830033251.232992-1-yujiaoliang@vivo.com/

2. https://lore.kernel.org/all/20240828122650.1324246-1-11162571@vivo.com/
(I sent a revert for this)

3. https://lore.kernel.org/all/20240829072016.2329466-1-11162571@vivo.com/

and probably more...

introduce dev_err_probe() outside of probe path which is not desired,
because it marks a probed (working) device as deferred.

The patches look trivial and/or helpful, so people tend to accept them
through default trust.

I kindly suggest reverse - do not trust them by default and instead do a
thorough review before accepting any cleanup/trivial patch from @vivo.com.

Best regards,
Krzysztof


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ