linux-kernel - Re: [PATCH] mm/backing-dev: Check return value of the debugfs_create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150826092302.GB3871@quack.suse.cz>
Date:	Wed, 26 Aug 2015 11:23:02 +0200
From:	Jan Kara <jack@...e.cz>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Alexander Kuleshov <kuleshovmail@...il.com>,
	Tejun Heo <tj@...nel.org>, Jan Kara <jack@...e.cz>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/backing-dev: Check return value of the
 debugfs_create_dir()

On Tue 25-08-15 14:08:58, Andrew Morton wrote:
> On Tue, 25 Aug 2015 13:54:23 +0600 Alexander Kuleshov <kuleshovmail@...il.com> wrote:
> 
> > The debugfs_create_dir() function may fail and return error. If the
> > root directory not created, we can't create anything inside it. This
> > patch adds check for this case.
> > 
> > ...
> >
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -117,15 +117,21 @@ static const struct file_operations bdi_debug_stats_fops = {
> >  
> >  static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
> >  {
> > -	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> > -	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
> > -					       bdi, &bdi_debug_stats_fops);
> > +	if (bdi_debug_root) {
> > +		bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> > +		if (bdi->debug_dir)
> > +			bdi->debug_stats = debugfs_create_file("stats", 0444,
> > +							bdi->debug_dir, bdi,
> > +							&bdi_debug_stats_fops);
> > +	}
> 
> If debugfs_create_dir() fails, debugfs_create_file() will go ahead and
> attempt to create the debugfs file in the debugfs root directory:
> 
> : static struct dentry *start_creating(const char *name, struct dentry *parent)
> : {
> : ...
> : 	/* If the parent is not specified, we create it in the root.
> : 	 * We need the root dentry to do this, which is in the super
> : 	 * block. A pointer to that is in the struct vfsmount that we
> : 	 * have around.
> : 	 */
> : 	if (!parent)
> : 		parent = debugfs_mount->mnt_root;
> 
> I'm not sure that this is very useful behaviour, and putting the files
> in the wrong place is a very obscure way of informing the user that
> debugfs_create_dir() failed :(

But this patch actually makes sure that we don't call debugfs_create_dir()
and debugfs_create_file() with parent == NULL so this patch avoids creation
of entries in debugfs root. So IMHO it really improves the situation. And I
agree with you that falling back to debugfs root is just broken...

> I don't think it's worth making little changes such as this - handling
> debugfs failures needs a deeper rethink.

Well, handling debugfs failures like in this patch is the right way to go,
isn't it? Or what else would you imagine than checking for errors and
bailing out instead of trying to create entries in non-existent dirs?

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/