Create a temporary struct vfs_lookup in file_permission() instead of
passing a NULL value.
Signed-off-by: Andreas Gruenbacher <[email protected]>
---
fs/namei.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -292,14 +292,15 @@ int vfs_permission(struct vfs_lookup *lo
*
* Used to check for read/write/execute permissions on an already opened
* file.
- *
- * Note:
- * Do not use this function in new code. All access checks should
- * be done using vfs_permission().
*/
int file_permission(struct file *file, int mask)
{
- return permission(file->f_path.dentry->d_inode, mask, NULL);
+ struct vfs_lookup lookup;
+
+ lookup.path = file->f_path;
+ lookup.flags = 0;
+
+ return permission(file->f_path.dentry->d_inode, mask, &lookup);
}
/*
On Wed, Aug 08, 2007 at 07:16:26PM +0200, Andreas Gruenbacher wrote:
> Create a temporary struct vfs_lookup in file_permission() instead of
> passing a NULL value.
>
> Signed-off-by: Andreas Gruenbacher <[email protected]>
>
> ---
> fs/namei.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -292,14 +292,15 @@ int vfs_permission(struct vfs_lookup *lo
> *
> * Used to check for read/write/execute permissions on an already opened
> * file.
> - *
> - * Note:
> - * Do not use this function in new code. All access checks should
> - * be done using vfs_permission().
Should this comment be removed?
> */
> int file_permission(struct file *file, int mask)
> {
> - return permission(file->f_path.dentry->d_inode, mask, NULL);
> + struct vfs_lookup lookup;
> +
> + lookup.path = file->f_path;
> + lookup.flags = 0;
I tend to find this little bit cleaner:
struct vfs_lookup lookup = {
.path = file->f_path,
.flags = 0,
};
> +
> + return permission(file->f_path.dentry->d_inode, mask, &lookup);
> }
>
> /*
--
Humans were created by water to transport it upward.
On Wednesday 08 August 2007 19:58, Josef Sipek wrote:
> On Wed, Aug 08, 2007 at 07:16:26PM +0200, Andreas Gruenbacher wrote:
> > Create a temporary struct vfs_lookup in file_permission() instead of
> > passing a NULL value.
> >
> > Signed-off-by: Andreas Gruenbacher <[email protected]>
> >
> > ---
> > fs/namei.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -292,14 +292,15 @@ int vfs_permission(struct vfs_lookup *lo
> > *
> > * Used to check for read/write/execute permissions on an already opened
> > * file.
> > - *
> > - * Note:
> > - * Do not use this function in new code. All access checks should
> > - * be done using vfs_permission().
>
> Should this comment be removed?
IMO yes. If vfs_permission() works for a piece of code it's the obvious
preference. If on the other hand you really need to check permissions on a
file, then why not use this function?
> > */
> > int file_permission(struct file *file, int mask)
> > {
> > - return permission(file->f_path.dentry->d_inode, mask, NULL);
> > + struct vfs_lookup lookup;
> > +
> > + lookup.path = file->f_path;
> > + lookup.flags = 0;
>
> I tend to find this little bit cleaner:
>
> struct vfs_lookup lookup = {
> .path = file->f_path,
> .flags = 0,
> };
I didn't use initializers because they initialize the entire data structure.
In case of struct vfs_lookup, unless the LOOKUP_OPEN flag is set, then the
open_intent doesn't need initialization. We could use a DEFINE_... macro; not
sure this would improve anything though.
> > +
> > + return permission(file->f_path.dentry->d_inode, mask, &lookup);
> > }
> >
> > /*
Thanks,
Andreas
On Wed, Aug 08, 2007 at 07:16:26PM +0200, Andreas Gruenbacher wrote:
> Create a temporary struct vfs_lookup in file_permission() instead of
> passing a NULL value.
NACK. file_permission is special in that it doesn't happen in the
context of any kind of lookup operation, and the nd/intent paramater
to ->permission should be NULL in that case instead of faking up some crap.
On Wednesday 08 August 2007 21:25, Christoph Hellwig wrote:
> On Wed, Aug 08, 2007 at 07:16:26PM +0200, Andreas Gruenbacher wrote:
> > Create a temporary struct vfs_lookup in file_permission() instead of
> > passing a NULL value.
>
> NACK. file_permission is special in that it doesn't happen in the
> context of any kind of lookup operation, and the nd/intent paramater
> to ->permission should be NULL in that case instead of faking up some crap.
Lookup or not doesn't actually matter. Think of fchdir(2): it does a
permission check, and it should also pass down the LOOKUP_CHDIR flag. (Yes I
know, it doesn't do that right now. Bug.) I can't think of a better example
right now, but the intent does not only make sense in lookup context.
It's true that filesystems should never touch vfsmnts -- except for a few rare
exceptions. Filesystem stacking is one. NFS silly-rename is another: if the
vfsmnt of the object being silly-renamed were passed down to the file system,
we would mntget() it. Right now there is a reference counting bug that allows
to blow up the kernel by unmounting that mount point before the silly-renamed
file is closed. (It's client-side only of course, but still.) The vfsmnt that
this patch passes down in file_permission() is not some crap as you chose to
call it, it's the appropriate vfsmnt.
Last but not least, file_permission() is a vfs function not a filesystem
operation. It indirectly calls into security_inode_permission(). We need the
vfsmnt there for path-based LSMs, for operations like fchmod(2). But that's a
different set of patches, and a different discussion.
Thanks,
Andreas
On Wed, Aug 08, 2007 at 11:41:06PM +0200, Andreas Gruenbacher wrote:
> Lookup or not doesn't actually matter. Think of fchdir(2): it does a
> permission check, and it should also pass down the LOOKUP_CHDIR flag.
fchdir per defintion doesn't do any lookup, and it should not pretend to be
doing one.
> It's true that filesystems should never touch vfsmnts -- except for a few rare
> exceptions. Filesystem stacking is one. NFS silly-rename is another: if the
> vfsmnt of the object being silly-renamed were passed down to the file system,
> we would mntget() it. Right now there is a reference counting bug that allows
> to blow up the kernel by unmounting that mount point before the silly-renamed
> file is closed. (It's client-side only of course, but still.)
Wrong. Remember what we call unmount is two underlying operations:
- detach the subtree from the namespace, this is the vfsmount-based operation.
this one couldn't care less about an in-progress silly-rename
- actually teard down the filesystem. this is a superblock-related
operation, and you want your reference counting for the above case
to be on the superblock level if at all. A good explanation of
the bug you're seeing and how you intend to fix it outside of this
slightly heated thread might help, though..
Remember that passing down the vfsmount to the filesystem for namespace
operations is actually harmful, because all the namespace operations must
operate independent of the actual view (aka vfsmount) it's coming from -
all vfsmounts shared a single dentry subtree and operation on either of
them must give the same results.
> The vfsmnt that
> this patch passes down in file_permission() is not some crap as you chose to
> call it, it's the appropriate vfsmnt.
No, it's wrong. There is no path except for informal purposes attached to
a struct file. I created file_permission to document that clearly and
people don't try things like this. There's lots of chances where fs passing
has happened, were into a lazily detached tree, after a pviot_root, etc where
trying to do anything that remotely looks like pathname based operation just
doesn't make any sense in this case.
On Thursday 09 August 2007 01:24, Christoph Hellwig wrote:
> On Wed, Aug 08, 2007 at 11:41:06PM +0200, Andreas Gruenbacher wrote:
> > Lookup or not doesn't actually matter. Think of fchdir(2): it does a
> > permission check, and it should also pass down the LOOKUP_CHDIR flag.
>
> fchdir per defintion doesn't do any lookup, and it should not pretend to be
> doing one.
fchdir doesn't pretend to do a lookup by passing the LOOKUP_CHDIR flag, it
indicates that a chdir is being done. The lookup happened earlier at a time
at which the kernel didn't know that a chdir would follow. If fchdir doesn't
pass down the LOOKUP_CHDIR flag, then that flag is pretty pointless: it could
be bypassed any time by an open followed by fchdir.
> > The vfsmnt that this patch passes down in file_permission() is not some
> > crap as you chose to call it, it's the appropriate vfsmnt.
>
> No, it's wrong. There is no path except for informal purposes attached to
> a struct file.
I'm sorry, but the dentry and vfsmnt in struct file is *not* only used for
informational purposes: openat does a lookup relative to a file's dentry and
vfsmnt, and there are other examples as well.
The dentry and vfsmnt identify a particular object in the filesystem
hierarchy. As long as the file hasn't been deleted, they can be used to
compute a pathname that leads to the file (if the file is reachable at all).
You can used that pathname for more than informational purposes. It is real,
and canonical too.
> I created file_permission to document that clearly and people don't try
> things like this. There's lots of chances where fs passing has happened,
^ you must mean fd passing
> were into a lazily detached tree, after a pviot_root, etc where trying to do
> anything that remotely looks like pathname based operation just doesn't make
> any sense in this case.
What you are saying doesn't make sense. It makes perfect sense to compute the
pathname of an open file in some situations. Obviously it does not make sense
when the file has been deleted, and you really do want to detect when a file
has become unreachable (lazily unmounted, other namespace, whatever). You
will always find that out when trying to compute its pathname, so that's not
an issue. (Caveat: this requires the d_path fixes I have posted earlier.)
Thanks,
Andreas