On Mon 02-11-26 15:21:33, Christian Brauner wrote:
> proc_register() WARNs when the directory already has an entry of the
> same name and fails the registration. Callers that pick their names
> themselves should never get there but some take names from userspace.
> pktgen creates a file for each device a writer of "add_device" adds,
> named after the device. A device can have any name, including "pgctrl"
> or "kpktgend_0", the names of pktgen's own files:
>
> ip link set lo name pgctrl
> ip link set pgctrl up
> echo "add_device pgctrl" > /proc/net/pktgen/kpktgend_0
>
> proc_dir_entry 'pktgen/pgctrl' already registered
> WARNING: fs/proc/generic.c:404 at proc_register+0x555/0x8b0, CPU#0: bash/241
> Call Trace:
> proc_create_data+0x9f/0xf0
> pktgen_thread_write+0xd9b/0x1780
> proc_reg_write+0x1c2/0x3c0
> vfs_write+0x1cd/0xfa0
>
> The files of a network namespace belong to the root of its user
> namespace and none of this requires a capability. So once pktgen is
> loaded an unprivileged user can taint the kernel, or crash it with
> panic_on_warn, from namespaces of its own. pktgen itself copes with
> the failed registration just fine.
>
> So go back to what proc_register() did before commit 6c2f91e077f1
> ("proc: use WARN() rather than printk+backtrace") and what
> sysfs_warn_dup() does for sysfs: report the name with pr_warn() and
> dump the stack. While at it, do that after dropping proc_subdir_lock
> instead of printing a backtrace while holding the lock that every
> lookup and readdir in procfs needs.
>
> Reported-by: Chris Mason <[email protected]>
> Fixes: 6c2f91e077f1 ("proc: use WARN() rather than printk+backtrace")
> Cc: [email protected]
> Signed-off-by: Christian Brauner (Amutable) <[email protected]>
Makes sense. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/proc/generic.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index c560c96a64eb..ef804cd3ec45 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -401,9 +401,10 @@ struct proc_dir_entry *proc_register(struct
> proc_dir_entry *dir,
> write_lock(&proc_subdir_lock);
> dp->parent = dir;
> if (pde_subdir_insert(dir, dp) == false) {
> - WARN(1, "proc_dir_entry '%s/%s' already registered\n",
> - dir->name, dp->name);
> write_unlock(&proc_subdir_lock);
> + pr_warn("proc_dir_entry '%s/%s' already registered\n",
> + dir->name, dp->name);
> + dump_stack();
> goto out_free_inum;
> }
> write_unlock(&proc_subdir_lock);
>
> --
> 2.53.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
