linux-kernel - Re: [PATCH] xen/debugfs: Check debugfs initialization before using it

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]

Date:	Mon, 9 Dec 2013 03:17:47 -0800
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Ethan Zhao <ethan.kernel@...il.com>
Cc:	konrad.wilk@...cle.com, raghavendra.kt@...ux.vnet.ibm.com,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] xen/debugfs: Check debugfs initialization before using it

On Mon, Dec 09, 2013 at 05:57:22PM +0800, Ethan Zhao wrote:
> On Mon, Dec 9, 2013 at 4:55 PM, Greg KH <gregkh@...uxfoundation.org> wrote:
> > On Mon, Dec 09, 2013 at 04:44:05PM +0800, Ethan Zhao wrote:
> >> On Mon, Dec 9, 2013 at 4:25 PM, Greg KH <gregkh@...uxfoundation.org> wrote:
> >> > On Mon, Dec 09, 2013 at 09:43:16AM +0800, Ethan Zhao wrote:
> >> >> On Sun, Dec 8, 2013 at 10:01 PM, Greg KH <gregkh@...uxfoundation.org> wrote:
> >> >> > On Sun, Dec 08, 2013 at 07:31:02PM +0800, ethan.zhao wrote:
> >> >> >> Should check debugfs initialization with debugfs_initialized() before using it,
> >> >> >> Because if it isn't initialized, the return value of fake debugfs_create_dir() etc
> >> >> >> functions would be ERR_PTR(-ENODEV), checking with NULL will not work.
> >> >> >
> >> >> > So?  It should "just work" without this check, right?  What happens if
> >> >> > your patch isn't applied and debugfs isn't enabled?
> >> >>
> >> >> If debugfs isn't  configured, debugfs_initialized() and other
> >> >> functions are defined as following,
> >> >>
> >> >> static inline bool debugfs_initialized(void)
> >> >> {
> >> >> return false;
> >> >> }
> >> >>
> >> >> static inline struct dentry *debugfs_create_file(const char *name, umode_t mode,
> >> >> struct dentry *parent, void *data,
> >> >> const struct file_operations *fops)
> >> >> {
> >> >> return ERR_PTR(-ENODEV);
> >> >> }
> >> >>
> >> >> static inline struct dentry *debugfs_create_dir(const char *name,
> >> >> struct dentry *parent)
> >> >> {
> >> >> return ERR_PTR(-ENODEV);
> >> >> }
> >> >>
> >> >> And the checking code in xen\debugfs.c xen_init_debugfs() will not
> >> >> work, the return value is not NULL.
> >> >> d_xen_debug = debugfs_create_dir("xen", NULL);
> >> >>
> >> >> if (!d_xen_debug)
> >> >> pr_warning("Could not create 'xen' debugfs directory\n");
> >> >
> >> > Which is just fine, what is wrong with this?
> >>
> >> If we no check with debugfs_initialized(),  the above code should be
> >> if  (!d_xen_debug || IS_ERR (d_xen_debug))
> >>     pr_warning("Could not create 'xen' debugfs directory\n");
> >
> > No, you don't want to print out that message if debugfs is not enabled.
> >
> > You really don't want to print anything out, as what can a user do about
> > this?
> >
> Yep, should output a warning about "Debugfs is not configured and enabled"

No, not at all, it's not an issue you need to warn about because it was
explicitly told to be that way by the person who built that kernel.

It's not an error, please don't treat it as such.

greg k-h
--
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/