[RFC] atomic open(..., O_CREAT | ...)

[RFC] atomic open(..., O_CREAT | ...)

[Miklos Szeredi]

I'd like to make my filesystem be able to do file creation and opening
atomically.  This is needed for filesystems which cannot separate
checking open permission from the actual open operation.

Usually any filesystem served from userspace by an unprivileged (no
CAP_DAC_OVERRIDE) process will be such (ftp, sftp, etc.).

With nameidata->intent.open.* it is possible to do the actual open
from ->lookup() or ->create().  However there's no easy way to
associate the 'struct file *' returned by dentry_open() with the
filesystem's private file object.  Also if there's some error after
the file has been opened but before a successful return of the file
pointer, the filesystem has no way to know that it should destroy the
private file object.

The following patch makes this possible through a new file pointer
field in the open intent data, through which the filesystem can pass
an opened file to be returned by filp_open().

The filesystem can call dentry_open() from ->lookup() or ->create(),
and it in with it's private file data.  If there's an error the file
can be properly destroyed through f_op->release().

There's one question on which I'm not sure what is the best solution:

The filesystem needs to know whether it's f_op->open() method was
called from lookup/create, or from the filp_open(), because in the
first case it need not do anything (the private file object will be
created outside dentry_open()), but in the second case it must
actually prepare the private file object.

Two solutions come to mind:

  1) pass a special open flag to dentry_open() which will be passed on
     to f_op->open() in filp->f_flags

  2) create a new 'dentry_open_noopen()' variant, which doesn't call
     f_op->open()

Does one sound better?  Or something else?

Comments are welcome.

Thanks,
Miklos

Index: linux/include/linux/namei.h
===================================================================
--- linux.orig/include/linux/namei.h	2005-06-17 21:48:29.000000000 +0200
+++ linux/include/linux/namei.h	2005-08-06 17:12:55.000000000 +0200
@@ -8,6 +8,10 @@ struct vfsmount;
 struct open_intent {
 	int	flags;
 	int	create_mode;
+	int	orig_flags;
+
+	/* Fs may want to do dentry_open() in ->lookup(), or in ->create() */
+	struct file *file;
 };
 
 enum { MAX_NESTED_LINKS = 5 };
Index: linux/fs/open.c
===================================================================
--- linux.orig/fs/open.c	2005-08-06 12:34:14.000000000 +0200
+++ linux/fs/open.c	2005-08-08 13:03:08.000000000 +0200
@@ -762,9 +762,22 @@ struct file *filp_open(const char * file
 	if (namei_flags & O_TRUNC)
 		namei_flags |= 2;
 
+	/* Fill in the open() intent data */
+	nd.intent.open.flags = namei_flags;
+	nd.intent.open.create_mode = mode;
+	nd.intent.open.orig_flags = flags;
+	nd.intent.open.file = NULL;
+
 	error = open_namei(filename, namei_flags, mode, &nd);
-	if (!error)
-		return dentry_open(nd.dentry, nd.mnt, flags);
+	if (!error) {
+		if (nd.intent.open.file)
+			return nd.intent.open.file;
+		else 
+			return dentry_open(nd.dentry, nd.mnt, flags);
+	}
+
+	if (nd.intent.open.file && !IS_ERR(nd.intent.open.file))
+		fput(nd.intent.open.file);
 
 	return ERR_PTR(error);
 }
