Re: [RFC PATCH 04/19] rust: fs: introduce `FileSystem::super_params`

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/30/23 09:21, Benno Lossin wrote:
On 28.10.23 18:39, Alice Ryhl wrote:
On 10/18/23 18:34, Benno Lossin wrote:>> +        from_result(|| {
+            // SAFETY: The C callback API guarantees that `fc_ptr` is valid.
+            let fc = unsafe { &mut *fc_ptr };

This safety comment is not enough, the pointer needs to be unique and
pointing to a valid value for this to be ok. I would recommend to do
this instead:

       unsafe { addr_of_mut!((*fc_ptr).ops).write(&Tables::<T>::CONTEXT) };

It doesn't really need to be unique. Or at least, that wording gives the
wrong intuition even if it's technically correct when you use the right
definition of "unique".

To clarify what I mean: Using `ptr::write` on a raw pointer is valid if
and only if creating a mutable reference and using that to write is
valid. (Assuming the type has no destructor.)

I tried looking in the nomicon and UCG, but was not able to find this
statement, where is it from?

Not sure where I got it from originally, but it follows from the tree borrows reference:

First, if the type is !Unpin, then the mutable reference gets the same tag as the original pointer, so there's trivially no difference.

The more interesting case is for Unpin types. Here, the creation of the mutable reference corresponds to a read, and then there's the write of the mutable reference itself. The write of the mutable reference itself is equivalent to the `ptr::write` operation, since exactly the same tags are considered to be affected by child writes and foreign writes. Next, it must be shown that [read, write] is equivalent to just a write, which can be shown by analyzing the tree borrows rules case-by-case.

You can find a nice summary of tree borrows at the last page of:
https://github.com/Vanille-N/tree-borrows/blob/master/full/main.pdf

I'm pretty sure the same analysis works with stacked borrows.

Of course, in this case you *also* have the difference of whether you
create a mutable to the entire struct or just the field.
+                // SAFETY: This is a newly-created inode. No other references to it exist, so it is
+                // safe to mutably dereference it.
+                let inode = unsafe { &mut *inode };

The inode also needs to be initialized and have valid values as its fields.
Not sure if this is kept and it would probably be better to keep using raw
pointers here.

My understanding is that this is just a safety invariant, and not a
validity invariant, so as long as the uninitialized memory is not read,
it's fine.

See e.g.:
https://github.com/rust-lang/unsafe-code-guidelines/issues/346

I'm not so sure that that discussion is finished and agreed upon. The
nomicon still writes "It is illegal to construct a reference to
uninitialized data" [1].

Using this pattern (&mut uninit to initialize data) is also dangerous
if the underlying type has drop impls, since then by doing
`foo.bar = baz;` you drop the old uninitialized value. Sure in
our bindings there are no types that implement drop (AFAIK) so
it is less of an issue.

If we decide to do this, we should have a comment that explains that
this reference might point to uninitialized memory. Since otherwise
it might be easy to give the reference to another safe function that
then e.g. reads a bool.

[1]: https://doc.rust-lang.org/nomicon/unchecked-uninit.html

That's fair. I agree that we should explicitly decide whether or not to allow this kind of thing.

Alice





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux