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