Index: linux/fs/namei.c
===================================================================
--- linux.orig/fs/namei.c	2005-08-06 12:35:59.000000000 +0200
+++ linux/fs/namei.c	2005-08-06 17:12:55.000000000 +0200
@@ -1423,10 +1423,6 @@ int open_namei(const char * pathname, in
 	if (flag & O_APPEND)
 		acc_mode |= MAY_APPEND;
 
-	/* Fill in the open() intent data */
-	nd->intent.open.flags = flag;
-	nd->intent.open.create_mode = mode;
-
 	/*
 	 * The simplest case - just a plain lookup.
 	 */

Re: [RFC] atomic open(..., O_CREAT | ...)

[Trond Myklebust]

ty den 09.08.2005 Klokka 00:27 (+0200) skreiv Miklos Szeredi:
> I'd like to make my filesystem be able to do file creation and opening
> atomically.  This is needed for filesystems which cannot separate
> checking open permission from the actual open operation.
> 
> Usually any filesystem served from userspace by an unprivileged (no
> CAP_DAC_OVERRIDE) process will be such (ftp, sftp, etc.).
> 
> With nameidata->intent.open.* it is possible to do the actual open
> from ->lookup() or ->create().  However there's no easy way to
> associate the 'struct file *' returned by dentry_open() with the
> filesystem's private file object.  Also if there's some error after
> the file has been opened but before a successful return of the file
> pointer, the filesystem has no way to know that it should destroy the
> private file object.

We've already got a patch that does this, and that I'm queueing up for
inclusion. See

http://client.linux-nfs.org/Linux-2.6.x/2.6.12/linux-2.6.12-63-open_file_intents.dif

As for the "orig flags" thing. What is the point of that?

Cheers,
  Trond

Re: [RFC] atomic open(..., O_CREAT | ...)

[Miklos Szeredi]

> We've already got a patch that does this, and that I'm queueing up for
> inclusion.

Cool!

> http://client.linux-nfs.org/Linux-2.6.x/2.6.12/linux-2.6.12-63-open_file_intents.dif

Comments:

>  /*
> + * Open intents have to release any file pointer that was allocated
> + * but not used by the VFS.
> + */
> +void path_release_open_intent(struct nameidata *nd)
> +{
> +	if ((nd->flags & LOOKUP_OPEN) && nd->intent.open.file != NULL) {
> +		fput(nd->intent.open.file);

I think you should consider adding this:

+		if (!IS_ERR(nd->intent.open.file))
+			fput(nd->intent.open.file);

so the filesystem can delay returning the error from the open
operation until the other errors have been sorted out by the lookup
code.

> +		nd->intent.open.file = NULL;

Why is this NULL assignment needed?  nd will not be used after this.

> +	}
> +	path_release(nd);
> +}
> +
> 



> As for the "orig flags" thing. What is the point of that?

dentry_open() needs the original open flags, not the transformed ones
stored in intent.open.flags.

The behavior is slightly strange, since filp_open() calculates
namei_flags (which gets stored in intent.open.flags) so that an
O_ACCMODE of 3 is transformed into FMODE_READ | FMODE_WRITE.

But dentry_open() calculates filp->f_mode, so that O_ACCMODE of 3 is
transformed into zero.

This means that the (undocumented) access mode of 3 will require
read-write permission, but will allow neither read() nor write() on
the opened file.

If we want to keep this behavior, then the orig_flags field is needed.

Thanks,
Miklos

Re: [RFC] atomic open(..., O_CREAT | ...)

[Trond Myklebust]

ty den 09.08.2005 Klokka 09:46 (+0200) skreiv Miklos Szeredi:
> > We've already got a patch that does this, and that I'm queueing up for
> > inclusion.
> 
> Cool!
> 
> > http://client.linux-nfs.org/Linux-2.6.x/2.6.12/linux-2.6.12-63-open_file_intents.dif
> 
> Comments:
> 
> >  /*
> > + * Open intents have to release any file pointer that was allocated
> > + * but not used by the VFS.
> > + */
> > +void path_release_open_intent(struct nameidata *nd)
> > +{
> > +	if ((nd->flags & LOOKUP_OPEN) && nd->intent.open.file != NULL) {
> > +		fput(nd->intent.open.file);
> 
> I think you should consider adding this:
> 
> +		if (!IS_ERR(nd->intent.open.file))
> +			fput(nd->intent.open.file);
> 
> so the filesystem can delay returning the error from the open
> operation until the other errors have been sorted out by the lookup
> code.

Intents are meant as optimisations, not replacements for existing
operations. I'm therefore not really comfortable about having them
return errors at all.

> > +		nd->intent.open.file = NULL;
> 
> Why is this NULL assignment needed?  nd will not be used after this.
> 
> > +	}
> > +	path_release(nd);
> > +}
> > +
> > 

It could be dropped. The reason for putting it in is that some parts of
the VFS may restart a path walk operation if it fails (see for instance
the ESTALE handling).

> > As for the "orig flags" thing. What is the point of that?
> 
> dentry_open() needs the original open flags, not the transformed ones
> stored in intent.open.flags.
> 
> The behavior is slightly strange, since filp_open() calculates
> namei_flags (which gets stored in intent.open.flags) so that an
> O_ACCMODE of 3 is transformed into FMODE_READ | FMODE_WRITE.
> 
> But dentry_open() calculates filp->f_mode, so that O_ACCMODE of 3 is
> transformed into zero.
> 
> This means that the (undocumented) access mode of 3 will require
> read-write permission, but will allow neither read() nor write() on
> the opened file.
> 
> If we want to keep this behavior, then the orig_flags field is needed.

Why do we want to keep this behaviour? It is undocumented, it is
non-posix, and it appears to do nothing you cannot do with the existing
access() call.

Are there any applications using it? If so, which ones, and why?

Cheers,
  Trond

Re: [RFC] atomic open(..., O_CREAT | ...)

[Bryan Henderson]

>Intents are meant as optimisations, not replacements for existing
>operations. I'm therefore not really comfortable about having them
>return errors at all.

That's true of normal intents, but not what are called intents here.  A 
normal intent merely expresses an intent, and it can be totally ignored 
without harm to correctness.  But these "intents" were designed to be 
responded to by actually performing the foreshadowed operation now - 
irreversibly.

Linux needs an atomic lookup/open/create in order to participate in a 
shared filesystem and provide a POSIX interface (where shared filesystem 
means a filesystem that is simultaneously accessed by something besides 
the Linux system in question).  Some operating systems do this simply with 
a VFS lookup/open/create function.  Linux does it with this intents 
interface.

It's hard to merge the concepts in code or in one's mind, which is why 
we're here now.  A filesystem driver that needs to do atomic 
lookup/open/create has to bend over backwards to split the operation 
across the three filesystem driver calls that Linux wants to make.

I've always preferred just to have a new inode operation for 
lookup/open/create (mirroring the POSIX open operation, used for all opens 
if available), but if enough arguments to lookup can do it, that's 
practically as good.  But that means returning final status from lookup, 
and not under any circumstance proceeding to create or open when the 
filesystem driver has said the entire operation is complete.

--
Bryan Henderson                     IBM Almaden Research Center
San Jose CA                         Filesystems

Re: [RFC] atomic open(..., O_CREAT | ...)

[Trond Myklebust]

ty den 09.08.2005 Klokka 08:47 (-0700) skreiv Bryan Henderson:
> >Intents are meant as optimisations, not replacements for existing
> >operations. I'm therefore not really comfortable about having them
> >return errors at all.
> 
> That's true of normal intents, but not what are called intents here.  A 
> normal intent merely expresses an intent, and it can be totally ignored 
> without harm to correctness.  But these "intents" were designed to be 
> responded to by actually performing the foreshadowed operation now - 
> irreversibly.
> 
> Linux needs an atomic lookup/open/create in order to participate in a 
> shared filesystem and provide a POSIX interface (where shared filesystem 
> means a filesystem that is simultaneously accessed by something besides 
> the Linux system in question).  Some operating systems do this simply with 
> a VFS lookup/open/create function.  Linux does it with this intents 
> interface.
> 
> It's hard to merge the concepts in code or in one's mind, which is why 
> we're here now.  A filesystem driver that needs to do atomic 
> lookup/open/create has to bend over backwards to split the operation 
> across the three filesystem driver calls that Linux wants to make.
> 
> I've always preferred just to have a new inode operation for 
> lookup/open/create (mirroring the POSIX open operation, used for all opens 
> if available), but if enough arguments to lookup can do it, that's 
> practically as good.  But that means returning final status from lookup, 
> and not under any circumstance proceeding to create or open when the 
> filesystem driver has said the entire operation is complete.

Have you looked at how we're dealing with this in NFSv4?

Cheers,
  Trond

Re: [RFC] atomic open(..., O_CREAT | ...)

[Bryan Henderson]

>Have you looked at how we're dealing with this in NFSv4?

No.

--
Bryan Henderson                     IBM Almaden Research Center
San Jose CA                         Filesystems

Re: [RFC] atomic open(..., O_CREAT | ...)

[Miklos Szeredi]

> Intents are meant as optimisations, not replacements for existing
> operations. I'm therefore not really comfortable about having them
> return errors at all.

In my case they are not an optimization, rather the only way to
correctly perform an open with O_CREAT.

> > > +		nd->intent.open.file = NULL;
> > 
> > Why is this NULL assignment needed?  nd will not be used after this.
> > 
> > > +	}
> > > +	path_release(nd);
> > > +}
> > > +
> > > 
> 
> It could be dropped. The reason for putting it in is that some parts of
> the VFS may restart a path walk operation if it fails (see for instance
> the ESTALE handling).

