public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH - RESEND - 000 of 2] Avoid subtle cache consistancy problem
@ 2006-05-22  4:46 NeilBrown
  2006-05-22  4:46 ` [PATCH 001 of 2] Prepare for __copy_from_user_inatomic to not zero missed bytes NeilBrown
  2006-05-22  4:46 ` [PATCH 002 of 2] Make copy_from_user_inatomic NOT zero the tail on i386 NeilBrown
  0 siblings, 2 replies; 6+ messages in thread
From: NeilBrown @ 2006-05-22  4:46 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel

This is a resend of a pair of patches that didn't get a lot of attention
last time.
I've cleaned up the second one a bit, as it had some ugliness that might
have put some people off...

The problem is that when we write to a file, the copy from userspace
to pagecache is first done with preemption disabled, so if the source
address is not immediately available the copy fails *and* *zeros*
*the*  *destination*.

This is a problem because a concurrent read (which admittedly is an
odd thing to do) might see zeros rather that was there before the
write, or what was there after, or some mixture of the two (any of
these being a reasonable thing to see).

If the copy did fail, it will immediately be retried with preemption
re-enabled so any transient problem with accessing the source won't
cause an error.

The first copying does not need to zero any uncopied bytes, and doing
so causes the problem.
It uses copy_from_user_atomic rather than copy_from_user so the simple
expedient is to change copy_from_user_atomic to *not* zero out bytes
on failure.

The first of these two patches prepares for the change by fixing two
places which assume copy_from_user_atomic does zero the tail.  The
two usages are very similar pieces of code which copy from
a userspace iovec into one or more page-cache pages.  These are
changed to remove the assumption.

The second patch changes __copy_from_user_inatomic* to not zero the
tail.
Once these are accepted, I will look at similar patches of other
architectures where this is important (ppc, mips and sparc being the
ones I can find).

Feedback very welcome.

Thanks.
NeilBrown


 [PATCH 001 of 2] Prepare  for __copy_from_user_inatomic to not zero missed bytes.
 [PATCH 002 of 2] Make copy_from_user_inatomic NOT zero the tail on i386

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH INTRO] Re: [RFC] copy_from_user races with readpage, [PATCH 000 of 2] Introduction
@ 2006-04-28  2:04 NeilBrown
  2006-04-28  2:10 ` [PATCH 002 of 2] Make copy_from_user_inatomic NOT zero the tail on i386 NeilBrown
  0 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2006-04-28  2:04 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Chris Mason, linux-kernel, andrea

On Wednesday April 19, akpm@osdl.org wrote:
> Chris Mason <mason@suse.com> wrote:
> >
> > Hello everyone,
> > 
> > I've been working with IBM on a long standing bug where zeros unexpectedly pop 
> > up during a disk certification test.  We tracked it down to copy_from_user.  
....
> 
> I'd have thought that a sufficient fix would be to change
> __copy_from_user_inatomic() to not do the zeroing, then review all users to
> make sure that they cannot leak uninitialised memory.

So I'm following this up and trying to figure out how best to make this
"right".

Following are two patches.  
The first is the result of the suggested "review".
The only users of copy_from_user_inatomic that cannot safely lose the
zeroing are two separate (but similar:-() implmentations of 
  copy_from_user_iovec
These I have 'fixed'.

It is unfortunate that both chose to "know" exactly the difference between
the _inatomic and the regular versions, and call _inatomic not in atomic context.
It seems to suggest poor interface design, but I'm not sure exactly what
the poor choice is.

Also after reading this code I am very aware that on architectures that
aren't saddled with highmem (e.g. 64bit) the duplication of copy_from_user
is simply wasted icache space.  Possibly it might make sense to guard the first
_inatomic copy with "if(PageHighMem(page))" which should complie it away to
nothing when highmem isn't present.

The second patch changes __copy_from_user_inatomic to not do zeroing
in i386.  I'm quite open to the possiblity of being told that something
I did there is either very silly or very ugly or both.  However not being
very experienced in arch/asm code I'm not sure what.  Constructive
criticism very welcome.

If happiness is achieved with these patches, we then need to look at similar
patches for powerpc, mips, and sparc.

Thanks for your time.

NeilBrown


 [PATCH 001 of 2] Prepare  for __copy_from_user_inatomic to not zero missed bytes.
 [PATCH 002 of 2] Make copy_from_user_inatomic NOT zero the tail on i386

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2006-05-25 12:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-05-22  4:46 [PATCH - RESEND - 000 of 2] Avoid subtle cache consistancy problem NeilBrown
2006-05-22  4:46 ` [PATCH 001 of 2] Prepare for __copy_from_user_inatomic to not zero missed bytes NeilBrown
2006-05-25 11:59   ` David Howells
2006-05-25 12:24     ` Neil Brown
2006-05-22  4:46 ` [PATCH 002 of 2] Make copy_from_user_inatomic NOT zero the tail on i386 NeilBrown
  -- strict thread matches above, loose matches on Subject: below --
2006-04-28  2:04 [PATCH INTRO] Re: [RFC] copy_from_user races with readpage, [PATCH 000 of 2] Introduction NeilBrown
2006-04-28  2:10 ` [PATCH 002 of 2] Make copy_from_user_inatomic NOT zero the tail on i386 NeilBrown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox