Re: lock_kernel check...

Re: lock_kernel check...

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 3180 bytes --]

cc'ing LKML 'cause this is interesting...

dan carpenter wrote:
 > As you can see, the attached script is dead simple.  It prints an
 > error every time you call return while lock_kernel is held.  On
 > your computer you will want to comment out print_url() and
 > uncomment the regular print statement.

I am continually amazed at all the simple, useful, cool stuff that 
people come up with.  I like!

 > I tested it on linux-2.5.7 because then I could just put a link to
 > the offending lines and get your feedback.  I compiled fs/*/*.c

Time to run it on some more recent versions.  Don't worry about not 
giving links, filename:line# is just fine for most people.  I promise 
_I_ won't complain :)

time for the breakdown:
  > http://lxr.linux.no/source/fs/affs/namei.c?v=2.5.7#L349
error

 > http://lxr.linux.no/source/fs/hpfs/dir.c?v=2.5.7#L194
error

 > http://lxr.linux.no/source/fs/intermezzo/dir.c?v=2.5.7#L580
 > http://lxr.linux.no/source/fs/intermezzo/dir.c?v=2.5.7#L594
 > http://lxr.linux.no/source/fs/intermezzo/file.c?v=2.5.7#L303
 > http://lxr.linux.no/source/fs/intermezzo/vfs.c?v=2.5.7#L1952
 > http://lxr.linux.no/source/fs/intermezzo/vfs.c?v=2.5.7#L2053

Intermezzo is doing some subtle things here.  It needs to do some 
dentry lookups and doesn't want to deadlock with dcache_lock (I 
think).  It releases the BKL to do these, then reacquires it so that 
the caller function doesn't double-unlock.  These are weird, but 
correct.

 > http://lxr.linux.no/source/fs/jbd/journal.c?v=2.5.7#L270

Another subtle one.  This is a throwback to the earliest days of the 
BKL where kernel daemons did a lock_kernel() while they were 
executing, and an unlock_kernel() when they were ready to give up the 
CPU.  This is still the case for kjournald, but in a much more 
convoluted way.  Since kjournald is a thread, when it returns, the 
process is destroyed and the BKL is released implcitly during the exit 
sequence.  That is why you never see an unlock_kernel().

Here's the basic process (may god help us all):

kernel_thread(kjournald,...) // start the new kjournald thread
lock_kernel()
work: // do work

// go to sleep for some reason
schedule()
	unlock_kernel()
	sleeping...
	wake_up()
	lock_kernel()

if( more to do )
	goto work;
else
	exit()
	unlock_kernel().


 > The question is are any of these actual errors?  What other things
 > are possibly illegal to do while lock_kernel is held?

It isn't absoulutely a bad thing to return while you have a lock held. 
    It might be hard to understand, or even crazy, but not immediately 
wrong :)

// BKL protects both "a", and io port 0xF00D
bar()
{
	lock_kernel();
	return inb(0xF00D);
}

int a;
foo()
{
	a = bar();
	a--;
	unlock_kernel();
}


But, back to the subject at hand.  Yes, you've found some errors here 
because in almost all normal cases, you don't mean to return with a 
lock still held.   These were most likely caused by myself or others 
pushing the BKL into these functions from above and are bugs.

Would you like to do the patches to fix it, or do you want me to do 
them?  They shouldn't be hard to do.

What is smatch.pm? I can't find it anywhere.

-- 
Dave Hansen
haveblue@us.ibm.com

[-- Attachment #2: lock-check.pl --]
[-- Type: text/plain, Size: 883 bytes --]

#!/usr/bin/perl -w
use smatch;

sub print_url {
  if (get_filename() =~ /home\/carp0110\/progs\/kernel\/devel\/linux-2.5.7\/(.*)/){
    print 'http://lxr.linux.no/source/', $1, '?v=2.5.7#', get_lineno(), "\n";
  }
}

while ($data = get_data()){
  # if we see a lock_kernel then we set the state to "locked"
  if ($data =~ /call_expr\(\(addr_expr function_decl\(lock_kernel/){
    set_state("locked");
    next;
  }

  # if we see an unlock_kernel we set the state to "unlocked"
  if ($data =~ /call_expr\(\(addr_expr function_decl\(unlock_kernel/){
    set_state("unlocked");
    next;
  }

  # if we see a return statement and the kernel is 
  # locked then we print a mesg.
  if ($data =~ /^return_stmt/){
    if (get_state() =~ /^locked$/){
      #print "Not unlocked: ", get_filename(), " ", get_start_line(), " ", get_lineno(), "\n";
      print_url();
      next;
    }
  }
}

Re: lock_kernel check...

[Zwane Mwaikambo]

On Tue, 9 Jul 2002, Dave Hansen wrote:

> It isn't absoulutely a bad thing to return while you have a lock held. 
>     It might be hard to understand, or even crazy, but not immediately 
> wrong :)
> 
> // BKL protects both "a", and io port 0xF00D
> bar()
> {
> 	lock_kernel();
> 	return inb(0xF00D);
> }
> 
> int a;
> foo()
> {
> 	a = bar();
> 	a--;
> 	unlock_kernel();
> }

But broken nonetheless, that kinda thing just looks ugly. Especially when 
someone tries to call bar multiple times or consecutively or with the lock 
already held or...

	Zwane Mwaikambo

-- 
function.linuxpower.ca

Re: lock_kernel check...

[Dave Hansen]

Zwane Mwaikambo wrote:
> On Tue, 9 Jul 2002, Dave Hansen wrote:
> 
> 
>>It isn't absoulutely a bad thing to return while you have a lock held. 
>>    It might be hard to understand, or even crazy, but not immediately 
>>wrong :)
>>
>>// BKL protects both "a", and io port 0xF00D
>>bar()
>>{
>>	lock_kernel();
>>	return inb(0xF00D);
>>}
>>
>>int a;
>>foo()
>>{
>>	a = bar();
>>	a--;
>>	unlock_kernel();
>>}
> 
> But broken nonetheless, that kinda thing just looks ugly. Especially when 
> someone tries to call bar multiple times or consecutively or with the lock 
> already held or...

Yes, it is horribly ugly, but it is not broken.  As a function, if you 
document what you require your caller to do, there shouldn't be a 
problem.

Also, it is valid to have nested holds of the BKL.  You can never 
deadlock with another lock_kernel() which was done in the same process.

-- 
Dave Hansen
haveblue@us.ibm.com

Re: lock_kernel check...

[dan carpenter]

----- Original Message -----
From: Dave Hansen <haveblue@us.ibm.com>
Date: Tue, 09 Jul 2002 02:08:18 -0700 
To: dan carpenter <error27@email.com>
Subject: Re: lock_kernel check...

> cc'ing LKML 'cause this is interesting...
> 
> dan carpenter wrote:
>  > As you can see, the attached script is dead simple.  It prints an
>  > error every time you call return while lock_kernel is held.  On
>  > your computer you will want to comment out print_url() and
>  > uncomment the regular print statement.
> 
> I am continually amazed at all the simple, useful, cool stuff that 
> people come up with.  I like!
> 

Glad you liked it.  :) 

Smatch.pm is from the smatch.sf.net scripts page.  Smatch is a really unfinished code checker that I've been working on.  It is based on reading the papers about the Stanford checker.   

Unfortunately, after a night of sleep I realize that my script is broken for 2 reasons.  
1)  Smatch.pm is meant to track state changes down different code paths.  But unfortunately it wasn't doing that in this case; it was just going down the code without taking into consideration any if_stmts  etc.  I'm extremely embarassed about that.  Sorry.  
2)  What the Stanford checker does is print an error if one return_stmt is called while the kernel is locked and one is called while the kernel is unlocked.  This seems reasonable.

I will fix both mistakes later on this week.  Unfortunately I'm in the process of moving and looking for a job etc so I might not get to it for a bit.

regards,
dan carpenter

PS.  If you liked this script, try out my kmalloc script.  I don't think anyone besides me has successfully installed it yet, so if you have any questions I'd be glad to help.  :P  My phone number until tomorrow evening is (510) 835-7695.

-- 
__________________________________________________________
Sign-up for your own FREE Personalized E-mail at Mail.com
http://www.mail.com/?sr=signup

Save up to $160 by signing up for NetZero Platinum Internet service.
http://www.netzero.net/?refcd=N2P0602NEP8

Re: lock_kernel check...

[Dave Hansen]

dan carpenter wrote:
 > Smatch.pm is from the smatch.sf.net scripts page.  Smatch is a
 > really unfinished code checker that I've been working on.  It is
 > based on reading the papers about the Stanford checker.

There was a time when I was thinking about the same thing.  It kept 
scaring me the more I thought about it.

 > Unfortunately, after a night of sleep I realize that my script is
 > broken for 2 reasons. 1)  Smatch.pm is meant to track state changes
 > down different code paths.  But unfortunately it wasn't doing that
 > in this case; it was just going down the code without taking into
 > consideration any if_stmts  etc.  I'm extremely embarassed about
 > that.  Sorry.

Don't be sorry.  The script is smarter than the people who caused the 
errors.  (once again, probably me)

 > 2)  What the Stanford checker does is print an error
 > if one return_stmt is called while the kernel is locked and one is
 > called while the kernel is unlocked.  This seems reasonable.

Could you clarify that a bit?

 > I will fix both mistakes later on this week.  Unfortunately I'm in
 > the process of moving and looking for a job etc so I might not get
 > to it for a bit.


-- 
Dave Hansen
haveblue@us.ibm.com

Re: lock_kernel check...

[dan carpenter]

----- Original Message -----
From: Dave Hansen <haveblue@us.ibm.com>
Date: Tue, 09 Jul 2002 10:41:01 -0700 
To: dan carpenter <error27@email.com>
Subject: Re: lock_kernel check...

> dan carpenter wrote:
>  > Smatch.pm is from the smatch.sf.net scripts page.  Smatch is a
>  > really unfinished code checker that I've been working on.  It is
>  > based on reading the papers about the Stanford checker.
> 
> There was a time when I was thinking about the same thing.  It kept 
> scaring me the more I thought about it.
> 

True.  But someone is going to write a checker at some point.  It's only a couple days work if you know what you are doing.  There doesn't seem to be much advantage in waiting a year or two.

>  > Unfortunately, after a night of sleep I realize that my script is
>  > broken for 2 reasons. 1)  Smatch.pm is meant to track state changes
>  > down different code paths.  But unfortunately it wasn't doing that
>  > in this case; it was just going down the code without taking into
>  > consideration any if_stmts  etc.  I'm extremely embarassed about
>  > that.  Sorry.
> 
> Don't be sorry.  The script is smarter than the people who caused the 
> errors.  (once again, probably me)
> 
>  > 2)  What the Stanford checker does is print an error
>  > if one return_stmt is called while the kernel is locked and one is
>  > called while the kernel is unlocked.  This seems reasonable.
> 
> Could you clarify that a bit?
> 

If someone made a mistake where they always returned under a kernel_lock() they would find the mistake themselves.  

Why would they return under kernel_lock() on error, for example, but not on success?  That would be confusing.  There would still be some false positives but the number of cases is really small.

regards,
dan carpenter


-- 
__________________________________________________________
Sign-up for your own FREE Personalized E-mail at Mail.com
http://www.mail.com/?sr=signup

Save up to $160 by signing up for NetZero Platinum Internet service.
http://www.netzero.net/?refcd=N2P0602NEP8