If you use the nameidata after path_release_open_intent(), you're
screwed anyway, since nd->mnt and nd->dentry have already been
released.

If there's any chance that the path walk restart thing will invoke the
filesystems open code twice (I doubt it), then the filesystem must
make sure to check intent.open.file, whether it has already been set,
and fput() it before setting it another time.

> Why do we want to keep this behaviour? It is undocumented, it is
> non-posix, and it appears to do nothing you cannot do with the existing
> access() call.
> 
> Are there any applications using it? If so, which ones, and why?

I have absolutely no idea. 

Looking closer, there's a problem with O_TRUNC as well:

	namei_flags = flags;
	if ((namei_flags+1) & O_ACCMODE)
		namei_flags++;
	if (namei_flags & O_TRUNC)
		namei_flags |= 2;

So if flags is O_RDONLY|O_TRUNC, intent.open.flags will be
FMODE_WRITE|FMODE_READ|O_TRUNC, but filp->f_mode will be FMODE_READ.

This is also undocumented (or rather documented to be undefined), but
the behavior is perfectly logical, and I can imagine some application
relying on it.

Miklos

Re: [RFC] atomic open(..., O_CREAT | ...)

[Trond Myklebust]

ty den 09.08.2005 Klokka 18:40 (+0200) skreiv Miklos Szeredi:
> > Intents are meant as optimisations, not replacements for existing
> > operations. I'm therefore not really comfortable about having them
> > return errors at all.
> 
> In my case they are not an optimization, rather the only way to
> correctly perform an open with O_CREAT.

