linux-kernel - Re: powerpc/cell/oprofile: fix mutex locking for spu-oprofile

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]

Date:	Wed, 20 Aug 2008 07:58:16 -0700
From:	Carl Love <cel@...ibm.com>
To:	Robert Richter <robert.richter@....com>
Cc:	Arnd Bergmann <arnd@...db.de>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linuxppc-dev@...abs.org, Paul Mackerras <paulus@...ba.org>,
	oprofile-list@...ts.sourceforge.net, cel <cel@...ux.vnet.ibm.com>,
	cbe-oss-dev@...abs.org
Subject: Re: powerpc/cell/oprofile: fix mutex locking for spu-oprofile


On Wed, 2008-08-20 at 14:39 +0200, Robert Richter wrote:
> On 20.08.08 14:05:31, Arnd Bergmann wrote:
> > On Wednesday 20 August 2008, Robert Richter wrote:
> > > I am fine with the changes with the exception of removing
> > > add_event_entry() from include/linux/oprofile.h. Though there is no
> > > usage of the function also in other architectures anymore, this change
> > > in the API should be discussed on the oprofile mailing list. Please
> > > separate the change in a different patch and submit it to the mailing
> > > list. If there are no objections then, this change can go upstream as
> > > well.
> > 
> > As an explanation, the removal of add_event_entry is the whole point
> > of this patch. add_event_entry must only be called with buffer_mutex
> > held, but buffer_mutex itself is not exported.
> 
> Thanks for pointing this out.
> 

We originally added add_event_entry() to include/linux/oprofile.h
specifically because it was needed for the CELL SPU support.  As it
turns out it the approach was not completely thought through.  We were
using the function call without holding the mutex lock.  As we
discovered later, this can result in corrupting the data put into the
event buffer.  So exposing the function without a way to hold the mutex
lock is actually a really bad idea as it would encourage others to fall
into the same mistake that we made.  So, as Arnd said, the whole point
of this patch is to come up with a correct approach to adding the data.

> > I'm pretty sure that no other user of add_event_entry exists, as it
> > was exported specifically for the SPU support and that never worked.
> > Any other (theoretical) code using it would be broken in the same way
> > and need a corresponding fix.
> > 
> > We can easily leave the declaration in place, but I'd recommend removing
> > it eventually. If you prefer to keep it, how about marking it as
> > __deprecated?
> 
> No, since this is broken by design we remove it. The patch can go
> upstream as it is.
> 
> Thanks,

> -Robert
> 

It really is best to remove it.  Thank you for taking the time to review
and comment on the patch.

               Carl Love

--
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/