From: Bjorn Helgaas <helgaas@kernel.org>
To: "Slivka, Danijel" <Danijel.Slivka@amd.com>
Cc: "bhelgaas@google.com" <bhelgaas@google.com>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"Krzysztof Wilczyński" <kw@linux.com>
Subject: Re: [PATCH] PCI: Fix accessing freed memory in pci_remove_resource_files
Date: Tue, 26 Oct 2021 17:12:27 -0500 [thread overview]
Message-ID: <20211026221227.GA172193@bhelgaas> (raw)
In-Reply-To: <20210510225733.GA2307664@bjorn-Precision-5520>
On Mon, May 10, 2021 at 05:57:33PM -0500, Bjorn Helgaas wrote:
> On Mon, May 10, 2021 at 03:02:45PM +0000, Slivka, Danijel wrote:
> > Hi Bjorn,
> >
> > Yes, I get segmentation fault on unloading custom module during the
> > check of error handling case.
> >
> > There is no directly visible access to res_attr fields, as you
> > mentioned, other than the one in a call chain after a check in
> > pci_remove_resource_files() which seems to cause the issue
> > (accessing name).
> >
> > Load and unload module will invoke pci_enable_sriov() and
> > disable_pci_sriov() respectively that are both having a call of
> > pci_remove_resource_files() in call chain.
> >
> > In that function existing check is not behaving as expected since
> > memory is freed but pointers left dangling.
> >
> > Below is call trace and detail description.
> >
> > During loading of module pci_enable_sriov() is called, I have
> > following invoking sequence:
> >
> > device_create_file
> > pci_create_capabilities_sysfs
> > pci_create_sysfs_dev_files
> > pci_bus_add_device
> > pci_iov_add_virtfn
> > sriov_enable
> > pci_enable_sriov
>
> OK. For anybody following along, this call path changed in v5.13-rc1,
> so pci_create_capabilities_sysfs() longer exists. But looking at
> v5.12, I think the sequence you're seeing is:
>
> pci_create_sysfs_dev_files
> pci_create_capabilities_sysfs
> retval = device_create_file(&dev->dev, &dev_attr_reset)
> return retval # I guess this what failed, right?
> if (retval) goto err_rom_file
> err_rom_file:
> ...
> pci_remove_resource_files
> sysfs_remove_bin_file(pdev->res_attr[i])
> kfree(pdev->res_attr[i])
> # pdev->res_attr[i] not set to NULL in v5.12
>
> Later, on module unload, we have this sequence:
>
> pci_disable_sriov
> sriov_disable
> sriov_del_vfs
> pci_iov_remove_virtfn
> pci_stop_and_remove_bus_device
> pci_stop_bus_device
> pci_stop_dev
> pci_remove_sysfs_dev_files
> pci_remove_resource_files
> sysfs_remove_bin_file(pdev->res_attr[i])
> # pdev->res_attr[i] points to a freed object
>
> Definitely seems like a problem. Hmmm. I'm not really a fan of
> checking the pointer to see whether it's been freed. That seems like
> a band-aid.
This patch does:
+ pdev->res_attr[i] = NULL;
+ pdev->res_attr_wc[i] = NULL;
which I think essentially relies on the fact that kfree(NULL) is a
no-op.
I'm going to drop this for now because I don't want to rely on that.
I'd rather avoid doing the kfree() altogether.
IIRC this happens when device_create_file() fails, and it likely fails
because of a race when creating the sysfs files, which would explain
why we don't see lots of reports of this.
Bjorn
next prev parent reply other threads:[~2021-10-26 22:12 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-07 10:27 [PATCH] PCI: Fix accessing freed memory in pci_remove_resource_files Danijel Slivka
2021-05-07 22:07 ` Bjorn Helgaas
2021-05-10 15:02 ` Slivka, Danijel
2021-05-10 22:57 ` Bjorn Helgaas
2021-05-25 22:54 ` Krzysztof Wilczyński
2021-06-03 18:37 ` Krzysztof Wilczyński
2021-10-26 22:12 ` Bjorn Helgaas [this message]
2021-05-10 15:08 ` Slivka, Danijel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211026221227.GA172193@bhelgaas \
--to=helgaas@kernel.org \
--cc=Danijel.Slivka@amd.com \
--cc=bhelgaas@google.com \
--cc=kw@linux.com \
--cc=linux-pci@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).