Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] proc: proc_readdir_de() returns 1 at EOF, breaking old_readdir() --
commit 6510b1c4f87c45306294a13ba8447b50990db356 Author: Christian Brauner <[email protected]> proc: resume readdir after the last name it returned This commit records the name of the last emitted directory entry in a cursor stored in file->private_data so that subsequent readdir calls can resume after that entry by name rather than by numeric index. It also continues after an entry's name if the entry is removed while being emitted, and returns 1 when no entries remain at the start of proc_readdir_de(). > diff --git a/fs/proc/generic.c b/fs/proc/generic.c > index 9e59796349e15..588999058e66d 100644 > --- a/fs/proc/generic.c > +++ b/fs/proc/generic.c [ ... ] > @@ -290,23 +346,26 @@ struct dentry *proc_lookup(struct inode *dir, struct > dentry *dentry, > int proc_readdir_de(struct file *file, struct dir_context *ctx, > struct proc_dir_entry *de) > { > + struct proc_dir_cursor *cursor = file->private_data; > + struct proc_dir_entry *dir = de; > int i; > > if (!dir_emit_dots(file, ctx)) > return 0; > > - i = ctx->pos - 2; > read_lock(&proc_subdir_lock); > - de = pde_subdir_first(de); > - for (;;) { > - if (!de) { > - read_unlock(&proc_subdir_lock); > - return 0; > - } > - if (!i) > - break; > - de = pde_subdir_next(de); > - i--; > + if (cursor && cursor->pos == ctx->pos) { > + /* Resume after the last entry emitted, wherever it is now. */ > + de = pde_subdir_after(dir, cursor->name, cursor->len); > + } else { > + /* Moved by a seek, count from the start like always. */ > + de = pde_subdir_first(dir); > + for (i = ctx->pos - 2; de && i; i--) > + de = pde_subdir_next(de); > + } > + if (!de) { > + read_unlock(&proc_subdir_lock); > + return 1; > } [Severity: High] Does returning 1 here when !de cause old_readdir() and compat_old_readdir() to return 1 instead of 0 at end-of-directory on non-root /proc directories? While proc_root_readdir() checks for a positive return value from proc_readdir_de() to advance to the PID entries, proc_readdir() (for proc_dir_operations) and proc_tgid_net_readdir() (for proc_net_operations) return the result of proc_readdir_de() directly as their .iterate_shared return value: fs/proc/generic.c:proc_readdir() { ... return proc_readdir_de(file, ctx, PDE(inode)); } fs/proc/proc_net.c:proc_tgid_net_readdir() { ... ret = proc_readdir_de(file, ctx, net->proc_net); put_net(net); return ret; } Previously, proc_readdir_de() only returned 1 after completing the do-while loop (where dir_emit() had succeeded at least once and set buf.result = 1 in fillonedir()), and returned 0 when !de at the start of the call. In fs/readdir.c, old_readdir() and compat_old_readdir() do: error = iterate_dir(fd_file(f), &buf.ctx); if (buf.result) error = buf.result; return error; When old_readdir() or compat_old_readdir() is called at EOF (ctx->pos >= 2 and !de), dir_emit_dots() returns true without invoking ctx->actor, so buf.result remains 0 and iterate_dir() returns 1 from proc_readdir_de(). Wouldn't old_readdir() and compat_old_readdir() then return 1 to userspace without writing to the dirent buffer or advancing file->f_pos, causing callers to read uninitialized or stale dirent memory in an infinite loop? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=32
