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. :-)
