Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 20 Jul 2021 22:37:57 +0800 (GMT+08:00)
From:   lyl2019@mail.ustc.edu.cn
To:     "Dongliang Mu" <mudongliangabcd@gmail.com>
Cc:     siglesias@igalia.com, jens.taprogge@taprogge.org,
        "Greg KH" <gregkh@linuxfoundation.org>,
        industrypack-devel@lists.sourceforge.net,
        linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: Doubts about Patch "ipack/carriers/tpci200: Fix a double free
 in tpci200_pci_probe"
In-Reply-To: <CAD-N9QVFnhKGfRYDDjWb0o_ty57AbjfzEnFPHZxEC5NOKL1ecg@mail.gmail.com>
References: <CAD-N9QVFnhKGfRYDDjWb0o_ty57AbjfzEnFPHZxEC5NOKL1ecg@mail.gmail.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=UTF-8
MIME-Version: 1.0
Message-ID: <77df3e1f.22e1.17ac45a1bd9.Coremail.lyl2019@mail.ustc.edu.cn>
Precedence: bulk


Hello Dongliang Mu,

>  Depending on if CONFIG_PCI defines, the "tpci200->info->cfg_regs" may
> not be freed.
> 
> #ifdef CONFIG_PCI
> /* Destroy a virtual mapping cookie for a PCI BAR (memory or IO) */
> struct pci_dev;
> extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
> #elif defined(CONFIG_GENERIC_IOMAP)
> struct pci_dev;
> static inline void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
> { }
> #endif

I think only `CONFIG_PCI=n` and `CONFIG_GENERIC_IOMAP=y` cause pci_iounmap an empty
implementation. Actually, `CONFIG_PCI` is a default option when run `make defconfig`,
pci_iounmap() usually is acted as an extern function.


> Even if CONFIG_PCI is undefined, it is possible that
> tpci200->info->cfg_regs is not freed at all. Therefore, this patch
> would cause memory leak. Take a look at the following code:
> 
> void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
> {
>         IO_COND(addr, /* nothing */, iounmap(addr));
> }

Here i am not sure this is the final implementation of pci_iounmap(),
because pci_iounmap() is re-implementated in many architectures.
Even so, i observed there still many call-sites of pci_iounmap() have reset
`the addr = NULL` after calling.
Can you have some ways to determine the actual implementation of 
pci_iounmap in our cases?


> #define IO_COND(addr, is_pio, is_mmio) do {                     \
>         unsigned long port = (unsigned long __force)addr;       \
>         if (port >= PIO_RESERVED) {                             \
>                 is_mmio;                                        \
>         } else if (port > PIO_OFFSET) {                         \
>                 port &= PIO_MASK;                               \
>                 is_pio;                                         \
>         } else                                                  \
>                 bad_io_access(port, #is_pio );                  \
> } while (0)
> 

Although the above codes is actually called, the addr might be freed 
if `port >= PIO_RESERVED` is true. The double free still existed.


If I make any mistakes, please tell me.
Thanks your report.
---
Lv Yunlong