How so? In NFS the only case that can be tricky is O_EXCL, and that
simply _cannot_ be done inside nfs_lookup without confusing the VFS. 
Instead, we always create a negative dentry, and then do the actual
atomic exclusive create inside vfs_create().

We can do the ordinary atomic open(O_CREAT) inside nfs_lookup though. If
that fails, then we simply return the error as an ERR_PTR in the dentry
(i.e. the lookup won't succeed).

The only case which we have that can be iffy is the case of open() on an
existing dentry. If that fails, then we drop the dentry and force an
uncached lookup in order to get it all right.

> > > > +		nd->intent.open.file = NULL;
> > > 
> > > Why is this NULL assignment needed?  nd will not be used after this.
> > > 
> > > > +	}
> > > > +	path_release(nd);
> > > > +}
> > > > +
> > > > 
> > 
> > It could be dropped. The reason for putting it in is that some parts of
> > the VFS may restart a path walk operation if it fails (see for instance
> > the ESTALE handling).
> 
> If you use the nameidata after path_release_open_intent(), you're
> screwed anyway, since nd->mnt and nd->dentry have already been
> released.

There is quite a bit of code out there that assumes it is free to stuff
things into nd->mnt and nd->dentry. Some of it is Al Viro's code, some
of it is from other people.
For instance, the ESTALE handling will just save nd->mnt/nd->dentry
before calling __link_path_walk(), then restore + retry if the ESTALE
error comes up.

