Prev: drivers/hid: Eliminate use after free
Next: drivers/char: Eliminate use after free
From: Paul E. McKenney on 19 May 2010 19:00
On Wed, May 19, 2010 at 12:54:49PM -0700, David Miller wrote:
> From: "Paul E. McKenney" <paulmck(a)linux.vnet.ibm.com>
> Date: Wed, 19 May 2010 08:01:32 -0700
>
> > On Wed, May 19, 2010 at 11:03:27PM +1000, Nick Piggin wrote:
> >> For atomic_read it shouldn't matter unless gcc is *really* bad at it.
> >> Ah, for atomic_read, the required semantic is surely ACCESS_ONCE, so
> >> that's where the volatile is needed? (maybe it would be clearer to
> >> explicitly use ACCESS_ONCE?)
> >
> > Explicit use of ACCESS_ONCE() where needed makes a lot of sense to me,
> > and allows better code to be generated for initialization and cleanup
> > code where no other task has access to the atomic_t.
>
> I agree and I want to see this too, but I think with the tree the size
> that it is we have to work backwards at this point.
>
> Existing behavior by default, and optimized cases get tagged by using
> a new interface (atomic_read_light(), test_bit{,s}_light(), etc.)

Fair enough!

Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Nick Piggin on 21 May 2010 01:30
On Wed, May 19, 2010 at 03:50:46PM -0700, Paul E. McKenney wrote:
> On Wed, May 19, 2010 at 12:54:49PM -0700, David Miller wrote:
> > From: "Paul E. McKenney" <paulmck(a)linux.vnet.ibm.com>
> > Date: Wed, 19 May 2010 08:01:32 -0700
> >
> > > On Wed, May 19, 2010 at 11:03:27PM +1000, Nick Piggin wrote:
> > >> For atomic_read it shouldn't matter unless gcc is *really* bad at it.
> > >> Ah, for atomic_read, the required semantic is surely ACCESS_ONCE, so
> > >> that's where the volatile is needed? (maybe it would be clearer to
> > >> explicitly use ACCESS_ONCE?)
> > >
> > > Explicit use of ACCESS_ONCE() where needed makes a lot of sense to me,
> > > and allows better code to be generated for initialization and cleanup
> > > code where no other task has access to the atomic_t.
> >
> > I agree and I want to see this too, but I think with the tree the size
> > that it is we have to work backwards at this point.
> >
> > Existing behavior by default, and optimized cases get tagged by using
> > a new interface (atomic_read_light(), test_bit{,s}_light(), etc.)
>
> Fair enough!

Hmm, I'm missing something. David, back up a second, as far as I can see,
with Anton's patches, atomic_read() *is* effectively just ACCESS_ONCE()
now. Linus pointed out that header tangle is the reason not to just use
the macro.

Am I wrong, or is it that ACCESS_ONCE has a more relaxed semantic *in
theory* that allows a future more aggressive implementation if the
compiler supports it?

do {
done = ACCESS_ONCE(blah);
} while (!done);

Is this (in theory) allowed to be turned into a branch into an infinite
loop? Wheras the volatile deref would require it be reloaded each time?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Miller on 21 May 2010 02:00
From: Nick Piggin <npiggin(a)suse.de>
Date: Fri, 21 May 2010 15:27:46 +1000

> Hmm, I'm missing something. David, back up a second, as far as I can see,
> with Anton's patches, atomic_read() *is* effectively just ACCESS_ONCE()
> now. Linus pointed out that header tangle is the reason not to just use
> the macro.

My bad, I was under the impression that the proposal was to remove
volatile usage and also not even do ACCESS_ONCE() in atomic_read().

And then explicitly annotate call sits that actually need the
ACCESS_ONCE() semantic.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Miller on 21 May 2010 02:20
From: Nick Piggin <npiggin(a)suse.de>
Date: Fri, 21 May 2010 16:06:00 +1000

> Actually, I bet we have a lot of bugs there with loading integers and
> pointers atomically, where the code assumes the loaded value will not
> be reloaded by the compiler, because it is an easy thing to assume.

Alexey Kuznetsov was aware of this problem 8+ years ago when we were
first adding fine-grained locking the the networking.

> atomic_read_light could be useful though, for sure.

I definitely think so. And every usage of it should have a big fat
comment right next to it explaining how it's usage is valid in that
spot :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Nick Piggin on 21 May 2010 02:30
On Thu, May 20, 2010 at 10:54:54PM -0700, David Miller wrote:
> From: Nick Piggin <npiggin(a)suse.de>
> Date: Fri, 21 May 2010 15:27:46 +1000
>
> > Hmm, I'm missing something. David, back up a second, as far as I can see,
> > with Anton's patches, atomic_read() *is* effectively just ACCESS_ONCE()
> > now. Linus pointed out that header tangle is the reason not to just use
> > the macro.
>
> My bad, I was under the impression that the proposal was to remove
> volatile usage and also not even do ACCESS_ONCE() in atomic_read().
>
> And then explicitly annotate call sits that actually need the
> ACCESS_ONCE() semantic.

Ah ok, no. I see ACCESS_ONCE is a fundamental ("obvious") property of
atomic_read, so we definitely should keep it, even if we could audit
everyone.

Actually, I bet we have a lot of bugs there with loading integers and
pointers atomically, where the code assumes the loaded value will not
be reloaded by the compiler, because it is an easy thing to assume.

atomic_read_light could be useful though, for sure.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: drivers/hid: Eliminate use after free
Next: drivers/char: Eliminate use after free