On Mon, 30 Mar 2015 11:27:25 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Mon, Mar 30, 2015 at 04:38:23PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Mar 30, 2015 at 10:23:10AM -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@xxxxxxxxxx>
> > >
> > > Subject: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions
> > >
> > > Debugfs files and and directories are created by kernel subsystems not
> > > directly by users, so we shouldn't be using lookup_one_len, which checks
> > > permissions.
> > >
> > > This was causing krb5 mounts to fail to Fedora servers using gss-proxy
> > > if selinux was enabled, on kernels since 388f0c776781 "sunrpc: add a
> > > debugfs rpc_xprt directory with an info file in it", which creates a new
> > > debugfs directory for each new rpc client.
> >
> > No kernel code should care / fail if a debugfs function fails, so please
> > fix up the sunrpc code first.
>
> The sunrpc code is using debugfs to keep a list of all rpc clients
> together with some basic information about each one.
>
> If the permissions issue is fixed then I think the only possible
> failures are lack of debugfs support and ENOMEM?
>
> If there are situations where ENOMEM's actually possible, I'd worry a
> bit about this debugging information becoming unreliable after you go
> through some extreme memory shortage (possibly because that's what was
> necessary to reproduce the bug you're chasing).
>
> But, I don't know, maybe I'm excessively paranoid; Jeff?
>
The debugfs files are really just nice-to-have things. I don't think
you can read too much into it if they don't show up for some reason.
So, I'm fine with turning those into non-fatal errors.
> > > Reported-by: Anthony Messina <amessina@xxxxxxxxxxxx>
> > > Reported-by: Jason Tibbits <tibbs@xxxxxxxxxxx>
> > > Cc: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> > > Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
> > > ---
> > > fs/debugfs/inode.c | 17 +++++++++++++----
> > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > I swiped this code fragment from net/sunrpc/rpc_pipe.c, and it's gotten
> > > only minimal testing. (It does fix krb5 mounts, though.)
> > >
> > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > > index 96400ab42d13..75e5daa6a63f 100644
> > > --- a/fs/debugfs/inode.c
> > > +++ b/fs/debugfs/inode.c
> > > @@ -251,6 +251,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
> > > {
> > > struct dentry *dentry;
> > > int error;
> > > + struct qstr q = QSTR_INIT(name, strlen(name));
> > >
> > > pr_debug("debugfs: creating file '%s'\n",name);
> > >
> > > @@ -268,11 +269,19 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
> > > parent = debugfs_mount->mnt_root;
> > >
> > > mutex_lock(&parent->d_inode->i_mutex);
> > > - dentry = lookup_one_len(name, parent, strlen(name));
> > > - if (!IS_ERR(dentry) && dentry->d_inode) {
> > > + dentry = d_hash_and_lookup(parent, &q);
> > > + if (!dentry) {
> > > + dentry = d_alloc(parent, &q);
> > > + if (!dentry) {
> > > + dentry = ERR_PTR(-ENOMEM);
> > > + goto out;
> > > + }
> > > + }
> > > + if (dentry->d_inode) {
> >
> >
> > No, I'd rather not "open code" lookup_one_len() if at all possible
> > please.
> >
> > What exactly is the problem here that the sunrpc code is failing from?
>
> Sunrpc is creating a new rpc client (to talk to an rpc server).
>
> Debugfs will fail to do that whenever the user doesn't have execute
> permissions on a debugfs directory. The user in this case is whoever
> gss-proxy's running as, but I think it could just as well be someone
> doing an nfs mount, for example.
>
> > Is it just interacting with selinux? How is the debugfs code to blame
> > here?
>
> As long as the debugfs directories permit "x" to everone, I guess you're
> only going to hit this with selinux (or some other security module).
>
> We don't want to require updated selinux policies just because some
> subsystem started creating debugfs entries as a side-effect of some
> system call (which is what happened here).
>
> --b.
Right.
--
Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
--
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
