linux-kernel - Re: [PATCH] add check do_direct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <46A9A81C.1070309@oracle.com>
Date:	Fri, 27 Jul 2007 13:39:00 +0530
From:	gurudas pai <gurudas.pai@...cle.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	Joe Jin <joe.jin@...cle.com>, torvalds@...ux-foundation.org,
	jens.axboe@...cle.com, linux-kernel@...r.kernel.org,
	wen.gang.wang@...cle.com, Badari Pulavarty <pbadari@...ibm.com>,
	Zach Brown <zach.brown@...cle.com>
Subject: Re: [PATCH] add check do_direct_IO() return val



Andrew Morton wrote:
> On Thu, 26 Jul 2007 17:04:00 +0800 Joe Jin <joe.jin@...cle.com> wrote:
>
>> This is the patch for check do_direct_IO() return val.
>>
>> At do_direct_IO(), sometimes dio_get_page() will return -EFAULT/-ENOMEM,
>> according to orig source, it will go on left work. buf for dio_get_page()
>> return a error will made many useful member of dio not initialized like
>> dio->map_bh and others, at this point, kernel will panic.
>>
>> Signed-off-by: Joe Jin <joe.jin@...cle.com>
>>
>>
>> ---
>> --- linux-2.6.22/fs/direct-io.c.orig	2007-07-26 11:32:27.000000000 +0800
>> +++ linux-2.6.22/fs/direct-io.c	2007-07-26 11:33:58.000000000 +0800
>> @@ -1031,7 +1031,9 @@ direct_io_worker(int rw, struct kiocb *i
>>  			((dio->final_block_in_request - dio->block_in_file) <<
>>  					blkbits);
>>  
>> -		if (ret) {
>> +		if (ret == -EFAULT || ret == -ENOMEM) 
>> +			goto out;
>> +		else if (ret) {
>>  			dio_cleanup(dio);
>>  			break;
>>  		}
>> @@ -1113,6 +1115,7 @@ direct_io_worker(int rw, struct kiocb *i
>>  	} else
>>  		BUG_ON(ret != -EIOCBQUEUED);
>>  
>> +out:
>>  	return ret;
>>  }
>>  
>
> I think we still want to run dio_cleanup() if do_direct_IO() failed? 
> Otherwise we can leak pages.
>
> And there's nothing special about EFAULT or ENOMEM here: if do_direct_IO()
> returns any error then that's it: we bale out, yes?
>
> In fact I'm suspecting that this is what the code in there used to do. 
> Something like:
>
> 	for (...) {
> 		...
> 		ret = do_direct_IO(...);
> 		...
> 		if (ret) {
> 			dio_dleanup(dio);
> 			break
> 		}
> 	}
> 	return ret;
>
> but then someone later came along and added more code between the end of
> the for loop and the `return' statement.
>
> <I do miss the BK diffviewer sometimes>
>
> <does a binary search>
>
> yep, that's exactly what happened.  We broke it in the 2.5.45 release back
> in October 2002. (Where "we" == "Badari")
>
> So I think what we need to do is something close to this:
>
> --- a/fs/direct-io.c~add-check-do_direct_io-return-val
> +++ a/fs/direct-io.c
> @@ -1033,7 +1033,7 @@ direct_io_worker(int rw, struct kiocb *i
>  
>  		if (ret) {
>  			dio_cleanup(dio);
> -			break;
> +			goto out;
>  		}
>  	} /* end iovec loop */
>  
> @@ -1112,7 +1112,7 @@ direct_io_worker(int rw, struct kiocb *i
>  		kfree(dio);
>  	} else
>  		BUG_ON(ret != -EIOCBQUEUED);
> -
> +out:
>  	return ret;
>  }
>  
> _
>
> However I'd like to ask you guys to carefully review and test that please.
Tested Andrew's patch , works! . My test case passed without any panic. 
But got couple of ENOTBLK.


Thanks,
-Guru
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/