All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps
@ 2019-05-15  8:41 Konstantin Khlebnikov
  2019-05-15  8:41 ` [PATCH 2/5] proc: use down_read_killable for /proc/pid/smaps_rollup Konstantin Khlebnikov
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Konstantin Khlebnikov @ 2019-05-15  8:41 UTC (permalink / raw)
  To: linux-mm, Andrew Morton, linux-kernel
  Cc: Cyrill Gorcunov, Kirill Tkhai, Al Viro

Do not stuck forever if something wrong.
This function also used for /proc/pid/smaps.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/proc/task_mmu.c   |    6 +++++-
 fs/proc/task_nommu.c |    6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 01d4eb0e6bd1..2bf210229daf 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -166,7 +166,11 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
 	if (!mm || !mmget_not_zero(mm))
 		return NULL;
 
-	down_read(&mm->mmap_sem);
+	if (down_read_killable(&mm->mmap_sem)) {
+		mmput(mm);
+		return ERR_PTR(-EINTR);
+	}
+
 	hold_task_mempolicy(priv);
 	priv->tail_vma = get_gate_vma(mm);
 
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 36bf0f2e102e..7907e6419e57 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -211,7 +211,11 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 	if (!mm || !mmget_not_zero(mm))
 		return NULL;
 
-	down_read(&mm->mmap_sem);
+	if (down_read_killable(&mm->mmap_sem)) {
+		mmput(mm);
+		return ERR_PTR(-EINTR);
+	}
+
 	/* start from the Nth VMA */
 	for (p = rb_first(&mm->mm_rb); p; p = rb_next(p))
 		if (n-- == 0)


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/5] proc: use down_read_killable for /proc/pid/smaps_rollup
  2019-05-15  8:41 [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Konstantin Khlebnikov
@ 2019-05-15  8:41 ` Konstantin Khlebnikov
  2019-05-17 12:45   ` Michal Hocko
  2019-05-15  8:41 ` [PATCH 3/5] proc: use down_read_killable for /proc/pid/pagemap Konstantin Khlebnikov
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Konstantin Khlebnikov @ 2019-05-15  8:41 UTC (permalink / raw)
  To: linux-mm, Andrew Morton, linux-kernel
  Cc: Cyrill Gorcunov, Kirill Tkhai, Al Viro

Ditto.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/proc/task_mmu.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 2bf210229daf..781879a91e3b 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -832,7 +832,10 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
 
 	memset(&mss, 0, sizeof(mss));
 