> If there's any chance that the path walk restart thing will invoke the
> filesystems open code twice (I doubt it), then the filesystem must
> make sure to check intent.open.file, whether it has already been set,
> and fput() it before setting it another time.

The only user of that code is NFSv4, and we will never even try to
allocate a file if the OPEN call returned an ESTALE.

> > Why do we want to keep this behaviour? It is undocumented, it is
> > non-posix, and it appears to do nothing you cannot do with the existing
> > access() call.
> > 
> > Are there any applications using it? If so, which ones, and why?
> 
> I have absolutely no idea. 
> 
> Looking closer, there's a problem with O_TRUNC as well:
> 
> 	namei_flags = flags;
> 	if ((namei_flags+1) & O_ACCMODE)
> 		namei_flags++;
> 	if (namei_flags & O_TRUNC)
> 		namei_flags |= 2;
> 
> So if flags is O_RDONLY|O_TRUNC, intent.open.flags will be
> FMODE_WRITE|FMODE_READ|O_TRUNC, but filp->f_mode will be FMODE_READ.

That is a bug that needs to be fixed in the intent.open.flags. We don't
ever want to be opening the file for writing at the filesystem level
when the user specified open for read.

Cheers,
  Trond

Re: [RFC] atomic open(..., O_CREAT | ...)

[Miklos Szeredi]

> > > > > +		nd->intent.open.file = NULL;
> > > > 
> > > > Why is this NULL assignment needed?  nd will not be used after this.
> > > > 
> > > > > +	}
> > > > > +	path_release(nd);
> > > > > +}
> > > > > +
> > > > > 
> > > 
> > > It could be dropped. The reason for putting it in is that some parts of
> > > the VFS may restart a path walk operation if it fails (see for instance
> > > the ESTALE handling).
> > 
> > If you use the nameidata after path_release_open_intent(), you're
> > screwed anyway, since nd->mnt and nd->dentry have already been
> > released.
> 
> There is quite a bit of code out there that assumes it is free to stuff
> things into nd->mnt and nd->dentry. Some of it is Al Viro's code, some
> of it is from other people.
> For instance, the ESTALE handling will just save nd->mnt/nd->dentry
> before calling __link_path_walk(), then restore + retry if the ESTALE
> error comes up.

Yeah, but how is that relevant to the fact, that after
path_release_*() _nothing_ will be valid in the nameidata, not
nd->mnt, not nd->dentry, and not nd->intent.open.file.  So what's the
point in setting it to NULL if it must never be used anyway?

> > If there's any chance that the path walk restart thing will invoke the
> > filesystems open code twice (I doubt it), then the filesystem must
> > make sure to check intent.open.file, whether it has already been set,
> > and fput() it before setting it another time.
> 
> The only user of that code is NFSv4, and we will never even try to
> allocate a file if the OPEN call returned an ESTALE.

That's why I doubted that this is an issue.

> > > Why do we want to keep this behaviour? It is undocumented, it is
> > > non-posix, and it appears to do nothing you cannot do with the existing
> > > access() call.
> > > 
> > > Are there any applications using it? If so, which ones, and why?
> > 
> > I have absolutely no idea. 
> > 
> > Looking closer, there's a problem with O_TRUNC as well:
> > 
> > 	namei_flags = flags;
> > 	if ((namei_flags+1) & O_ACCMODE)
> > 		namei_flags++;
> > 	if (namei_flags & O_TRUNC)
> > 		namei_flags |= 2;
> > 
> > So if flags is O_RDONLY|O_TRUNC, intent.open.flags will be
> > FMODE_WRITE|FMODE_READ|O_TRUNC, but filp->f_mode will be FMODE_READ.
> 
> That is a bug that needs to be fixed in the intent.open.flags. We don't
> ever want to be opening the file for writing at the filesystem level
> when the user specified open for read.

No, it's being opened for read.  The namei_flags (and hence
intent.open.flags) will have FMODE_WRITE, so that the permission is
checked for write.

