Re: [PATCH v4 03/11] rust: xarray: add `XArrayState`

[Andreas Hindborg <a.hindborg@kernel.org>]

Daniel Almeida <daniel.almeida@collabora.com> writes:

> Hi Andreas,
>
> My main worry here is cursor invalidation, i.e.: what happens if someone
> mutates the tree while we have live instances of XArrayState handed out via
> patch 7? For example, what if someone writes "self.state.guard.remove(...)"
> inside the kernel crate itself while some user code has an OccupiedEntry?

The type system handles this. Obtaining `Entry` requires a mutable
reference whose lifetime is extended to the `Entry`. So while entry is
alive, it is not possible to construct `XArrayState`.

For the operations directly on `Guard` that require shared ownership,
multiple `XArrayState` instances may exist at the same time. But as
these are only used for reads and can never modify the tree,
invalidation can not occur.

>
> I think this could be solved by hiding XArrayState in its own module and
> exposing only a small set of checked helpers while its fields remain private so
> that we carefully vet against the situation above.

I did not consider safe code within the xarray module modifying the
fields of the state object. Do we really need to handle that?

> Additionally we have to ensure that, in order to get the guard, one must
> destroy the XArrayState, for the same reason. This is currently the case with
> into_guard() IIUC, but I think it's worth to spell it out in the
> invariant section.

Since users outside of the module cannot construct `Guard` safely, I
don't think this is an issue?

But, something is not right. I did not consider the safety of
`XArrayState::new`, because it is a private method. But it should be
unsafe as it is, because it allows insert operations with only a shared
reference to the guard.

I think if I just gate he modifying operations (only
`XArrayState::insert` for now) on `R` being a mutable reference to the
guard, we should be fine?


Best regards,
Andreas Hindborg