All of lore.kernel.org
 help / color / mirror / Atom feed
From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman)
To: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Linus Torvalds
	<torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>,
	Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>,
	Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
	Roland McGrath <roland-/Z5OmTQCD9xF6kxbq+BtvQ@public.gmane.org>,
	Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>,
	David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
	"Michael Kerrisk (man-pages)"
	<mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock
Date: Wed, 07 Jun 2017 09:08:30 -0500	[thread overview]
Message-ID: <87vao7riu9.fsf@xmission.com> (raw)
In-Reply-To: <20170607123657.GA22199-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> (Oleg Nesterov's message of "Wed, 7 Jun 2017 14:36:57 +0200")

Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:

> Hi Eric,
>
> I'll try very much to read this series tomorrow, can't do this today...
>
> On 06/06, Eric W. Biederman wrote:
>>
>> @@ -1380,13 +1380,6 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
>>  			return -EPERM;
>>  	}
>>  
>> -	/* protect tsk->signal and tsk->sighand from disappearing */
>> -	read_lock(&tasklist_lock);
>> -	if (!tsk->sighand) {
>> -		retval = -ESRCH;
>> -		goto out;
>> -	}
>
> Yes, the comment is wrong.
>
> However we do need read_lock(tasklist_lock) to access ->group_leader. And the
> ->sighand != NULL check ensures that ->group_leader is the valid
> pointer.

As of 4.12-rc1 The code does not access group_leader anymore.

> Also, update_rlimit_cpu() is not safe without tasklist / sighand-check.
>
> We can probably change this code to rely on rcu.

Good point a NULL sighand will cause update_rlimit_cpu to OOPS.

Grr.  There is a point in my tree where this is perfectly safe.  But not
at this point.  Consider this patch dropped for the moment.

Eric

WARNING: multiple messages have this Message-ID (diff)
From: ebiederm@xmission.com (Eric W. Biederman)
To: Oleg Nesterov <oleg@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Ingo Molnar <mingo@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Kees Cook <keescook@chromium.org>,
	Roland McGrath <roland@hack.frob.com>,
	Al Viro <viro@ZenIV.linux.org.uk>,
	David Howells <dhowells@redhat.com>,
	"Michael Kerrisk \(man-pages\)" <mtk.manpages@gmail.com>
