linux-kernel - Re: [PATCH] x86: check for valid irq_cfg pointer in smp_irq_move_cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.02.1205212324260.3231@ionos>
Date:	Mon, 21 May 2012 23:34:32 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Dimitri Sivanich <sivanich@....com>
cc:	Ingo Molnar <mingo@...hat.com>, "H. Peter Anvin" <hpa@...or.com>,
	x86@...nel.org, Suresh Siddha <suresh.b.siddha@...el.com>,
	Yinghai Lu <yinghai@...nel.org>,
	Naga Chumbalkar <nagananda.chumbalkar@...com>,
	Jacob Pan <jacob.jun.pan@...ux.intel.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86: check for valid irq_cfg pointer in
 smp_irq_move_cleanup_interrupt

On Mon, 21 May 2012, Dimitri Sivanich wrote:

> On Mon, May 21, 2012 at 11:07:04PM +0200, Thomas Gleixner wrote:
> > On Mon, 21 May 2012, Dimitri Sivanich wrote:
> > 
> > > The smp_irq_move_cleanup_interrupt routine should be checking for a valid
> > > irq_cfg pointer prior to accessing it.  It also seems that this should be
> > > done after taking the desc lock.
> > 
> > It seems that you either missed or failed to explain why it should be
> > done _after_ taking the lock.
> > 
> > Changelogs matter, really.
> >
> How about this?
>
> The smp_irq_move_cleanup_interrupt routine should be checking for a valid
> irq_cfg pointer prior to accessing it.

Why should it?
 
> Follow the same protocol shown in irq_set_chip_data(), by taking the desc
> lock before accessing this location.

This is not a proper explanation. irq_set_chip_data() might be wrong
as well and aside of that it might be correct to ignore that protocol
in that particular situation.

What's wrong with adding the actual wreckage scenario _AND_ the
solution to the changelog so that a casual reader, who is not
completely familiar with the code can understand what you are trying
to solve?

Don't misunderstand me. The patch is correct, just the explanation
sucks.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/