| Message ID | 20240924201023.193135-1-lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: refactor mm_access() to not return NULL | expand |
On Tue, Sep 24, 2024 at 09:10:23PM +0100, Lorenzo Stoakes wrote: > + if (IS_ERR(mm)) > + return PTR_ERR(mm) == -ESRCH ? NULL : mm; Pet peeve: what's wrong with mm == ERR_PTR(-ESRCH)?
On Tue, Sep 24, 2024 at 09:13:52PM GMT, Al Viro wrote: > On Tue, Sep 24, 2024 at 09:10:23PM +0100, Lorenzo Stoakes wrote: > > + if (IS_ERR(mm)) > > + return PTR_ERR(mm) == -ESRCH ? NULL : mm; > > Pet peeve: what's wrong with mm == ERR_PTR(-ESRCH)? Ha, yeah I'm easy either way, though your version is less ugly than mine (and places the macro/constants together which is clearer) so let's go with that... Andrew - fix-patch attached, please apply. Thanks! ----8<---- From cb132b841e8cee09da5428ea625b50df9222a74e Mon Sep 17 00:00:00 2001 From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Date: Wed, 25 Sep 2024 08:48:27 +0100 Subject: [PATCH] procfs: prefer neater pointer error comparison We can compare a pointer to a known error code via PTR_ERR(ptr) == -EINVAL or via ptr == ERR_PTR(-EINVAL) - the latter is neater and collects the macro and constant in one, so refactor to use this form in proc_mem_open(). Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> --- 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 fe31decc12b5..94112df5f2a2 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -841,7 +841,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) put_task_struct(task); if (IS_ERR(mm)) - return PTR_ERR(mm) == -ESRCH ? NULL : mm; + return mm == ERR_PTR(-ESRCH) ? NULL : mm; /* ensure this mm_struct can't be freed */ mmgrab(mm); -- 2.46.0
diff --git a/fs/proc/base.c b/fs/proc/base.c index b31283d81c52..fe31decc12b5 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -832,19 +832,21 @@ static const struct file_operations proc_single_file_operations = { struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) { struct task_struct *task = get_proc_task(inode); - struct mm_struct *mm = ERR_PTR(-ESRCH); + struct mm_struct *mm; - if (task) { - mm = mm_access(task, mode | PTRACE_MODE_FSCREDS); - put_task_struct(task); + if (!task) + return ERR_PTR(-ESRCH); - if (!IS_ERR_OR_NULL(mm)) { - /* ensure this mm_struct can't be freed */ - mmgrab(mm); - /* but do not pin its memory */ - mmput(mm); - } - } + mm = mm_access(task, mode | PTRACE_MODE_FSCREDS); + put_task_struct(task); + + if (IS_ERR(mm)) + return PTR_ERR(mm) == -ESRCH ? NULL : mm; + + /* ensure this mm_struct can't be freed */ + mmgrab(mm); + /* but do not pin its memory */ + mmput(mm); return mm; } @@ -2208,7 +2210,7 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags) goto out_notask; mm = mm_access(task, PTRACE_MODE_READ_FSCREDS); - if (IS_ERR_OR_NULL(mm)) + if (IS_ERR(mm)) goto out; if (!dname_to_vma_addr(dentry, &vm_start, &vm_end)) { diff --git a/kernel/fork.c b/kernel/fork.c index cbdaca45d0c1..841f60630c58 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1545,8 +1545,9 @@ struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) return ERR_PTR(err); mm = get_task_mm(task); - if (mm && mm != current->mm && - !ptrace_may_access(task, mode)) { + if (!mm) { + mm = ERR_PTR(-ESRCH); + } else if (mm != current->mm && !ptrace_may_access(task, mode)) { mmput(mm); mm = ERR_PTR(-EACCES); } diff --git a/mm/madvise.c b/mm/madvise.c index ff139e57cca2..50d223ab3894 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -1511,8 +1511,8 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, /* Require PTRACE_MODE_READ to avoid leaking ASLR metadata. */ mm = mm_access(task, PTRACE_MODE_READ_FSCREDS); - if (IS_ERR_OR_NULL(mm)) { - ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH; + if (IS_ERR(mm)) { + ret = PTR_ERR(mm); goto release_task; } diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c index b308e96cd05a..656d3e88755b 100644 --- a/mm/process_vm_access.c +++ b/mm/process_vm_access.c @@ -201,8 +201,8 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter, } mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS); - if (!mm || IS_ERR(mm)) { - rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH; + if (IS_ERR(mm)) { + rc = PTR_ERR(mm); /* * Explicitly map EACCES to EPERM as EPERM is a more * appropriate error code for process_vw_readv/writev
The mm_access() function can return NULL if the mm is not found, but this is handled the same as an error in all callers, with some translating this into an -ESRCH error. Only proc_mem_open() returns NULL if no mm is found, however in this case it is clearer and makes more sense to explicitly handle the error. Additionally we take the opportunity to refactor the function to eliminate unnecessary nesting. Simplify things by simply returning -ESRCH if no mm is found - this both eliminates confusing use of the IS_ERR_OR_NULL() macro, and simplifies callers which would return -ESRCH by returning this error directly. Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> --- fs/proc/base.c | 26 ++++++++++++++------------ kernel/fork.c | 5 +++-- mm/madvise.c | 4 ++-- mm/process_vm_access.c | 4 ++-- 4 files changed, 21 insertions(+), 18 deletions(-) -- 2.46.0