So I thing the bug is not in the calculation of namei_flags, rather
the fact that intent.open.flags is set to namei_flags, rather than the
original open flags.  

Miklos

Re: [RFC] atomic open(..., O_CREAT | ...)

[Trond Myklebust]

ty den 09.08.2005 Klokka 19:44 (+0200) skreiv Miklos Szeredi:

> > There is quite a bit of code out there that assumes it is free to stuff
> > things into nd->mnt and nd->dentry. Some of it is Al Viro's code, some
> > of it is from other people.
> > For instance, the ESTALE handling will just save nd->mnt/nd->dentry
> > before calling __link_path_walk(), then restore + retry if the ESTALE
> > error comes up.
> 
> Yeah, but how is that relevant to the fact, that after
> path_release_*() _nothing_ will be valid in the nameidata, not
> nd->mnt, not nd->dentry, and not nd->intent.open.file.  So what's the
> point in setting it to NULL if it must never be used anyway?

path_release() does _not_ invalidate the nameidata. Look for instance at
__emul_lookup_dentry(), which clearly makes use of that fact.

> > > If there's any chance that the path walk restart thing will invoke the
> > > filesystems open code twice (I doubt it), then the filesystem must
> > > make sure to check intent.open.file, whether it has already been set,
> > > and fput() it before setting it another time.
> > 
> > The only user of that code is NFSv4, and we will never even try to
> > allocate a file if the OPEN call returned an ESTALE.
> 
> That's why I doubted that this is an issue.
> 
> > > > Why do we want to keep this behaviour? It is undocumented, it is
> > > > non-posix, and it appears to do nothing you cannot do with the existing
> > > > access() call.
> > > > 
> > > > Are there any applications using it? If so, which ones, and why?
> > > 
> > > I have absolutely no idea. 
> > > 
> > > Looking closer, there's a problem with O_TRUNC as well:
> > > 
> > > 	namei_flags = flags;
> > > 	if ((namei_flags+1) & O_ACCMODE)
> > > 		namei_flags++;
> > > 	if (namei_flags & O_TRUNC)
> > > 		namei_flags |= 2;
> > > 
> > > So if flags is O_RDONLY|O_TRUNC, intent.open.flags will be
> > > FMODE_WRITE|FMODE_READ|O_TRUNC, but filp->f_mode will be FMODE_READ.
> > 
> > That is a bug that needs to be fixed in the intent.open.flags. We don't
> > ever want to be opening the file for writing at the filesystem level
> > when the user specified open for read.
> 
> No, it's being opened for read.  The namei_flags (and hence
> intent.open.flags) will have FMODE_WRITE, so that the permission is
> checked for write.

Firstly, the open_namei() flags field is not a "permissions" field. It
contains open mode information. The calculation of the open permissions
flags is done by open_namei() itself.

Secondly, what advantage is there in allowing callers of open_namei() to
be able to override the MAY_WRITE check when doing open(O_TRUNC)? This
is a calculation that should be done _once_ in order to always get it
right, and it should therefore be done in open_namei() together with the
rest of the permissions calculation.

Cheers,
  Trond

Re: [RFC] atomic open(..., O_CREAT | ...)

[Miklos Szeredi]

> > > There is quite a bit of code out there that assumes it is free to stuff
> > > things into nd->mnt and nd->dentry. Some of it is Al Viro's code, some
> > > of it is from other people.
> > > For instance, the ESTALE handling will just save nd->mnt/nd->dentry
> > > before calling __link_path_walk(), then restore + retry if the ESTALE
> > > error comes up.
> > 
> > Yeah, but how is that relevant to the fact, that after
> > path_release_*() _nothing_ will be valid in the nameidata, not
> > nd->mnt, not nd->dentry, and not nd->intent.open.file.  So what's the
> > point in setting it to NULL if it must never be used anyway?
> 
> path_release() does _not_ invalidate the nameidata. Look for instance at
> __emul_lookup_dentry(), which clearly makes use of that fact.

