Re: [PATCH v4 02/13] rust: sync: introduce `Lock` and `Guard`

[Alice Ryhl]

On 4/12/23 13:38, Wedson Almeida Filho wrote:
> On Tue, 11 Apr 2023 at 17:42, Gary Guo <gary@xxxxxxxxxxx> wrote:
> > On Tue, 11 Apr 2023 02:45:32 -0300
> > Wedson Almeida Filho <wedsonaf@xxxxxxxxx> wrote:
> >
> > > From: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> > >
> > > They are generic Rust implementations of a lock and a lock guard that
> > > contain code that is common to all locks. Different backends will be
> > > introduced in subsequent commits.
> > >
> > > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@xxxxxxxxx>
> > > Suggested-by: Gary Guo <gary@xxxxxxxxxxx>
> > > Signed-off-by: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> > > --- >>
> > There is not drop implementation on `Lock`, which implies all locks can
> > be just forgotten?
>
> Yes, all locks can be forgotten.

Are the locks not pinned? Pinning comes with the guarantee that the 
value remains valid until the destructor runs, so the only kind of 
forgetting you can do to a lock is the one where the memory containing 
it is leaked, meaning that its not a UAF to use it after forgetting it.

My understanding is that the issue had to do with forgetting the guard 
types, since those were not pinned.

> > I believe we discussed a case where this is can lead to UAF when a lock
> > is dropped while it is locked (e.g. because the guard is forgotten).
>
> Yes, this is the issue brought up by Boqun:
> https://github.com/Rust-for-Linux/linux/issues/862
>
> The issue arises when a mutex guard is forgotten and the task that
> owns it exits. Then another task trying to acquire the mutex will lead
> to a UAF. A drop implementation on the lock doesn't solve this.
>
> One solution is to increment the refcount on the current task when we
> acquire the mutex and decrement it when we release, but if we do that,
> the cost of acquiring/releasing a mutex gets much worse in Rust than
> it is in C.
>
> Another solution might be to force disable CONFIG_MUTEX_SPIN_ON_OWNER
> when Rust is enabled, which is undesirable because it affects the
> performance of C code as well.
>
> Even a closure-based lock (which I believe you suggested at the time)
> doesn't solve this completely because the thread may exit during the
> closure execution and leave a dangling pointer in the mutex.
>
> So we don't have a good solution for this yet.