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:   Fri, 16 Apr 2021 14:07:49 +0100
From:   Wedson Almeida Filho <wedsonaf@google.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     ojeda@kernel.org, Linus Torvalds <torvalds@linux-foundation.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        rust-for-linux@vger.kernel.org, linux-kbuild@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 00/13] [RFC] Rust support
Message-ID: <YHmMJWmzz2vZ3qQH@google.com>
References: <20210414184604.23473-1-ojeda@kernel.org>
 <YHlz54rd1YQHsOA/@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YHlz54rd1YQHsOA/@hirez.programming.kicks-ass.net>
Precedence: bulk

On Fri, Apr 16, 2021 at 01:24:23PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 14, 2021 at 08:45:51PM +0200, ojeda@kernel.org wrote:
> >   - Featureful language: sum types, pattern matching, generics,
> >     RAII, lifetimes, shared & exclusive references, modules &
> >     visibility, powerful hygienic and procedural macros...
> 
> IMO RAII is over-valued, but just in case you care, the below seems to
> work just fine. No fancy new language needed, works today. Similarly you
> can create refcount_t guards, or with a little more work full blown
> smart_ptr crud.

Peter, we do care, thank you for posting this. It's a great example for us to
discuss some of the minutiae of what we think Rust brings to the table in
addition to what's already possible in C.

> 
> ---
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index e19323521f9c..f03a72dd8cea 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -197,4 +197,22 @@ extern void mutex_unlock(struct mutex *lock);
>  
>  extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
>  
> +struct mutex_guard {
> +	struct mutex *mutex;
> +};
> +
> +static inline struct mutex_guard mutex_guard_lock(struct mutex *mutex)
> +{
> +	mutex_lock(mutex);
> +	return (struct mutex_guard){ .mutex = mutex, };
> +}
> +
> +static inline void mutex_guard_unlock(struct mutex_guard *guard)
> +{
> +	mutex_unlock(guard->mutex);
> +}
> +
> +#define DEFINE_MUTEX_GUARD(name, lock)			\
> +	struct mutex_guard __attribute__((__cleanup__(mutex_guard_unlock))) name = mutex_guard_lock(lock)
> +
>  #endif /* __LINUX_MUTEX_H */
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 8ee3249de2f0..603d197a83b8 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5715,16 +5715,15 @@ static long perf_compat_ioctl(struct file *file, unsigned int cmd,
>  
>  int perf_event_task_enable(void)
>  {
> +	DEFINE_MUTEX_GUARD(event_mutex, &current->perf_event_mutex);

There is nothing in C forcing developers to actually use DEFINE_MUTEX_GUARD. So
someone may simply forget (or not know that they need) to lock
current->perf_event_mutex and directly access some field protected by it. This
is unlikely to happen when one first writes the code, but over time as different
people modify the code and invariants change, it is possible for this to happen.

In Rust, this isn't possible: the data protected by a lock is only accessible
when the lock is locked. So developers cannot accidentally make mistakes of this
kind. And since the enforcement happens at compile time, there is no runtime
cost.

This, we believe, is fundamental to the discussion: we agree that many of these
idioms can be implemented in C (albeit in this case with a compiler extension),
but their use is optional, people can (and do) still make mistakes that lead to
vulnerabilities; Rust disallows classes of  mistakes by construction.

Another scenario: suppose within perf_event_task_enable you need to call a
function that requires the mutex to be locked and that will unlock it for you on
error (or unconditionally, doesn't matter). How would you do that in C? In Rust,
there is a clean idiomatic way of transferring ownership of a guard (or any
other object) such that the previous owner cannot continue to use it after
ownership is transferred. Again, this is enforced at compile time. I'm happy to
provide a small example if that would help.

Again, thanks for bringing this up. And please keep your concerns and feedback
coming, we very much want to have these discussions and try to improve what we
have based on feedback from the community.

>  	struct perf_event_context *ctx;
>  	struct perf_event *event;
>  
> -	mutex_lock(&current->perf_event_mutex);
>  	list_for_each_entry(event, &current->perf_event_list, owner_entry) {
>  		ctx = perf_event_ctx_lock(event);
>  		perf_event_for_each_child(event, _perf_event_enable);
>  		perf_event_ctx_unlock(event, ctx);
>  	}
> -	mutex_unlock(&current->perf_event_mutex);
>  
>  	return 0;
>  }