Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 Jan 2021 16:07:09 +0100
From:   Piotr Figiel <figiel@google.com>
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     Alexey Dobriyan <adobriyan@gmail.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Kees Cook <keescook@chromium.org>,
        Alexey Gladkov <gladkov.alexey@gmail.com>,
        Michel Lespinasse <walken@google.com>,
        Bernd Edlinger <bernd.edlinger@hotmail.de>,
        Andrei Vagin <avagin@gmail.com>,
        mathieu.desnoyers@efficios.com, viro@zeniv.linux.org.uk,
        peterz@infradead.org, paulmck@kernel.org, boqun.feng@gmail.com,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        posk@google.com, kyurtsever@google.com, ckennelly@google.com,
        pjt@google.com
Subject: Re: [PATCH v3] fs/proc: Expose RSEQ configuration
Message-ID: <YBGBnQWJDVZ7Y7s6@google.com>
References: <20210126185412.175204-1-figiel@google.com>
 <20210126112547.d3f18b6a2abe864e8bfa7fcd@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210126112547.d3f18b6a2abe864e8bfa7fcd@linux-foundation.org>
Precedence: bulk

On Tue, Jan 26, 2021 at 11:25:47AM -0800, Andrew Morton wrote:
> On Tue, 26 Jan 2021 19:54:12 +0100 Piotr Figiel <figiel@google.com> wrote:
> > To achieve above goals expose the RSEQ structure address and the
> > signature value with the new per-thread procfs file "rseq".
> Using "/proc/<pid>/rseq" would be more informative.
> 
> >  fs/exec.c      |  2 ++
> >  fs/proc/base.c | 22 ++++++++++++++++++++++
> >  kernel/rseq.c  |  4 ++++
> 
> A Documentation/ update would be appropriate.
> 
> > +	task_lock(current);
> >  	rseq_execve(current);
> > +	task_unlock(current);
> 
> There's a comment over the task_lock() implementation which explains
> what things it locks.  An update to that would be helpful.

Agreed I'll include fixes for above comments in v4.

> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -662,6 +662,22 @@ static int proc_pid_syscall(struct seq_file *m, struct pid_namespace *ns,
> >  
> >  	return 0;
> >  }
> > +
> > +#ifdef CONFIG_RSEQ
> > +static int proc_pid_rseq(struct seq_file *m, struct pid_namespace *ns,
> > +				struct pid *pid, struct task_struct *task)
> > +{
> > +	int res = lock_trace(task);
> > +
> > +	if (res)
> > +		return res;
> > +	task_lock(task);
> > +	seq_printf(m, "%px %08x\n", task->rseq, task->rseq_sig);
> > +	task_unlock(task);
> > +	unlock_trace(task);
> > +	return 0;
> > +}
> 
> Do we actually need task_lock() for this purpose?  Would
> exec_update_lock() alone be adequate and appropriate?

Now rseq syscall which modifies those fields isn't synchronised with
exec_update_lock. So either a new lock or task_lock() could be used or
exec_update_lock could be reused in the syscall.  I decided against
exec_update_lock reuse in the syscall because it's normally used to
guard access checks against concurrent setuid exec. This could be
potentially confusing as it's not relevant for the the rseq syscall
code.
I think task_lock usage here is also consistent with how it's used
across the kernel.

Whether we need consistent rseq and rseq_sig pairs in the proc output, I
think there's some argument for it (discussed also in parallel thread
with Mathieu Desnoyers).

Best regards,
Piotr.