Messages in this thread | | | Subject | Re: [PATCH v2 6/9] debugfs: Implement debugfs_create_str() | From | Rasmus Villemoes <> | Date | Fri, 26 Mar 2021 15:58:37 +0100 |
| |
On 26/03/2021 15.22, Peter Zijlstra wrote: > On Fri, Mar 26, 2021 at 01:53:59PM +0100, Rasmus Villemoes wrote: >> On 26/03/2021 12.38, Peter Zijlstra wrote: > >>> + >>> +again: >>> + rcu_read_lock(); >>> + str = rcu_dereference(*(char **)file->private_data); >>> + len = strlen(str) + 1; >>> + >>> + if (!copy || copy_len < len) { >>> + rcu_read_unlock(); >>> + kfree(copy); >>> + copy = kmalloc(len + 1, GFP_KERNEL); >>> + if (!copy) { >>> + debugfs_file_put(dentry); >>> + return -ENOMEM; >>> + } >>> + copy_len = len; >>> + goto again; >>> + } >>> + >>> + strncpy(copy, str, len); >>> + copy[len] = '\n'; >>> + copy[len+1] = '\0'; >>> + rcu_read_unlock(); >> >> As noted (accidentally off-list), this is broken. I think you want this >> on top >> >> - len = strlen(str) + 1; >> + len = strlen(str); > > kmalloc(len + 2, ...);
No, because nul-terminating the stuff you pass to simple_read_from_buffer is pointless cargo-culting. Yeah, read_file_bool does it, but that's just bogus.
>> - strncpy(copy, str, len); >> + memcpy(copy, str, len); >> copy[len] = '\n'; >> - copy[len+1] = '\0'; > > I'll go with strscpy() I tihnk, something like: > > len = strscpy(copy, str, len); > if (len < 0) > return len;
To what end? The only way that could possibly return -EFOO is if the nul-terminator in str vanished between the strlen() and here, and in that case you have bigger problems.
> copy[len] = '\n'; > copy[len + 1] = '\0'; > >>> +EXPORT_SYMBOL_GPL(debugfs_read_file_str); >> >> Why? > > Copy-pasta from debugfs_*_bool(). This thing seems to export everything > and I figured I'd go along with that.
I thought the convention was not to export anything until the kernel itself had a (modular) user. But I can't find that stated under Documentation/ anywhere - but it does say "Every function that is exported to loadable modules using ``EXPORT_SYMBOL`` or ``EXPORT_SYMBOL_GPL`` should have a kernel-doc comment.". Anyway, the *_bool stuff doesn't seem to be something to be copy-pasted.
Rasmus
| |