Re: [PATCH v2] printk: Userspace format enumeration support

[Greg Kroah-Hartman]

On Tue, Feb 09, 2021 at 04:37:19AM +0000, Chris Down wrote:
> +
> +	file = debugfs_create_file(ps_get_module_name(ps), 0444, dfs_formats,
> +				   mod, &dfs_formats_fops);
> +
> +	if (IS_ERR_OR_NULL(file))

How can file ever be NULL?

And if it is an error, what is the problem here?  You can always feed
the output of a debugfs_* call back into debugfs, and you never need to
check the return values.

> +		ps->file = NULL;
> +	else
> +		ps->file = file;
> +}
> +
> +#ifdef CONFIG_MODULES
> +static void remove_printk_fmt_sec(struct module *mod)
> +{
> +	struct printk_fmt_sec *ps = NULL;
> +
> +	if (WARN_ON_ONCE(!mod))
> +		return;
> +
> +	mutex_lock(&printk_fmts_mutex);
> +
> +	ps = find_printk_fmt_sec(mod);
> +	if (!ps) {
> +		mutex_unlock(&printk_fmts_mutex);
> +		return;
> +	}
> +
> +	hash_del(&ps->hnode);
> +
> +	mutex_unlock(&printk_fmts_mutex);
> +
> +	debugfs_remove(ps->file);
> +	kfree(ps);
> +}
> +
> +static int module_printk_fmts_notify(struct notifier_block *self,
> +				     unsigned long val, void *data)
> +{
> +	struct module *mod = data;
> +
> +	if (mod->printk_fmts_sec_size) {
> +		switch (val) {
> +		case MODULE_STATE_COMING:
> +			store_printk_fmt_sec(mod, mod->printk_fmts_start,
> +					     mod->printk_fmts_start +
> +						     mod->printk_fmts_sec_size);
> +			break;
> +
> +		case MODULE_STATE_GOING:
> +			remove_printk_fmt_sec(mod);
> +			break;
> +		}
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static const char *ps_get_module_name(const struct printk_fmt_sec *ps)
> +{
> +	return ps->module ? ps->module->name : "vmlinux";
> +}
> +
> +static struct notifier_block module_printk_fmts_nb = {
> +	.notifier_call = module_printk_fmts_notify,
> +};
> +
> +static int __init module_printk_fmts_init(void)
> +{
> +	return register_module_notifier(&module_printk_fmts_nb);
> +}
> +
> +core_initcall(module_printk_fmts_init);
> +
> +#else /* !CONFIG_MODULES */
> +static const char *ps_get_module_name(const struct printk_fmt_sec *ps)
> +{
> +	return "vmlinux";
> +}
> +#endif /* CONFIG_MODULES */
> +
> +static int debugfs_pf_show(struct seq_file *s, void *v)
> +{
> +	struct module *mod = s->file->f_inode->i_private;
> +	struct printk_fmt_sec *ps = NULL;
> +	const char **fptr = NULL;
> +	int ret = 0;
> +
> +	mutex_lock(&printk_fmts_mutex);
> +
> +	/*
> +	 * The entry might have been invalidated in the hlist between _open and
> +	 * _show, which is we need to eyeball the entries under
> +	 * printk_fmts_mutex again.
> +	 */
> +	ps = find_printk_fmt_sec(mod);
> +	if (unlikely(!ps)) {
> +		ret = -ENOENT;
> +		goto out_unlock;
> +	}
> +
> +	for (fptr = ps->start; fptr < ps->end; fptr++) {
> +		/* For callsites without facility/level preamble. */
> +		if (unlikely(*fptr[0] != KERN_SOH_ASCII))
> +			seq_printf(s, "%c%d", KERN_SOH_ASCII,
> +				   MESSAGE_LOGLEVEL_DEFAULT);
> +		seq_printf(s, "%s%c", *fptr, '\0');
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&printk_fmts_mutex);
> +	return ret;
> +}
> +
> +static int debugfs_pf_open(struct inode *inode, struct file *file)
> +{
> +	struct module *mod = inode->i_private;
> +	struct printk_fmt_sec *ps = NULL;
> +	int ret;
> +
> +	/*
> +	 * We can't pass around the printk_fmt_sec because it might be freed
> +	 * before we enter the mutex. Do the hash table lookup each time to
> +	 * check.
> +	 */
> +	mutex_lock(&printk_fmts_mutex);
> +
> +	ps = find_printk_fmt_sec(mod);
> +	if (unlikely(!ps)) {
> +		ret = -ENOENT;
> +		goto out_unlock;
> +	}
> +
> +	ret = single_open_size(file, debugfs_pf_show, NULL, ps->output_size);
> +
> +out_unlock:
> +	mutex_unlock(&printk_fmts_mutex);
> +
> +	return ret;
> +}
> +
> +static int __init init_printk_fmts(void)
> +{
> +	struct dentry *dfs_root = debugfs_create_dir("printk", NULL);
> +	struct dentry *tmp = NULL;
> +
> +	if (IS_ERR_OR_NULL(dfs_root))

Again, how can dfs_root be NULL?

And why care about any error?  No kernel code should ever work
differently if debugfs is acting up or not.

> +		return -ENOMEM;
> +
> +	tmp = debugfs_create_dir("formats", dfs_root);
> +
> +	if (IS_ERR_OR_NULL(tmp))

Again, NULL can never happen.  Where did you copy this logic from?  I
need to go fix that...

thanks,

greg k-h