linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  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).