Trond, wake up!  __emul_lookup_dentry() does nothing of the sort.
Neither does anything else.  In theory it could, but that's not a
reason to do a confusing thing like that.

> Firstly, the open_namei() flags field is not a "permissions" field. It
> contains open mode information. The calculation of the open permissions
> flags is done by open_namei() itself.

Based on flags.  It's just a FMODE_* -> MAY_* transformation

> Secondly, what advantage is there in allowing callers of open_namei() to
> be able to override the MAY_WRITE check when doing open(O_TRUNC)? This
> is a calculation that should be done _once_ in order to always get it
> right, and it should therefore be done in open_namei() together with the
> rest of the permissions calculation.

I think the _only_ caller of open_namei() is filp_open(), so this is
not much of an issue, but yeah, you could do it that way too.

Or you could initialize nameidata from filp_open().

Miklos

Re: [RFC] atomic open(..., O_CREAT | ...)

[Trond Myklebust]

ty den 09.08.2005 Klokka 22:42 (+0200) skreiv Miklos Szeredi:

> Trond, wake up!  __emul_lookup_dentry() does nothing of the sort.
> Neither does anything else.  In theory it could, but that's not a
> reason to do a confusing thing like that.

Really?

static int __emul_lookup_dentry(const char *name, struct nameidata *nd)
{
		.....
		if (path_walk(name, nd) == 0) {
			if (nd->dentry->d_inode) {
				dput(old_dentry);
				mntput(old_mnt);
				return 1;
			}
			path_release(nd);
		}
		nd->dentry = old_dentry;
		nd->mnt = old_mnt;
		nd->last = last;
		nd->last_type = last_type;
	}
	return 1;
}

Which is called by path_lookup(), which again returns success, and
expects the user to call path_release() later.

> > Firstly, the open_namei() flags field is not a "permissions" field. It
> > contains open mode information. The calculation of the open permissions
> > flags is done by open_namei() itself.
> 
> Based on flags.  It's just a FMODE_* -> MAY_* transformation
> 
> > Secondly, what advantage is there in allowing callers of open_namei() to
> > be able to override the MAY_WRITE check when doing open(O_TRUNC)? This
> > is a calculation that should be done _once_ in order to always get it
> > right, and it should therefore be done in open_namei() together with the
> > rest of the permissions calculation.
> 
> I think the _only_ caller of open_namei() is filp_open(), so this is
> not much of an issue, but yeah, you could do it that way too.

Currently, yes. The only caller of open_namei() is filp_open(). That was
not always the case previously.

If we think it will never be the case in the future, then there is an
argument for merging the two and/or making open_namei() and inlined
function.

Cheers,
  Trond

Re: [RFC] atomic open(..., O_CREAT | ...)

[Miklos Szeredi]

> Really?
> 
> static int __emul_lookup_dentry(const char *name, struct nameidata *nd)
> {
> 		.....
> 		if (path_walk(name, nd) == 0) {
> 			if (nd->dentry->d_inode) {
> 				dput(old_dentry);
> 				mntput(old_mnt);
> 				return 1;
> 			}
> 			path_release(nd);
> 		}
> 		nd->dentry = old_dentry;
> 		nd->mnt = old_mnt;
> 		nd->last = last;
> 		nd->last_type = last_type;
> 	}
> 	return 1;
> }

I see what you are getting at.  But notice, that every (relevant)
field of nameidata is reinitialized, except nd->flags, which is
_obviously_ not changed by either path_walk() or path_release().

So your argument doesn't hold.

You basically argue, that intent.open.file must be zeroed, because
someone might call path_release_open_intent() twice, which very
obviously does not make any sense, unless it does some magic like the
above (which it should not), in which case it might as well be aware,
that it has to save/restore the intent.open.file field as well.

> Currently, yes. The only caller of open_namei() is filp_open(). That was
> not always the case previously.
> 
> If we think it will never be the case in the future, then there is an
> argument for merging the two and/or making open_namei() and inlined
> function.

Yes, that would make sense.

Miklos