On Fri, Sep 02, 2011 at 20:37 +0400, Vasiliy Kulikov wrote:
> On Thu, Sep 01, 2011 at 12:05 +0400, Cyrill Gorcunov wrote:
> > ...
> > > > +/*
> > > > + * NOTE: The getattr/setattr for both /proc/$pid/map_files and
> > > > + * /proc/$pid/fd seems to have share the code, so need to be
> > > > + * unified and code duplication eliminated!
> > >
> > > Why not do this now?
> >
> > There are a couple of reasons. Yesterday I was talking to
> > Vasiliy Kulikov about this snippet, so he seems about to send
> > you patches related to /proc/$pid/fd update, and after those
> > patches will be merged we are to drop code duplication.
> > Vasiliy, what the status of the update?
>
> It looks like protecting directories with sensible contents is a nasty
> thing. The problem here is that if the dentry is present in the cache,
> ->lookup() is not called at all and the permissions can be checked in
> fop/dop/iop specific handler (getattr(), readlink(), etc.). However, it
> would be much simplier to hook ->lookup() only. Otherwise, we have to
> define procfs handlers for all operations, which don't call
> ->d_revalidate().
>
> Is it possible to disable caching dentry for specific files? It is not
> performace critical thing in fd and map_files and it would much simplify
> the task. Creating handlers for all these op handler bloats procfs.
Looks like the following patch solves the problem. Tested on stat() and
link().
diff --git a/fs/proc/base.c b/fs/proc/base.c
index d44c701..219588b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1665,46 +1665,12 @@ out:
return error;
}
-static int proc_pid_fd_link_getattr(struct vfsmount *mnt, struct dentry *dentry,
- struct kstat *stat)
-{
- struct inode *inode = dentry->d_inode;
- struct task_struct *task = get_proc_task(inode);
- int rc;
-
- if (task == NULL)
- return -ESRCH;
-
- rc = -EACCES;
- if (lock_trace(task))
- goto out_task;
-
- generic_fillattr(inode, stat);
- unlock_trace(task);
- rc = 0;
-out_task:
- put_task_struct(task);
- return rc;
-}
-
static const struct inode_operations proc_pid_link_inode_operations = {
.readlink = proc_pid_readlink,
.follow_link = proc_pid_follow_link,
.setattr = proc_setattr,
};
-static const struct inode_operations proc_fdinfo_link_inode_operations = {
- .setattr = proc_setattr,
- .getattr = proc_pid_fd_link_getattr,
-};
-
-static const struct inode_operations proc_fd_link_inode_operations = {
- .readlink = proc_pid_readlink,
- .follow_link = proc_pid_follow_link,
- .setattr = proc_setattr,
- .getattr = proc_pid_fd_link_getattr,
-};
-
/* building an inode */
@@ -2044,9 +2010,18 @@ static int tid_fd_revalidate(struct dentry *dentry, struct nameidata *nd)
return 0;
}
+static int pid_no_revalidate(struct dentry *dentry, struct nameidata *nd)
+{
+ if (nd && nd->flags & LOOKUP_RCU)
+ return -ECHILD;
+
+ d_drop(dentry);
+ return 0;
+}
+
static const struct dentry_operations tid_fd_dentry_operations =
{
- .d_revalidate = tid_fd_revalidate,
+ .d_revalidate = pid_no_revalidate,
.d_delete = pid_delete_dentry,
};
@@ -2085,7 +2060,7 @@ static struct dentry *proc_fd_instantiate(struct inode *dir,
spin_unlock(&files->file_lock);
put_files_struct(files);
- inode->i_op = &proc_fd_link_inode_operations;
+ inode->i_op = &proc_pid_link_inode_operations;
inode->i_size = 64;
ei->op.proc_get_link = proc_fd_link;
d_set_d_op(dentry, &tid_fd_dentry_operations);
@@ -2267,7 +2242,6 @@ static struct dentry *proc_fdinfo_instantiate(struct inode *dir,
ei->fd = fd;
inode->i_mode = S_IFREG | S_IRUSR;
inode->i_fop = &proc_fdinfo_file_operations;
- inode->i_op = &proc_fdinfo_link_inode_operations;
d_set_d_op(dentry, &tid_fd_dentry_operations);
d_add(dentry, inode);
/* Close the race of the process dying before we return the dentry */
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
