Re: [PATCH] add check do_direct_IO() return val

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





Andrew Morton wrote:
On Thu, 26 Jul 2007 17:04:00 +0800 Joe Jin <[email protected]> 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 <[email protected]>


---
--- 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 [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]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux