This function is used by ptrace and proc files like /proc/pid/cmdline and
/proc/pid/environ. Return 0 (bytes read) if current task is killed.
Mmap_sem could be locked for a long time or forever if something wrong.
Signed-off-by: Konstantin Khlebnikov <[email protected]>
---
mm/memory.c | 4 +++-
mm/nommu.c | 3 ++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index 96f1d473c89a..2e6846d09023 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4348,7 +4348,9 @@ int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
void *old_buf = buf;
int write = gup_flags & FOLL_WRITE;
- down_read(&mm->mmap_sem);
+ if (down_read_killable(&mm->mmap_sem))
+ return 0;
+
/* ignore errors, just check how much was successfully transferred */
while (len) {
int bytes, ret, offset;
diff --git a/mm/nommu.c b/mm/nommu.c
index b492fd1fcf9f..cad8fb34088f 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1791,7 +1791,8 @@ int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
struct vm_area_struct *vma;
int write = gup_flags & FOLL_WRITE;
- down_read(&mm->mmap_sem);
+ if (down_read_killable(&mm->mmap_sem))
+ return 0;
/* the access must start within one of the target process's mappings */
vma = find_vma(mm, addr);
Hi,
making this holder of mmap_sem killable was for the reasons of /proc/...
diagnostics was an idea I was pondeering too. However, I think the
approach of pretending we read 0 bytes is not correct. The API would IMO
need to be extended to allow pass a result such as EINTR to the end
caller.
Why do you think it's safe to return just 0?
Michal
On 15.05.2019 11:38, Michal Koutn? wrote:
> Hi,
> making this holder of mmap_sem killable was for the reasons of /proc/...
> diagnostics was an idea I was pondeering too. However, I think the
> approach of pretending we read 0 bytes is not correct. The API would IMO
> need to be extended to allow pass a result such as EINTR to the end
> caller.
> Why do you think it's safe to return just 0?
This function ignores any error like reading from unmapped area and
returns only size of successful transfer. It never returned any error codes.
>
> Michal
>
On Wed, May 15, 2019 at 10:38:26AM +0200, Michal Koutn? wrote:
> Hi,
> making this holder of mmap_sem killable was for the reasons of /proc/...
> diagnostics was an idea I was pondeering too. However, I think the
> approach of pretending we read 0 bytes is not correct. The API would IMO
> need to be extended to allow pass a result such as EINTR to the end
> caller.
> Why do you think it's safe to return just 0?
_killable_, not _interruptible_.
The return value will never be seen by userspace because it's dead.
> @@ -4348,7 +4348,9 @@ int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> void *old_buf = buf;
> int write = gup_flags & FOLL_WRITE;
>
> - down_read(&mm->mmap_sem);
> + if (down_read_killable(&mm->mmap_sem))
> + return 0;
> +
I too think that "return 0" looks a bit strange even if correct, to me
"return -EINTR" would look better.
But I won't insist, this is cosmetic.
Acked-by: Oleg Nesterov <[email protected]>
On Wed, May 15, 2019 at 11:48:32AM +0300, Konstantin Khlebnikov <[email protected]> wrote:
> This function ignores any error like reading from unmapped area and
> returns only size of successful transfer. It never returned any error codes.
This is a point I missed. Hence no need to adjust consumers of
__access_remote_vm() (they won't actually handle -EINTR correctly w/out
further changes). This beats my original idea with simplicity.
Reviewed-by: Michal Koutn? <[email protected]>
Michal
On Wed 15-05-19 11:21:18, Konstantin Khlebnikov wrote:
> This function is used by ptrace and proc files like /proc/pid/cmdline and
> /proc/pid/environ. Return 0 (bytes read) if current task is killed.
Please add an explanation about why this is OK (as explained in the
follow up email).
> Mmap_sem could be locked for a long time or forever if something wrong.
>
> Signed-off-by: Konstantin Khlebnikov <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/memory.c | 4 +++-
> mm/nommu.c | 3 ++-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 96f1d473c89a..2e6846d09023 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4348,7 +4348,9 @@ int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> void *old_buf = buf;
> int write = gup_flags & FOLL_WRITE;
>
> - down_read(&mm->mmap_sem);
> + if (down_read_killable(&mm->mmap_sem))
> + return 0;
> +
> /* ignore errors, just check how much was successfully transferred */
> while (len) {
> int bytes, ret, offset;
> diff --git a/mm/nommu.c b/mm/nommu.c
> index b492fd1fcf9f..cad8fb34088f 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1791,7 +1791,8 @@ int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> struct vm_area_struct *vma;
> int write = gup_flags & FOLL_WRITE;
>
> - down_read(&mm->mmap_sem);
> + if (down_read_killable(&mm->mmap_sem))
> + return 0;
>
> /* the access must start within one of the target process's mappings */
> vma = find_vma(mm, addr);
--
Michal Hocko
SUSE Labs