Re: [PATCH 17/21] MSI: Clear the irq_desc's msi pointer on free

Re: [PATCH 17/21] MSI: Clear the irq_desc's msi pointer on free

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

> Currently we never clear the msi_desc pointer in the irq_desc. This
> leaves us with a pointer to free'ed memory hanging around. No one seems
> to have hit this, so presumably other parts of the code are protecting
> us from ever using the stale pointer .. or we're just lucky, we should
> still clear it.

Hmm.  Maybe.  Currently this is done in dynamic_irq_cleanup,
at least for everything except sparc64.

>
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> ---
>
>  drivers/pci/msi.c |    1 +
>  1 file changed, 1 insertion(+)
>
> Index: msi-new/drivers/pci/msi.c
> ===================================================================
> --- msi-new.orig/drivers/pci/msi.c
> +++ msi-new/drivers/pci/msi.c
> @@ -520,6 +520,7 @@ static int msi_free_irq(struct pci_dev* 
>  	list_del(&entry->list);
>  
>  	arch_teardown_msi_irq(irq);
> +	set_irq_msi(irq, NULL);
>  	kfree(entry);
>  
>  	if (type == PCI_CAP_ID_MSIX && list_empty(&dev->msi_list))

Re: [PATCH 17/21] MSI: Clear the irq_desc's msi pointer on free

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

On Thu, 2007-03-22 at 08:23 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
> 
> > Currently we never clear the msi_desc pointer in the irq_desc. This
> > leaves us with a pointer to free'ed memory hanging around. No one seems
> > to have hit this, so presumably other parts of the code are protecting
> > us from ever using the stale pointer .. or we're just lucky, we should
> > still clear it.
> 
> Hmm.  Maybe.  Currently this is done in dynamic_irq_cleanup,
> at least for everything except sparc64.

OK, I missed that. I still think we should do it here, otherwise there's
a window, however small, where the msi_desc pointer is pointing at freed
memory.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 17/21] MSI: Clear the irq_desc's msi pointer on free

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

> On Thu, 2007-03-22 at 08:23 -0600, Eric W. Biederman wrote:
>> Michael Ellerman <michael@ellerman.id.au> writes:
>> 
>> > Currently we never clear the msi_desc pointer in the irq_desc. This
>> > leaves us with a pointer to free'ed memory hanging around. No one seems
>> > to have hit this, so presumably other parts of the code are protecting
>> > us from ever using the stale pointer .. or we're just lucky, we should
>> > still clear it.
>> 
>> Hmm.  Maybe.  Currently this is done in dynamic_irq_cleanup,
>> at least for everything except sparc64.
>
> OK, I missed that. I still think we should do it here, otherwise there's
> a window, however small, where the msi_desc pointer is pointing at freed
> memory.

After following the code through the current cleanup happens before you are
proposing, and in fact the irq is return to the set of irq's that can
be allocated before you are calling set_irq_msi(irq, NULL).

Therefore you are doing this too late and we need to ensure the
architecture code does this in arch_teardown_msi_irq.

Eric

Re: [PATCH 17/21] MSI: Clear the irq_desc's msi pointer on free

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

On Thu, 2007-03-22 at 21:00 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
> 
> > On Thu, 2007-03-22 at 08:23 -0600, Eric W. Biederman wrote:
> >> Michael Ellerman <michael@ellerman.id.au> writes:
> >> 
> >> > Currently we never clear the msi_desc pointer in the irq_desc. This
> >> > leaves us with a pointer to free'ed memory hanging around. No one seems
> >> > to have hit this, so presumably other parts of the code are protecting
> >> > us from ever using the stale pointer .. or we're just lucky, we should
> >> > still clear it.
> >> 
> >> Hmm.  Maybe.  Currently this is done in dynamic_irq_cleanup,
> >> at least for everything except sparc64.
> >
> > OK, I missed that. I still think we should do it here, otherwise there's
> > a window, however small, where the msi_desc pointer is pointing at freed
> > memory.
> 
> After following the code through the current cleanup happens before you are
> proposing, and in fact the irq is return to the set of irq's that can
> be allocated before you are calling set_irq_msi(irq, NULL).

We don't call dynamic_irq_cleanup(), so it never gets done. Perhaps we
should be using your dynamic_irq_init/cleanup.

> Therefore you are doing this too late and we need to ensure the
> architecture code does this in arch_teardown_msi_irq.

As long as the arch teardown routine somehow calls dynamic_irq_cleanup()
it should be fine. But I guess it's probably safer to just have all
archs do set_msi_irq(irq, NULL) in the teardown.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 17/21] MSI: Clear the irq_desc's msi pointer on free

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

> We don't call dynamic_irq_cleanup(), so it never gets done. Perhaps we
> should be using your dynamic_irq_init/cleanup.

It depends.  If you are going through another irq controller etc.
dynamic_irq_cleanup is probably excessive.

>> Therefore you are doing this too late and we need to ensure the
>> architecture code does this in arch_teardown_msi_irq.
>
> As long as the arch teardown routine somehow calls dynamic_irq_cleanup()
> it should be fine. But I guess it's probably safer to just have all
> archs do set_msi_irq(irq, NULL) in the teardown.

Yes.  That sounds correct.

Eric