Hello!
It seems there is error check missing in open_namei for errors returned
through intent.open.file (from lookup_instantiate_filp).
If there is plain open performed, then such a check done inside
__path_lookup_intent_open called from path_lookup_open(), but
when the open is performed with O_CREAT flag set, then
__path_lookup_intent_open is only called with LOOKUP_PARENT set where no file
opening can occur yet. Later on lookup_hash is called where exact opening
might take place and intent.open.file may be filled. If it is filled
with error value of some sort, then we get kernel attempting to dereference
this error value as address (and corresponding oops) in nameidata_to_filp()
called from filp_open().
While this is relatively simple to workaround in ->lookup() method by just
checking lookup_instantiate_filp() return value and returning error as
needed, this is not so easy in ->d_revalidate(), where we can only return
"yes, dentry is valid" or "no, dentry is invalid, perform full lookup again",
and just returning 0 on error would cause extra lookup (with potential
extra costly RPCs).
So in short, I believe that there should be no difference in error handling
for opening a file and creating a file in open_namei() and propose
this simple patch as a solution.
What do you think?
--- fs/namei.c.orig 2006-02-19 00:33:24.000000000 +0200
+++ fs/namei.c 2006-02-19 00:46:28.000000000 +0200
@@ -1575,6 +1575,12 @@ do_last:
goto exit;
}
+ if (IS_ERR(nd->intent.open.file)) {
+ up(&dir->d_inode->i_sem);
+ error = PTR_ERR(nd->intent.open.file);
+ goto exit_dput;
+ }
+
/* Negative dentry, just create the file */
if (!path.dentry->d_inode) {
if (!IS_POSIXACL(dir->d_inode))
Bye,
Oleg
On Sun, 2006-02-19 at 01:11 +0200, Oleg Drokin wrote:
> Hello!
>
> It seems there is error check missing in open_namei for errors returned
> through intent.open.file (from lookup_instantiate_filp).
> If there is plain open performed, then such a check done inside
> __path_lookup_intent_open called from path_lookup_open(), but
> when the open is performed with O_CREAT flag set, then
> __path_lookup_intent_open is only called with LOOKUP_PARENT set where no file
> opening can occur yet. Later on lookup_hash is called where exact opening
> might take place and intent.open.file may be filled. If it is filled
> with error value of some sort, then we get kernel attempting to dereference
> this error value as address (and corresponding oops) in nameidata_to_filp()
> called from filp_open().
> While this is relatively simple to workaround in ->lookup() method by just
> checking lookup_instantiate_filp() return value and returning error as
> needed, this is not so easy in ->d_revalidate(), where we can only return
> "yes, dentry is valid" or "no, dentry is invalid, perform full lookup again",
> and just returning 0 on error would cause extra lookup (with potential
> extra costly RPCs).
> So in short, I believe that there should be no difference in error handling
> for opening a file and creating a file in open_namei() and propose
> this simple patch as a solution.
> What do you think?
You're going to have to convert that semaphore call to a mutex call if
you want to apply it to 2.6.16 too, but otherwise it looks good.
Cheers,
Trond
> --- fs/namei.c.orig 2006-02-19 00:33:24.000000000 +0200
> +++ fs/namei.c 2006-02-19 00:46:28.000000000 +0200
> @@ -1575,6 +1575,12 @@ do_last:
> goto exit;
> }
>
> + if (IS_ERR(nd->intent.open.file)) {
> + up(&dir->d_inode->i_sem);
> + error = PTR_ERR(nd->intent.open.file);
> + goto exit_dput;
> + }
> +
> /* Negative dentry, just create the file */
> if (!path.dentry->d_inode) {
> if (!IS_POSIXACL(dir->d_inode))
>
> Bye,
> Oleg