From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755967AbcEaHce (ORCPT ); Tue, 31 May 2016 03:32:34 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33144 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755527AbcEaHcb (ORCPT ); Tue, 31 May 2016 03:32:31 -0400 Date: Tue, 31 May 2016 09:32:28 +0200 From: Michal Hocko To: Oleg Nesterov Cc: linux-mm@kvack.org, Tetsuo Handa , David Rientjes , Vladimir Davydov , Andrew Morton , LKML Subject: Re: [PATCH 1/6] proc, oom: drop bogus task_lock and mm check Message-ID: <20160531073227.GA26128@dhcp22.suse.cz> References: <1464613556-16708-1-git-send-email-mhocko@kernel.org> <1464613556-16708-2-git-send-email-mhocko@kernel.org> <20160530174324.GA25382@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160530174324.GA25382@redhat.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 30-05-16 19:43:24, Oleg Nesterov wrote: > On 05/30, Michal Hocko wrote: > > > > both oom_adj_write and oom_score_adj_write are using task_lock, > > check for task->mm and fail if it is NULL. This is not needed because > > the oom_score_adj is per signal struct so we do not need mm at all. > > The code has been introduced by 3d5992d2ac7d ("oom: add per-mm oom > > disable count") but we do not do per-mm oom disable since c9f01245b6a7 > > ("oom: remove oom_disable_count"). > > > > The task->mm check is even not correct because the current thread might > > have exited but the thread group might be still alive - e.g. thread > > group leader would lead that echo $VAL > /proc/pid/oom_score_adj would > > always fail with EINVAL while /proc/pid/task/$other_tid/oom_score_adj > > would succeed. This is unexpected at best. > > > > Remove the lock along with the check to fix the unexpected behavior > > and also because there is not real need for the lock in the first place. > > ACK thanks! > and we should also remove lock_task_sighand(). as for oom_adj_read() and > oom_score_adj_read() we can just remove it right now; it was previously > needed to ensure the task->signal != NULL, today this is always true. OK, I will add the following patch to the series. --- >>From 952c464a31ffbe158233c4cc05f4b8a64384635c Mon Sep 17 00:00:00 2001 From: Michal Hocko Date: Tue, 31 May 2016 09:28:36 +0200 Subject: [PATCH] proc, oom: drop bogus sighand lock Oleg has pointed out that can simplify both oom_adj_write and oom_score_adj_write even further and drop the sighand lock. The only purpose of the lock was to protect p->signal from going away but this will not happen since ea6d290ca34c ("signals: make task_struct->signal immutable/refcountable"). Suggested-by: Oleg Nesterov Signed-off-by: Michal Hocko --- fs/proc/base.c | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index a6014e45c516..3761f107615a 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1057,7 +1057,6 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf, struct task_struct *task; char buffer[PROC_NUMBUF]; int oom_adj; - unsigned long flags; int err; memset(buffer, 0, sizeof(buffer)); @@ -1083,11 +1082,6 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf, goto out; } - if (!lock_task_sighand(task, &flags)) { - err = -ESRCH; - goto err_put_task; - } - /* * Scale /proc/pid/oom_score_adj appropriately ensuring that a maximum * value is always attainable. @@ -1100,7 +1094,7 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf, if (oom_adj < task->signal->oom_score_adj && !capable(CAP_SYS_RESOURCE)) { err = -EACCES; - goto err_sighand; + goto err_put_task; } /* @@ -1113,8 +1107,6 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf, task->signal->oom_score_adj = oom_adj; trace_oom_score_adj_update(task); -err_sighand: - unlock_task_sighand(task, &flags); err_put_task: put_task_struct(task); out: @@ -1152,7 +1144,6 @@ static ssize_t oom_score_adj_write(struct file *file, const char __user *buf, { struct task_struct *task; char buffer[PROC_NUMBUF]; - unsigned long flags; int oom_score_adj; int err; @@ -1179,15 +1170,10 @@ static ssize_t oom_score_adj_write(struct file *file, const char __user *buf, goto out; } - if (!lock_task_sighand(task, &flags)) { - err = -ESRCH; - goto err_put_task; - } - if ((short)oom_score_adj < task->signal->oom_score_adj_min && !capable(CAP_SYS_RESOURCE)) { err = -EACCES; - goto err_sighand; + goto err_put_task; } task->signal->oom_score_adj = (short)oom_score_adj; @@ -1195,8 +1181,6 @@ static ssize_t oom_score_adj_write(struct file *file, const char __user *buf, task->signal->oom_score_adj_min = (short)oom_score_adj; trace_oom_score_adj_update(task); -err_sighand: - unlock_task_sighand(task, &flags); err_put_task: put_task_struct(task); out: -- 2.8.1 -- Michal Hocko SUSE Labs