Hi, all On Mon, 24 Mar 2025 18:48:35 +0100, Mateusz Guzik wrote: > On Mon, Mar 24, 2025 at 09:23:53AM -0700, Penglei Jiang wrote: > > The following functions call proc_mem_open but do not handle the case > > where it returns NULL: > > > > __mem_open in fs/proc/base.c > > proc_maps_open in fs/proc/task_mmu.c > > smaps_rollup_open in fs/proc/task_mmu.c > > pagemap_open in fs/proc/task_mmu.c > > maps_open in fs/proc/task_nommu.c > > > > The following reported bugs may be related to this issue: > > > > https://lore.kernel.org/all/000000000000f52642060d4e3750@xxxxxxxxxx > > https://lore.kernel.org/all/0000000000001bc4a00612d9a7f4@xxxxxxxxxx > > > > Fix: > > > > Modify proc_mem_open to return an error code in case of errors, instead > > of returning NULL. > > > > The rw routines associated with these consumers explictly NULL check > mm, which becomes redundant with the patch. > > While I find it fishy that returning NULL was ever a thing to begin > with, it is unclear to me if it can be easily changed now from > userspace-visible behavior standpoint. > > I think the best way forward for the time being is to add the missing > NULL checks instead. > > > Signed-off-by: Penglei Jiang <superman.xpt@xxxxxxxxx> > > --- > > fs/proc/base.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index cd89e956c322..b5e7317cf0dc 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -840,7 +840,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) > > put_task_struct(task); > > > > if (IS_ERR(mm)) > > -return mm == ERR_PTR(-ESRCH) ? NULL : mm; > > +return mm; > > > > /* ensure this mm_struct can't be freed */ > > mmgrab(mm); > > -- > > 2.17.1 > > Mateusz Guzik provides valuable suggestions. Complete the missing NULL checks. Signed-off-by: Penglei Jiang <superman.xpt@xxxxxxxxx> --- fs/proc/base.c | 4 ++-- fs/proc/task_mmu.c | 12 ++++++------ fs/proc/task_nommu.c | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index cd89e956c322..d898cec3ee71 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -854,8 +854,8 @@ static int __mem_open(struct inode *inode, struct file *file, unsigned int mode) { struct mm_struct *mm = proc_mem_open(inode, mode); - if (IS_ERR(mm)) - return PTR_ERR(mm); + if (IS_ERR_OR_NULL(mm)) + return mm ? PTR_ERR(mm) : -ESRCH; file->private_data = mm; return 0; diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index f02cd362309a..14d1d8d3e432 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -212,8 +212,8 @@ static int proc_maps_open(struct inode *inode, struct file *file, priv->inode = inode; priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); - if (IS_ERR(priv->mm)) { - int err = PTR_ERR(priv->mm); + if (IS_ERR_OR_NULL(priv->mm)) { + int err = priv->mm ? PTR_ERR(priv->mm) : -ESRCH; seq_release_private(inode, file); return err; @@ -1312,8 +1312,8 @@ static int smaps_rollup_open(struct inode *inode, struct file *file) priv->inode = inode; priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); - if (IS_ERR(priv->mm)) { - ret = PTR_ERR(priv->mm); + if (IS_ERR_OR_NULL(priv->mm)) { + ret = priv->mm ? PTR_ERR(priv->mm) : -ESRCH; single_release(inode, file); goto out_free; @@ -2045,8 +2045,8 @@ static int pagemap_open(struct inode *inode, struct file *file) struct mm_struct *mm; mm = proc_mem_open(inode, PTRACE_MODE_READ); - if (IS_ERR(mm)) - return PTR_ERR(mm); + if (IS_ERR_OR_NULL(mm)) + return mm ? PTR_ERR(mm) : -ESRCH; file->private_data = mm; return 0; } diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c index bce674533000..59bfd61d653a 100644 --- a/fs/proc/task_nommu.c +++ b/fs/proc/task_nommu.c @@ -260,8 +260,8 @@ static int maps_open(struct inode *inode, struct file *file, priv->inode = inode; priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); - if (IS_ERR(priv->mm)) { - int err = PTR_ERR(priv->mm); + if (IS_ERR_OR_NULL(priv->mm)) { + int err = priv->mm ? PTR_ERR(priv->mm) : -ESRCH; seq_release_private(inode, file); return err; -- 2.17.1
