On Tue, 18 Aug 2026 16:17:20 +0100 Bradley Morgan <[email protected]> wrote:
> On 18 August 2026 03:01:18 BST, [email protected] wrote: > >Thank you for your contribution! Sashiko AI review found 1 potential > >issue(s) to consider: > >- [Low] The 'enabled' debugfs file is created using `debugfs_create_file()` > >instead of `debugfs_create_file_unsafe()` with a `DEFINE_DEBUGFS_ATTRIBUTE` > >struct, leading to redundant proxying and overhead. > > [email protected] > > I hate sashiko so damn much, best to wait for review! I found sashiko to be extremely useful. > >[ ... ] > > > >> -static const struct file_operations fops_kp = { > >> - .read = read_enabled_file_bool, > >> - .write = write_enabled_file_bool, > >> - .llseek = default_llseek, > >> -}; > >> +DEFINE_DEBUGFS_ATTRIBUTE(fops_kp, kprobes_enabled_get, > >> + kprobes_enabled_set, "%llu\n"); > >> > >> static int __init debugfs_kprobe_init(void) > >> { > > > >[Severity: Low] It is even saying this is of "low priority". That means it's more of an "FYI". > >Will this lead to double-proxying for the debugfs file? > > > >Since DEFINE_DEBUGFS_ATTRIBUTE natively provides debugfs_attr_read and > >debugfs_attr_write operations that already include debugfs_file_get() and > >debugfs_file_put() protection, passing fops_kp to debugfs_create_file() > >in debugfs_kprobe_init() will cause the debugfs core to wrap it again: > > > >kernel/kprobes.c:debugfs_kprobe_init() { > > ... > > debugfs_create_file("enabled", 0600, dir, NULL, &fops_kp); > > ... > >} > > > >Could this use debugfs_create_file_unsafe() instead? As this isn't a critical path, I don't think we really care here if it's wrapped or not. But it's nice to know that it is. Anyway, I'll let others review this, but it looks fine to me. -- Steve
