Re: [PATCH] fs: fput() can be called from interrupt context

[Eric Dumazet <dada1@cosmosbay.com>]

Andrew Morton a écrit :
> On Thu, 12 Mar 2009 07:10:38 +0100 Eric Dumazet <dada1@cosmosbay.com> wrote:
> 
>>> Did you reproduce the bug, and confirm that the patch fixes it?
>> take Davide program : http://www.xmailserver.org/eventfd-aio-test.c
>>
>> and add at line 318 :
>> close(afd);
>>
>> It should produce the kernel bug...
> 
> "should"?

Sorry I had to run this morning, so I was a litle bit lazy...

Maybe some kind of O_DIRECT / NFS / blockdev setup or something calling 
aio_complete() from interrupt context ? I am not familiar of this part,
but considering spin_lock_irq()/spin_unlock_irq() calls in fs/aio.c,
there must be a reason for this ?

> 
>>> Are there simpler ways of fixing it?  Maybe sneak a call to
>>> wait_for_all_aios() into the right place?  I doubt if it's performance
>>> critical, as nobody seems to have ever hit the bug.
>> Take the time to check how fs/aio.c handle the fput(req->ki_filp) case
>> (or read my 2nd patch, it should spot the thing)
> 
> Well yes, a kludge like that seems a bit safer.
> 
> It's somewhat encouraging that we're apparently already doing fput()
> from within keventd (although how frequently?).  There might be
> problems with file locking, security code, etc from doing fput() from
> an unexpected thread.  And then there are all the usual weird problem
> with using the keventd queues which take a long time to get discovered.
> 

Hum... Do you mean "if (in_interrupt())" is not the right test to
perform ? 

> 
>> If you want to add another kludge to properly fput(req->ki_eventfd),
>> be my guest :-(
>>
>>> Bear in mind that if the bug _is_ real then it's now out there, and
>>> we would like a fix which is usable by 2.6.<two-years-worth>.
> 
> The patches are large and scary and it would be a real problem to merge
> them into 2.6.29 at this stage, let alone 2.6.25, etc.
> 
> Especially as the code which you sent out appears to be untested:
> 

I actually tested it and got a working kernel before sending patch,
please trust me...

Oh yes, my dev machine always called the slow path then, this is
why I didnt noticed.

Thank you for spotting this inverted test.

>>  void fput(struct file *file)
>>  {
>> -	if (atomic_long_dec_and_test(&file->f_count))
>> -		__fput(file);
>> +	if (atomic_long_dec_and_test(&file->f_count)) {
>> +		if (unlikely(!in_interrupt()))
> 
>                              ^
> 
>> +			fd_defer_queue(NULL, file);
>> +		else
>> +			__fput(file);
>> +	}
>>  }
>