linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: John Garry <john.garry@huawei.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	gregkh@linuxfoundation.org, rafael@kernel.org,
	martin.petersen@oracle.com, jejb@linux.ibm.com,
	linuxarm@huawei.com, linux-scsi@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity_desc()
Date: Tue, 24 Nov 2020 16:51:59 +0000	[thread overview]
Message-ID: <0525a4bcf17a355cd141632d4f3714be@kernel.org> (raw)
In-Reply-To: <5a314713-c1ee-2d34-bee1-60beae274742@huawei.com>

On 2020-11-23 15:45, John Garry wrote:

Hi John,

>>> But it looks like there is more to it than that, which I'm worried is
>>> far from non-trivial. For example, just calling irq_dispose_mapping()
>>> for removal and then plaform_get_irq()->acpi_get_irq() second time
>>> fails as it looks like more tidy-up is needed for removal...
>> 
>> Most probably. I could imagine things failing if there is any trace
>> of an existing translation in the ITS or in the platform-MSI layer,
>> for example, or if the interrupt is still active...
> 
> So this looks to be a problem I have. So if I hack the code to skip
> the check in acpi_get_irq() for the irq already being init'ed, I run
> into a use-after-free in the gic-v3-its driver. I may be skipping
> something with this hack, but I'll ask anyway.
> 
> So initially in the msi_prepare method we setup the its dev - this is
> from the mbigen probe. Then when all the irqs are unmapped later for
> end device driver removal, we release this its device in
> its_irq_domain_free(). But I don't see anything to set it up again. Is
> it improper to have released the its device in this scenario?
> Commenting out the release makes things "good" again.

Huh, that's ugly. The issue is that the device that deals with the
interrupts isn't the device that the ITS knows about (there isn't a
1:1 mapping between mbigen and the endpoint).

The mbigen is responsible for the creation of the corresponding
irqdomain, and and crucially for the "prepare" phase, which results
in storing the its_dev pointer in info->scratchpad[0].

As we free all the interrupts associated with the endpoint, we
free the its_dev (nothing else needs it at this point). On the
next allocation, we reuse the damn its_dev pointer, and we're SOL.
This is wrong, because we haven't removed the mbigen, only the
device *connected* to the mbigen. And since the mbigen can be shared
across endpoints, we can't reliably tear it down at all. Boo.

The only thing to do is to convey that by marking the its_dev as
shared so that it isn't deleted when no LPIs are being used. After
all, it isn't like the mbigen is going anywhere.

It is just that passing that information down isn't a simple affair,
as msi_alloc_info_t isn't a generic type... Let me have a think.

         M.
-- 
Jazz is not dead. It just smells funny...

  reply	other threads:[~2020-11-24 16:52 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-28 12:33 [PATCH v2 0/3] Support managed interrupts for platform devices John Garry
2020-10-28 12:33 ` [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity_desc() John Garry
2020-10-28 18:22   ` Thomas Gleixner
2020-11-02 17:32     ` John Garry
2020-11-02 20:35       ` Thomas Gleixner
2020-11-17 21:28         ` Thomas Gleixner
2020-11-18 11:34           ` John Garry
2020-11-18 20:38             ` Thomas Gleixner
2020-11-19  9:31               ` John Garry
2020-11-19 18:09                 ` Thomas Gleixner
2020-11-19 19:56                   ` John Garry
2020-11-19 21:03                     ` Thomas Gleixner
2020-11-20 11:52                       ` John Garry
2020-11-22 13:38                         ` Marc Zyngier
2020-11-23 12:54                           ` John Garry
2020-11-23 13:26                             ` Marc Zyngier
2020-11-23 15:45                               ` John Garry
2020-11-24 16:51                                 ` Marc Zyngier [this message]
2020-11-24 17:38                                   ` John Garry
2020-11-25 18:35                                     ` Marc Zyngier
2020-11-26 10:47                                       ` John Garry
2020-11-26 11:08                                         ` Marc Zyngier
2020-11-26 11:29                                           ` John Garry
2020-11-26 16:52                                             ` John Garry
2020-11-27  9:57                                               ` Marc Zyngier
2020-11-27 12:45                                                 ` John Garry
2020-11-27 12:49                                                   ` Marc Zyngier
2020-10-28 12:33 ` [PATCH v2 2/3] Driver core: platform: Add platform_get_irqs_affinity() John Garry
2020-10-28 12:33 ` [PATCH v2 3/3] scsi: hisi_sas: Expose HW queues for v2 hw John Garry

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=0525a4bcf17a355cd141632d4f3714be@kernel.org \
    --to=maz@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jejb@linux.ibm.com \
    --cc=john.garry@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=martin.petersen@oracle.com \
    --cc=rafael@kernel.org \
    --cc=tglx@linutronix.de \
    /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).