Message-ID: <YCv+gpVGHTh9ZMNq@chrisdown.name>
Date:   Tue, 16 Feb 2021 17:18:58 +0000
From:   Chris Down <chris@...isdown.name>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        John Ogness <john.ogness@...utronix.de>,
        Johannes Weiner <hannes@...xchg.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kees Cook <keescook@...omium.org>, kernel-team@...com
Subject: Re: debugfs: was: Re: [PATCH v4] printk: Userspace format
 enumeration support

Petr Mladek writes:
>> +static size_t printk_fmt_size(const char *fmt)
>> +{
>> +	size_t sz = strlen(fmt) + 1;
>> +
>> +	/*
>> +	 * Some printk formats don't start with KERN_SOH + level. We will add
>> +	 * it later when rendering the output.
>> +	 */
>> +	if (unlikely(fmt[0] != KERN_SOH_ASCII))
>> +		sz += 2;
>
>This approach is hard to maintain. It might be pretty hard and error
>prone to count the size if we want to provide more information.
>
>There are many files in debugfs with not-well defined size.
>They are opened by seq_open_private(). It allows to add
>a line by line by an iterator.

Hmm, this is optional -- it was just to avoid seq_file having to realloc the 
buffer. I originally used an iterator and I'm happy to go back to it if it 
proves more convenient.

>We should revert the changes when the file could not get crated.
>It does not make sense to keep the structure when the file is not
>there.

See the reply from gregkh on v2, who was quite insistent that we should not 
check debugfs error codes. I'm happy to do either, but I can't please you both 
:-)

>I guess that remove_printk_fmt_sec() would even crash when
>ps->file was set to an error code.

debugfs checks if its input is an error, so it shouldn't, unless that's not 
what you're referring to?

>> +}
>> +
>> +#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);
>
>IMHO, we should remove the file before we remove the way how
>to read it. This should be done in the opposite order
>than in store_printk_fmt_sec().

There is a subtle issue with doing this as-is: debugfs_remove(ps->file) cannot 
be called under printk_fmts_mutex, because we may deadlock due to a pinned 
debugfs refcnt if debugfs_remove() and _show happen at the same time.

Imagine we go into remove_printk_fmt_sec and grab printk_fmts_lock. On another 
CPU, we call _show for the same file, which takes a reference in debugfs, but 
it will stall waiting for printk_fmts_lock. Now we go back into 
remove_printk_fmt_sec and can't make any forward progress, because 
debugfs_remove will stall until all reference holders have finished, and there 
is a deadlock.

That's the reason that debugfs_remove() must be called after we have already 
finished with the mutex and have the printk_fmt_sec, since we need to know that 
it's still valid, and we also need to not be under it at the time of removal.

One way to do what you're asking might be to have a flag in the printk_fmt_sec 
which indicates that we are freeing something, and then take and release the 
lock twice in remove_printk_fmt_sec. Personally, I feel indifferent to either 
the current solution or something like that, but if you have a preference for 
adding a flag or another similar solution, that's fine with me. Just let me 
know. :-)

Powered by blists - more mailing lists