-	down_read(&mm->mmap_sem);
+	ret = down_read_killable(&mm->mmap_sem);
+	if (ret)
+		goto out_put_mm;
+
 	hold_task_mempolicy(priv);
 
 	for (vma = priv->mm->mmap; vma; vma = vma->vm_next) {
@@ -849,8 +852,9 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
 
 	release_task_mempolicy(priv);
 	up_read(&mm->mmap_sem);
-	mmput(mm);
 
+out_put_mm:
+	mmput(mm);
 out_put_task:
 	put_task_struct(priv->task);
 	priv->task = NULL;


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/5] proc: use down_read_killable for /proc/pid/pagemap
  2019-05-15  8:41 [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Konstantin Khlebnikov
  2019-05-15  8:41 ` [PATCH 2/5] proc: use down_read_killable for /proc/pid/smaps_rollup Konstantin Khlebnikov
@ 2019-05-15  8:41 ` Konstantin Khlebnikov
  2019-05-17 12:46   ` Michal Hocko
  2019-05-15  8:41 ` [PATCH 4/5] proc: use down_read_killable for /proc/pid/clear_refs Konstantin Khlebnikov
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Konstantin Khlebnikov @ 2019-05-15  8:41 UTC (permalink / raw)
  To: linux-mm, Andrew Morton, linux-kernel
  Cc: Cyrill Gorcunov, Kirill Tkhai, Al Viro

Ditto.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/proc/task_mmu.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 781879a91e3b..78bed6adc62d 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1547,7 +1547,9 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
 		/* overflow ? */
 		if (end < start_vaddr || end > end_vaddr)
 			end = end_vaddr;
-		down_read(&mm->mmap_sem);
+		ret = down_read_killable(&mm->mmap_sem);
+		if (ret)
+			goto out_free;
 		ret = walk_page_range(start_vaddr, end, &pagemap_walk);
 		up_read(&mm->mmap_sem);
 		start_vaddr = end;


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/5] proc: use down_read_killable for /proc/pid/clear_refs
  2019-05-15  8:41 [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Konstantin Khlebnikov
  2019-05-15  8:41 ` [PATCH 2/5] proc: use down_read_killable for /proc/pid/smaps_rollup Konstantin Khlebnikov
  2019-05-15  8:41 ` [PATCH 3/5] proc: use down_read_killable for /proc/pid/pagemap Konstantin Khlebnikov
@ 2019-05-15  8:41 ` Konstantin Khlebnikov
  2019-05-17 12:47   ` Michal Hocko
  2019-05-15  8:41 ` [PATCH 5/5] proc: use down_read_killable for /proc/pid/map_files Konstantin Khlebnikov
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Konstantin Khlebnikov @ 2019-05-15  8:41 UTC (permalink / raw)
  To: linux-mm, Andrew Morton, linux-kernel
  Cc: Cyrill Gorcunov, Kirill Tkhai, Al Viro

Replace the only unkillable mmap_sem lock in clear_refs_write.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/proc/task_mmu.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 78bed6adc62d..7f84d1477b5b 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1140,7 +1140,10 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 			goto out_mm;
 		}
 
-		down_read(&mm->mmap_sem);
+		if (down_read_killable(&mm->mmap_sem)) {
+			count = -EINTR;
+			goto out_mm;
+		}
 		tlb_gather_mmu(&tlb, mm, 0, -1);
 		if (type == CLEAR_REFS_SOFT_DIRTY) {
 			for (vma = mm->mmap; vma; vma = vma->vm_next) {


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/5] proc: use down_read_killable for /proc/pid/map_files
  2019-05-15  8:41 [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Konstantin Khlebnikov
                   ` (2 preceding siblings ...)
  2019-05-15  8:41 ` [PATCH 4/5] proc: use down_read_killable for /proc/pid/clear_refs Konstantin Khlebnikov
@ 2019-05-15  8:41 ` Konstantin Khlebnikov
  2019-05-15 17:00   ` Roman Gushchin
  2019-05-15  9:05 ` [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Cyrill Gorcunov
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Konstantin Khlebnikov @ 2019-05-15  8:41 UTC (permalink / raw)
  To: linux-mm, Andrew Morton, linux-kernel
  Cc: Cyrill Gorcunov, Kirill Tkhai, Al Viro

It seems ->d_revalidate() could return any error (except ECHILD) to
abort validation and pass error as result of lookup sequence.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/proc/base.c |   27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9c8ca6cd3ce4..515ab29c2adf 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1962,9 +1962,12 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
 		goto out;
 
 	if (!dname_to_vma_addr(dentry, &vm_start, &vm_end)) {
-		down_read(&mm->mmap_sem);
-		exact_vma_exists = !!find_exact_vma(mm, vm_start, vm_end);
-		up_read(&mm->mmap_sem);
+		status = down_read_killable(&mm->mmap_sem);
+		if (!status) {
+			exact_vma_exists = !!find_exact_vma(mm, vm_start,
+							    vm_end);
+			up_read(&mm->mmap_sem);
+		}
 	}
 
 	mmput(mm);
@@ -2010,8 +2013,11 @@ static int map_files_get_link(struct dentry *dentry, struct path *path)
 	if (rc)
 		goto out_mmput;
 
+	rc = down_read_killable(&mm->mmap_sem);
+	if (rc)
+		goto out_mmput;
+
 	rc = -ENOENT;
-	down_read(&mm->mmap_sem);
 	vma = find_exact_vma(mm, vm_start, vm_end);
 	if (vma && vma->vm_file) {
 		*path = vma->vm_file->f_path;
@@ -2107,7 +2113,10 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 	if (!mm)
 		goto out_put_task;
 
-	down_read(&mm->mmap_sem);
+	result = ERR_PTR(-EINTR);
+	if (down_read_killable(&mm->mmap_sem))
+		goto out_put_mm;
+
 	vma = find_exact_vma(mm, vm_start, vm_end);
 	if (!vma)
 		goto out_no_vma;
@@ -2118,6 +2127,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 
 out_no_vma:
 	up_read(&mm->mmap_sem);
+out_put_mm:
 	mmput(mm);
 out_put_task:
 	put_task_struct(task);
@@ -2160,7 +2170,12 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
 	mm = get_task_mm(task);
 	if (!mm)
 		goto out_put_task;
-	down_read(&mm->mmap_sem);
+
+	ret = down_read_killable(&mm->mmap_sem);
+	if (ret) {
+		mmput(mm);
+		goto out_put_task;
+	}
 
 	nr_files = 0;
 


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps
  2019-05-15  8:41 [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Konstantin Khlebnikov
                   ` (3 preceding siblings ...)
  2019-05-15  8:41 ` [PATCH 5/5] proc: use down_read_killable for /proc/pid/map_files Konstantin Khlebnikov
@ 2019-05-15  9:05 ` Cyrill Gorcunov
  2019-05-15  9:16 ` Kirill Tkhai
  2019-05-17 12:41 ` Michal Hocko
  6 siblings, 0 replies; 14+ messages in thread
From: Cyrill Gorcunov @ 2019-05-15  9:05 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-mm, Andrew Morton, linux-kernel, Kirill Tkhai, Al Viro

On Wed, May 15, 2019 at 11:41:12AM +0300, Konstantin Khlebnikov wrote:
> Do not stuck forever if something wrong.
> This function also used for /proc/pid/smaps.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>

All patches in series look ok to me (actually I thought if there
is a scenario where might_sleep may trigger a warning, and didn't
find one, so should be safe).

Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps
  2019-05-15  8:41 [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Konstantin Khlebnikov
                   ` (4 preceding siblings ...)
  2019-05-15  9:05 ` [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Cyrill Gorcunov
@ 2019-05-15  9:16 ` Kirill Tkhai
  2019-05-17 12:41 ` Michal Hocko
  6 siblings, 0 replies; 14+ messages in thread
From: Kirill Tkhai @ 2019-05-15  9:16 UTC (permalink / raw)
  To: Konstantin Khlebnikov, linux-mm, Andrew Morton, linux-kernel
  Cc: Cyrill Gorcunov, Al Viro

On 15.05.2019 11:41, Konstantin Khlebnikov wrote:
> Do not stuck forever if something wrong.
> This function also used for /proc/pid/smaps.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>

For the series:

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  fs/proc/task_mmu.c   |    6 +++++-
>  fs/proc/task_nommu.c |    6 +++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 01d4eb0e6bd1..2bf210229daf 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -166,7 +166,11 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
>  	if (!mm || !mmget_not_zero(mm))
>  		return NULL;
>  
> -	down_read(&mm->mmap_sem);
> +	if (down_read_killable(&mm->mmap_sem)) {
> +		mmput(mm);
> +		return ERR_PTR(-EINTR);
> +	}
> +
>  	hold_task_mempolicy(priv);
>  	priv->tail_vma = get_gate_vma(mm);
>  
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index 36bf0f2e102e..7907e6419e57 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -211,7 +211,11 @@ static void *m_start(struct seq_file *m, loff_t *pos)
>  	if (!mm || !mmget_not_zero(mm))
>  		return NULL;
>  
> -	down_read(&mm->mmap_sem);
> +	if (down_read_killable(&mm->mmap_sem)) {
> +		mmput(mm);
> +		return ERR_PTR(-EINTR);
> +	}
> +
>  	/* start from the Nth VMA */
>  	for (p = rb_first(&mm->mm_rb); p; p = rb_next(p))
>  		if (n-- == 0)


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] proc: use down_read_killable for /proc/pid/map_files
  2019-05-15  8:41 ` [PATCH 5/5] proc: use down_read_killable for /proc/pid/map_files Konstantin Khlebnikov
@ 2019-05-15 17:00   ` Roman Gushchin
  0 siblings, 0 replies; 14+ messages in thread
From: Roman Gushchin @ 2019-05-15 17:00 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-mm, Andrew Morton, linux-kernel, Cyrill Gorcunov,
	Kirill Tkhai, Al Viro

On Wed, May 15, 2019 at 11:41:23AM +0300, Konstantin Khlebnikov wrote:
> It seems ->d_revalidate() could return any error (except ECHILD) to
> abort validation and pass error as result of lookup sequence.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> ---
>  fs/proc/base.c |   27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)

Hi!

The series looks good to me!

Reviewed-by: Roman Gushchin <guro@fb.com>
for all patches in the set.

The only thing, you need to add a bit more detailed commit messages
to patches 2 and 3.

Thanks!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps
  2019-05-15  8:41 [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Konstantin Khlebnikov
                   ` (5 preceding siblings ...)
  2019-05-15  9:16 ` Kirill Tkhai
@ 2019-05-17 12:41 ` Michal Hocko
  6 siblings, 0 replies; 14+ messages in thread
From: Michal Hocko @ 2019-05-17 12:41 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-mm, Andrew Morton, linux-kernel, Cyrill Gorcunov,
	Kirill Tkhai, Al Viro

On Wed 15-05-19 11:41:12, Konstantin Khlebnikov wrote:
> Do not stuck forever if something wrong.
> This function also used for /proc/pid/smaps.

I do agree that the killable variant is better but I do not understand
the changelog. What would keep the lock blocked for ever? I do not think
we have writer lock held for unbound amount of time anywhere, do we?

> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>

Other than that
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  fs/proc/task_mmu.c   |    6 +++++-
>  fs/proc/task_nommu.c |    6 +++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 01d4eb0e6bd1..2bf210229daf 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -166,7 +166,11 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
>  	if (!mm || !mmget_not_zero(mm))
>  		return NULL;
>  
> -	down_read(&mm->mmap_sem);
> +	if (down_read_killable(&mm->mmap_sem)) {
> +		mmput(mm);
> +		return ERR_PTR(-EINTR);
> +	}
> +
>  	hold_task_mempolicy(priv);
>  	priv->tail_vma = get_gate_vma(mm);
>  
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index 36bf0f2e102e..7907e6419e57 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -211,7 +211,11 @@ static void *m_start(struct seq_file *m, loff_t *pos)
>  	if (!mm || !mmget_not_zero(mm))
>  		return NULL;
>  
> -	down_read(&mm->mmap_sem);
> +	if (down_read_killable(&mm->mmap_sem)) {
> +		mmput(mm);
> +		return ERR_PTR(-EINTR);
> +	}
> +
>  	/* start from the Nth VMA */
>  	for (p = rb_first(&mm->mm_rb); p; p = rb_next(p))
>  		if (n-- == 0)

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] proc: use down_read_killable for /proc/pid/smaps_rollup
  2019-05-15  8:41 ` [PATCH 2/5] proc: use down_read_killable for /proc/pid/smaps_rollup Konstantin Khlebnikov
@ 2019-05-17 12:45   ` Michal Hocko
  2019-06-09  9:07     ` Konstantin Khlebnikov
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2019-05-17 12:45 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-mm, Andrew Morton, linux-kernel, Cyrill Gorcunov,
	Kirill Tkhai, Al Viro

On Wed 15-05-19 11:41:14, Konstantin Khlebnikov wrote:
> Ditto.

Proper changelog or simply squash those patches into a single patch if
you do not feel like copy&paste is fun

> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> ---
>  fs/proc/task_mmu.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 2bf210229daf..781879a91e3b 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -832,7 +832,10 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>  
>  	memset(&mss, 0, sizeof(mss));
>  
> -	down_read(&mm->mmap_sem);
> +	ret = down_read_killable(&mm->mmap_sem);
> +	if (ret)
> +		goto out_put_mm;

Why not ret = -EINTR. The seq_file code seems to be handling all errors
AFAICS.

> +
>  	hold_task_mempolicy(priv);
>  
>  	for (vma = priv->mm->mmap; vma; vma = vma->vm_next) {
> @@ -849,8 +852,9 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>  
>  	release_task_mempolicy(priv);
>  	up_read(&mm->mmap_sem);
> -	mmput(mm);
>  
> +out_put_mm:
> +	mmput(mm);
>  out_put_task:
>  	put_task_struct(priv->task);
>  	priv->task = NULL;

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] proc: use down_read_killable for /proc/pid/pagemap
  2019-05-15  8:41 ` [PATCH 3/5] proc: use down_read_killable for /proc/pid/pagemap Konstantin Khlebnikov
@ 2019-05-17 12:46   ` Michal Hocko
  0 siblings, 0 replies; 14+ messages in thread
From: Michal Hocko @ 2019-05-17 12:46 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-mm, Andrew Morton, linux-kernel, Cyrill Gorcunov,
	Kirill Tkhai, Al Viro

On Wed 15-05-19 11:41:19, Konstantin Khlebnikov wrote:
> Ditto.

ditto to the previous patch, including -EINTR.

> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> ---
>  fs/proc/task_mmu.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 781879a91e3b..78bed6adc62d 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1547,7 +1547,9 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
>  		/* overflow ? */
>  		if (end < start_vaddr || end > end_vaddr)
>  			end = end_vaddr;
> -		down_read(&mm->mmap_sem);
> +		ret = down_read_killable(&mm->mmap_sem);
> +		if (ret)
> +			goto out_free;
>  		ret = walk_page_range(start_vaddr, end, &pagemap_walk);
>  		up_read(&mm->mmap_sem);
>  		start_vaddr = end;

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] proc: use down_read_killable for /proc/pid/clear_refs
  2019-05-15  8:41 ` [PATCH 4/5] proc: use down_read_killable for /proc/pid/clear_refs Konstantin Khlebnikov
@ 2019-05-17 12:47   ` Michal Hocko
  0 siblings, 0 replies; 14+ messages in thread
From: Michal Hocko @ 2019-05-17 12:47 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-mm, Andrew Morton, linux-kernel, Cyrill Gorcunov,
	Kirill Tkhai, Al Viro

On Wed 15-05-19 11:41:21, Konstantin Khlebnikov wrote:
> Replace the only unkillable mmap_sem lock in clear_refs_write.

Poor changelog again.

The change itself looks ok to me.

> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> ---
>  fs/proc/task_mmu.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 78bed6adc62d..7f84d1477b5b 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1140,7 +1140,10 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
>  			goto out_mm;
>  		}
>  
> -		down_read(&mm->mmap_sem);
> +		if (down_read_killable(&mm->mmap_sem)) {
> +			count = -EINTR;
> +			goto out_mm;
> +		}
>  		tlb_gather_mmu(&tlb, mm, 0, -1);
>  		if (type == CLEAR_REFS_SOFT_DIRTY) {
>  			for (vma = mm->mmap; vma; vma = vma->vm_next) {

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] proc: use down_read_killable for /proc/pid/smaps_rollup
  2019-05-17 12:45   ` Michal Hocko
@ 2019-06-09  9:07     ` Konstantin Khlebnikov
  2019-06-10 18:58       ` Michal Hocko
  0 siblings, 1 reply; 14+ messages in thread
From: Konstantin Khlebnikov @ 2019-06-09  9:07 UTC (permalink / raw)
  To: Michal Hocko
  Cc: linux-mm, Andrew Morton, linux-kernel, Cyrill Gorcunov,
	Kirill Tkhai, Al Viro



On 17.05.2019 15:45, Michal Hocko wrote:
> On Wed 15-05-19 11:41:14, Konstantin Khlebnikov wrote:
>> Ditto.
> 
> Proper changelog or simply squash those patches into a single patch if
> you do not feel like copy&paste is fun
> 
>>
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>> ---
>>   fs/proc/task_mmu.c |    8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 2bf210229daf..781879a91e3b 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -832,7 +832,10 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>>   
>>   	memset(&mss, 0, sizeof(mss));
>>   
>> -	down_read(&mm->mmap_sem);
>> +	ret = down_read_killable(&mm->mmap_sem);
>> +	if (ret)
>> +		goto out_put_mm;
> 
> Why not ret = -EINTR. The seq_file code seems to be handling all errors
> AFAICS.
> 

I've missed your comment. Sorry.

down_read_killable returns 0 for success and exactly -EINTR for failure.

>> +
>>   	hold_task_mempolicy(priv);
>>   
>>   	for (vma = priv->mm->mmap; vma; vma = vma->vm_next) {
>> @@ -849,8 +852,9 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>>   
>>   	release_task_mempolicy(priv);
>>   	up_read(&mm->mmap_sem);
>> -	mmput(mm);
>>   
>> +out_put_mm:
>> +	mmput(mm);
>>   out_put_task:
>>   	put_task_struct(priv->task);
>>   	priv->task = NULL;
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] proc: use down_read_killable for /proc/pid/smaps_rollup
  2019-06-09  9:07     ` Konstantin Khlebnikov
@ 2019-06-10 18:58       ` Michal Hocko
  0 siblings, 0 replies; 14+ messages in thread
From: Michal Hocko @ 2019-06-10 18:58 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-mm, Andrew Morton, linux-kernel, Cyrill Gorcunov,
	Kirill Tkhai, Al Viro

On Sun 09-06-19 12:07:36, Konstantin Khlebnikov wrote:
[...]
> > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > index 2bf210229daf..781879a91e3b 100644
> > > --- a/fs/proc/task_mmu.c
> > > +++ b/fs/proc/task_mmu.c
> > > @@ -832,7 +832,10 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
> > >   	memset(&mss, 0, sizeof(mss));
> > > -	down_read(&mm->mmap_sem);
> > > +	ret = down_read_killable(&mm->mmap_sem);
> > > +	if (ret)
> > > +		goto out_put_mm;
> > 
> > Why not ret = -EINTR. The seq_file code seems to be handling all errors
> > AFAICS.
> > 
> 
> I've missed your comment. Sorry.
> 
> down_read_killable returns 0 for success and exactly -EINTR for failure.

You are right of course. I must have misread the code at the time. Sorry
about that.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-06-10 18:58 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-15  8:41 [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Konstantin Khlebnikov
2019-05-15  8:41 ` [PATCH 2/5] proc: use down_read_killable for /proc/pid/smaps_rollup Konstantin Khlebnikov
2019-05-17 12:45   ` Michal Hocko
2019-06-09  9:07     ` Konstantin Khlebnikov
2019-06-10 18:58       ` Michal Hocko
2019-05-15  8:41 ` [PATCH 3/5] proc: use down_read_killable for /proc/pid/pagemap Konstantin Khlebnikov
2019-05-17 12:46   ` Michal Hocko
2019-05-15  8:41 ` [PATCH 4/5] proc: use down_read_killable for /proc/pid/clear_refs Konstantin Khlebnikov
2019-05-17 12:47   ` Michal Hocko
2019-05-15  8:41 ` [PATCH 5/5] proc: use down_read_killable for /proc/pid/map_files Konstantin Khlebnikov
2019-05-15 17:00   ` Roman Gushchin
2019-05-15  9:05 ` [PATCH 1/5] proc: use down_read_killable for /proc/pid/maps Cyrill Gorcunov
2019-05-15  9:16 ` Kirill Tkhai
2019-05-17 12:41 ` Michal Hocko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.