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 <[email protected]> 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/[email protected]?part=35
