Re: [PATCH #2] console lock grabbed too early in printk...

From: Russell King (rmk@arm.linux.org.uk)
Date: Tue Jul 04 2000 - 14:58:43 EST


Chris Lattner writes:
> What makes an address a user address?

Depends on the architecture.

> When going through someone elses code, sometimes you can tell when it
> gets copied in, sometimes not...

If the kernel is using it without it copied in, then that is a bug,
and all praises to you who have found it.

> and should i have to do something like this everytime I want to printk?
>
> { char Buffer[1000];
> copyinto buffer
> printk(buffer);
> }

Err, no. You should just be aware that calling any kernel routine which
is not expected to handle user-mode addresses should not be passed user-mode
addresses.

> I think this is completely ridiculous. I'm asking to put one low
> contention lock and unlock into the printk path. Printk is not called
> very often except when "things are going wrong" or you are debugging... in
> either case, don't you want it to be as forgiving as possible??

Lets examine the facts shall we?

1. Your original problem was caused by calling printk with a user
     mode address.

Currently, we have ONE error here - calling printk from a user-mode address.

2. The user-mode address was not pointing at a present page, so printk
     faulted with its spinlock held.

At this point, we have TWO errors here - calling printk from a user-mode
address AND faulting with a spinlock held. Lets carry on with your case,
and you call printk from the page fault handling code.

3a. The page fault code calls printk

4a. We deadlock waiting on the spinlock.

Ok, lets say for argument that you didn't have that printk in:

3b. The page fault handler needs to access the disk to supply the page,
     so the handler sleeps.

Oh dear, another problem - sleeping with a spinlock held. Another favorite
for deadlocks. In fact, if the code before (1) locked a spinlock, you could
deadlock when that lock comes into contention.

You will also get the SAME behaviour and outcome as (3b) with your patch.
You're just making it harder to find out what the problem is when it does
happen. Who would think that the deadlock on foo_bar_lock in the depths
of the filesystem code was caused by someone calling printk with a user-mode
address?

> I'm starting to get annoyed at people who keep arguing that people don't
> make mistakes, and when they do, they should get punished for it. That is
> completely inappropriate crap.

Is it appropriate to add code to say "well, if this spinlock is held, we
can get around it this way and still execute the critical code?". Suddenly
all spinlocks have this condition in, just in case some kernel hacker makes
a mistake. Just think about the performance impact of all those extra
unnecessary tests which are just there incase of a mistake, and we don't
get a nice message to say "oi! you're being stupid calling me like that!"

> Maybe you are some macho experienced kernel developer, but I would be
> willing to be that even Linux and Alan make a mistake every once in a
> while... hell I bet they have to debug stuff they write too [and they
> might even use printk on occasion to do so!].

FYI, printk is my debugging tool too. Therefore your argument does not
hold water I'm afraid.

> What is the big conceptual problem here???

My point is that the rules are very very simple. The functions where
you can't use printk are very small in number (as I hinted in my previous
email), plus, even if you do put a printk in one of those, you're well
on the way to doom as soon as you save that source file and type make.

If you want a list, here they are:

        printk()
        vsprintf()
        spin_lock_irqsave()
        spin_unlock_irqrestore()
        wake_up_interruptible()
        vt_console_print()
        test_and_set_bit()
        pm_access()
        vc_cons_allocated()
        hide_cursor()
        scr_writew()
        set_cursor()
        poke_blanked_console()
        clear_bit()

And as I tried to point out before, your patch will not help if someone
puts a printk in any of those functions.

I hoped you'd understand the concept of infinite recusion.

Please try to understand the issues before you start getting annoyed that
your patch isn't being given a welcoming reception.
   _____
  |_____| ------------------------------------------------- ---+---+-
  | | Russell King rmk@arm.linux.org.uk --- ---
  | | | | http://www.arm.linux.org.uk/~rmk/aboutme.html / / |
  | +-+-+ --- -+-
  / | THE developer of ARM Linux |+| /|\
 / | | | --- |
    +-+-+ ------------------------------------------------- /\\\ |

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



This archive was generated by hypermail 2b29 : Fri Jul 07 2000 - 21:00:15 EST