Subject: Re: [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock
Date: Wed, 07 Jun 2017 09:08:30 -0500	[thread overview]
Message-ID: <87vao7riu9.fsf@xmission.com> (raw)
In-Reply-To: <20170607123657.GA22199@redhat.com> (Oleg Nesterov's message of "Wed, 7 Jun 2017 14:36:57 +0200")

Oleg Nesterov <oleg@redhat.com> writes:

> Hi Eric,
>
> I'll try very much to read this series tomorrow, can't do this today...
>
> On 06/06, Eric W. Biederman wrote:
>>
>> @@ -1380,13 +1380,6 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
>>  			return -EPERM;
>>  	}
>>  
>> -	/* protect tsk->signal and tsk->sighand from disappearing */
>> -	read_lock(&tasklist_lock);
>> -	if (!tsk->sighand) {
>> -		retval = -ESRCH;
>> -		goto out;
>> -	}
>
> Yes, the comment is wrong.
>
> However we do need read_lock(tasklist_lock) to access ->group_leader. And the
> ->sighand != NULL check ensures that ->group_leader is the valid
> pointer.

As of 4.12-rc1 The code does not access group_leader anymore.

> Also, update_rlimit_cpu() is not safe without tasklist / sighand-check.
>
> We can probably change this code to rely on rcu.

Good point a NULL sighand will cause update_rlimit_cpu to OOPS.

Grr.  There is a point in my tree where this is perfectly safe.  But not
at this point.  Consider this patch dropped for the moment.

Eric

  parent reply	other threads:[~2017-06-07 14:08 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-06 19:01 [PATCH 00/26] Fixing wait, exit, ptrace, exec, and CLONE_THREAD Eric W. Biederman
2017-06-06 19:01 ` Eric W. Biederman
2017-06-06 19:03 ` [PATCH 01/26] alpha: Remove unused TASK_GROUP_LEADER Eric W. Biederman
2017-06-06 19:03   ` [PATCH 02/26] cgroup: Don't open code tasklist_empty() Eric W. Biederman
2017-06-06 19:03   ` [PATCH 03/26] signal: Do not perform permission checks when sending pdeath_signal Eric W. Biederman
     [not found]     ` <20170606190338.28347-3-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2017-06-06 20:01       ` Linus Torvalds
2017-06-06 20:01         ` Linus Torvalds
     [not found]         ` <CA+55aFya7CgNozFrhQ9qk40UhZAD8SMva1+Y1vQ0YUEbpUpQUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-07 11:23           ` Eric W. Biederman
2017-06-07 11:23             ` Eric W. Biederman
2017-06-06 21:42       ` Richard Weinberger
2017-06-06 21:42         ` Richard Weinberger
2017-06-06 19:03   ` [PATCH 04/26] signal: Make group_send_sig_info static Eric W. Biederman
2017-06-06 19:03   ` [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock Eric W. Biederman
2017-06-07 12:36     ` Oleg Nesterov
     [not found]       ` <20170607123657.GA22199-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-06-07 14:08         ` Eric W. Biederman [this message]
2017-06-07 14:08           ` Eric W. Biederman
2017-06-06 19:03   ` [PATCH 07/26] pidns: Improve the error handling in alloc_pid Eric W. Biederman
2017-06-06 19:03   ` [PATCH 08/26] exit: Make the runqueue rcu safe Eric W. Biederman
     [not found]     ` <20170606190338.28347-8-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2017-06-07 13:16       ` Oleg Nesterov
2017-06-07 13:16         ` Oleg Nesterov
2017-06-06 19:03   ` [PATCH 09/26] signal: Don't allow sending SIGKILL or SIGSTOP to init Eric W. Biederman
2017-06-06 19:03   ` [PATCH 10/26] ptrace: Simplify ptrace_detach & exit_ptrace Eric W. Biederman
2017-06-06 19:03   ` [PATCH 11/26] wait: Properly implement __WCLONE handling in the presence of exec and ptrace Eric W. Biederman
2017-06-06 19:03   ` [PATCH 12/26] wait: Directly test for the two cases where wait_task_zombie is called Eric W. Biederman
2017-06-06 19:03   ` [PATCH 17/26] exit: Rework the exit states for ptracees Eric W. Biederman
2017-06-06 19:03   ` [PATCH 21/26] wait: Optmize waitpid Eric W. Biederman
     [not found]   ` <20170606190338.28347-1-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2017-06-06 19:03     ` [PATCH 05/26] exit: Remove the pointless clearing of SIGPENDING in __exit_signal Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 13/26] wait: Remove unused delay_group_leader Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 14/26] wait: Move changing of ptrace from wait_consider_task into wait_task_stopped Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 15/26] wait: Don't delay !ptrace_reparented leaders Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 16/26] exit: Fix reporting a ptraced !reparented leader has exited Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 18/26] wait: Fix WSTOPPED on a ptraced child Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 19/26] wait: Simpler code for clearing notask_error in wait_consider_task Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 20/26] wait: Don't pass the list to wait_consider_task Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 22/26] exit: Fix auto-wait of ptraced children Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 23/26] signal: Fix SIGCONT before group stop completes Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03     ` [PATCH 24/26] signal: In ptrace_stop improve identical signal detection Eric W. Biederman
2017-06-06 19:03       ` Eric W. Biederman
2017-06-06 19:03   ` [PATCH 25/26] signal: In ptrace_stop use CLD_TRAPPED in all ptrace signals Eric W. Biederman
2017-06-06 19:03   ` [PATCH 26/26] pidns: Ensure zap_pid_ns_processes always terminates Eric W. Biederman
     [not found] ` <877f0pym71.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2017-06-06 19:40   ` [PATCH 00/26] Fixing wait, exit, ptrace, exec, and CLONE_THREAD Aleksa Sarai
2017-06-06 19:40     ` Aleksa Sarai
     [not found]     ` <dd16b1bb-e99e-69f2-72f4-1be4cb24d18d-l3A5Bk7waGM@public.gmane.org>
2017-06-07 11:36       ` Eric W. Biederman
2017-06-07 11:36         ` Eric W. Biederman
     [not found]         ` <87ink8vxkf.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2017-06-07 12:21           ` Aleksa Sarai
2017-06-07 12:21             ` Aleksa Sarai
2017-06-07 11:36       ` Eric W. Biederman
2017-06-06 20:07   ` Linus Torvalds
2017-06-06 20:07     ` Linus Torvalds
     [not found]     ` <CA+55aFze5rR+rGcG6kt=8PtfgAcs02jqQ7Gm-1=1MzkbA7_nqA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-07 15:59       ` Eric W. Biederman
2017-06-07 15:59         ` Eric W. Biederman
2017-06-07 15:59       ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87vao7riu9.fsf@xmission.com \
    --to=ebiederm-as9lmozglivwk0htik3j/w@public.gmane.org \
    --cc=dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
    --cc=keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
    --cc=linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
    --cc=roland-/Z5OmTQCD9xF6kxbq+BtvQ@public.gmane.org \
    --cc=tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org \
    --cc=torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org \
    --cc=viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.