On Wednesday 21 September 2005 22:47, Andrew Morton wrote:
> Blaisorblade <[email protected]> wrote:
> > On Wednesday 21 September 2005 21:49, Andrew Morton wrote:
> > > "Paolo 'Blaisorblade' Giarrusso" <[email protected]> wrote:
> > > > From: Paolo 'Blaisorblade' Giarrusso <[email protected]>
> > > It has accidental side-effects,
> > > such as making copy_to_user() fail if inside spinlocks when
> > > CONFIG_PREEMPT=y.
> > Sorry, but should it ever succeed inside spinlocks? I mean, should it
> > ever call down() inside spinlocks? (We never do down_trylock, and ever if
> > we did the x86 trick, that wouldn't make the whole thing safe at all -
> > they still take the spinlock and potentially sleep. And it's legal only
> > if no spinlock is held).
> Not sure what you're asking here.
> copy_to/from_user() will fail inside spinlock if CONFIG_PREMPT=y and if the
> copy happens to cause a fault.
> Otherwise it will succeed inside spinlock,
> and it won't spew a sleeping-while-atomic warning, because that uses
> in_atomic() too.
> It might deadlock if we schedule away and try to retake
> the same lock.
Exactly - the point is: is it legal to call copy_from_user() while holding a
spinlock (which is my original question)? Or should copy_from_user try to
satisfy the fault, instead of seeing in_atomic() or something similar and
>From what you say, copy_*_user is called in such a way, but it's
deadlock-prone, when CONFIG_PREEMPT is disabled.
> > Even if spinlocks don't always trigger in_atomic() - which means that
> > we'd need to have a better fix for this.
> The patch you have will correctly cause copy_*_user()->pagefault to fail
> the copy if the caller has run inc_preempt_count(). It will not cause
> copy_*_user()->pagefault to fail inside spinlocks unless UML does
> inc_preempt_count() in its spinlock implementation.
No, it doesn't. But for that case, we're in the same situation as i386.
Consider even that while UML doesn't implement PREEMPT (but we'll fix that,
sooner or later) it does implement HIGHMEM, and answering to your original
> So I think this change is only needed if UML implements kmap_atomic, as in
> arch/i386/mm/highmem.c, which it surely does not do?
Apart that anybody would make sure that atomic kmaps are indeed atomic, UML
doesn't use a "similar implementation" - it verbatim symlinks the i386 file
and builds it (see arch/um/sys-i386/Makefile and
arch/um/scripts/Makefile.rules if you want).
Hmm, that's something worth considering... at least when debugging is enabled,
for debugging purposes.
> > NACK, see above.
> Yup, the patch is needed for the futex code,
> and for general correct
> implementation of inc_preempt_count()'s intended effect.
Ok, that's enough I guess (and I just read what Linus said).
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Video 4 Linux]
[Linux for the blind]