Re: [PATCH RFC 35/38] proc: sysctl: don't hold the tables while readdir copies to userspace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] proc: sysctl: premature EOF and lost errors in proc_sys_readdir()
- [Low] proc: sysctl: unused mode field in struct sysctl_dirent
--

commit 35a95ac245f1b380541472c3423535528d1bbf7d
Author: Christian Brauner <brauner@kernel.org>

proc: sysctl: don't hold the tables while readdir copies to userspace

This commit changes proc_sys_readdir() to avoid taking sysctl tables into use
during directory iteration, pinning only the header memory of the current
entry while dropping sysctl_lock before instantiating dentries and copying
entries to userspace.

> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index a1242247f4929..0b7a1e487ed6c 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c

[ ... ]

> @@ -367,86 +369,97 @@ static const struct ctl_table *lookup_entry(struct ctl_table_header **phead,
>  	return entry;
>  }
>  
> -static struct ctl_node *first_usable_entry(struct rb_node *node)
> -{
> -	struct ctl_node *ctl_node;
> +/*
> + * What readdir takes along for an entry between the trips through
> + * sysctl_lock: the header pinned, which keeps its memory and nothing else,
> + * the entry, its name and mode. For a link the header and entry are the
> + * ones it stands for in the caller's set.
> + */
> +struct sysctl_dirent {
> +	struct ctl_table_header *head;
> +	const struct ctl_table *table;
> +	struct ctl_node *node;		/* the entry's node in the directory, NULL for a link */
> +	ino_t ino;
> +	unsigned int type;
> +	umode_t mode;
> +	bool skip;
> +	int len;
> +	char name[NAME_MAX + 1];
> +};

[ ... ]

> @@ -454,15 +467,56 @@ static int entry_after(struct ctl_dir *dir, const char *name, int namelen,
>  			node = node->rb_right;
>  		}
>  	}
> -	ctl_node = first_usable_entry(after);
> -	spin_unlock(&sysctl_lock);
> -	if (ctl_node) {
> -		head = ctl_node->header;
> -		entry = &head->ctl_table[ctl_node - head->node];
> +	ctl_node = first_live_entry(after);
> +	if (!ctl_node)
> +		goto out;
> +	head = ctl_node->header;
> +	entry = &head->ctl_table[ctl_node - head->node];
> +
> +	d->len = strlen(entry->procname);
> +	if (d->len > NAME_MAX) {
> +		ret = -ENAMETOOLONG;
> +		goto out;
>  	}
> -	*phead = head;
> -	*pentry = entry;
> -	return 0;
> +	memcpy(d->name, entry->procname, d->len);
> +	d->mode = entry->mode;

[Severity: Low]
This isn't a bug, but is d->mode in struct sysctl_dirent needed?

sysctl_dir_next() stores entry->mode into d->mode here, but neither
proc_sys_fill_dirent() nor proc_sys_readdir() ever reads d->mode, as
proc_sys_make_inode() reads table->mode under sysctl_lock and
proc_sys_fill_dirent() derives d->type from inode->i_mode.

[ ... ]

> @@ -875,21 +891,30 @@ static int proc_sys_readdir(struct file *file, struct dir_context *ctx)
>  	 * seeking to an arbitrary position always did.
>  	 */
>  	if (cursor && cursor->pos == ctx->pos) {
> -		entry_after(ctl_dir, cursor->name, cursor->len, &h, &entry);
> -		pos = ctx->pos;
> +		err = sysctl_dir_next(ctl_dir, &d, cursor->name, cursor->len);
>  	} else {
> -		first_entry(ctl_dir, &h, &entry);
> -		pos = 2;
> -	}
> +		loff_t pos = 2;
>  
> -	for (; h; next_entry(&h, &entry)) {
> -		if (!scan(h, entry, &pos, file, ctx)) {
> -			sysctl_head_finish(h);
> -			break;
> +		err = sysctl_dir_next(ctl_dir, &d, NULL, 0);
> +		while (err > 0 && pos++ < ctx->pos)
> +			err = sysctl_dir_next(ctl_dir, &d, d.name, d.len);
> +	}
> +	if (err < 0)
> +		return err;
> +
> +	while (err > 0) {
> +		if (!d.skip) {
> +			err = proc_sys_fill_dirent(file, &d);
> +			if (err)
> +				break;
> +			if (!dir_emit(ctx, d.name, d.len, d.ino, d.type))
> +				break;
>  		}
> +		ctx->pos++;
> +		sysctl_cursor_set(file, d.name, d.len, ctx->pos);
> +		err = sysctl_dir_next(ctl_dir, &d, d.name, d.len);
>  	}
> -out:
> -	sysctl_head_finish(head);
> +	sysctl_dir_done(&d);
>  	return 0;
>  }

[Severity: Medium]
Can a concurrent unregister_sysctl_table() or error during the loop in
proc_sys_readdir() cause premature EOF or drop error codes?

Because sysctl_dir_next() only pins d->head->count via sysctl_pin() and does
not increment d->head->used, a concurrent unregister_sysctl_table() does not
wait in start_unregistering() and can set d->head->unregistering as soon as
sysctl_dir_next() releases sysctl_lock.

When proc_sys_readdir() then calls proc_sys_fill_dirent() ->
proc_sys_make_inode(), proc_sys_make_inode() checks head->unregistering under
sysctl_lock:

    spin_lock(&sysctl_lock);
    if (unlikely(head->unregistering)) {
        spin_unlock(&sysctl_lock);
        iput(inode);
        return ERR_PTR(-ENOENT);
    }

proc_sys_fill_dirent() propagates this -ENOENT back to proc_sys_readdir(),
which breaks out of the while (err > 0) loop and returns 0. If no entries
have been emitted yet in the current getdents64() call (for example on a
subsequent getdents64() call where ctx->pos >= 2), returning 0 causes
getdents64() to report EOF to userspace and omit all remaining live entries
in the directory.

Similarly, if proc_sys_fill_dirent() fails with -ENOMEM, or if the in-loop
sysctl_dir_next() call returns a negative error such as -ENOENT (when
ctl_dir itself is unregistered after a skipped link) or -ENAMETOOLONG,
proc_sys_readdir() still returns 0 instead of err.

Should proc_sys_readdir() skip entries when proc_sys_fill_dirent() returns
-ENOENT and continue to the next entry, and return a negative err at the end
if failed before emitting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-work-procfs-bugfixes-v1-0-59422ed3e465@kernel.org?part=35