Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f8b738940f2502e582acb59229d419825c9a1ffb.camel@collabora.com>
Subject: Re: [PATCH v2 2/2] rust: virtio: add virtio support
From:   Daniel Almeida <daniel.almeida@collabora.com>
To:     Wedson Almeida Filho <wedsonaf@gmail.com>
Cc:     ojeda@kernel.org, rust-for-linux@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org
Date:   Fri, 07 Apr 2023 20:36:35 -0300
In-Reply-To: <CANeycqoh+ePXODJ57UT1UdhHDgzDXr=KoQEo7HiSDJDHha2dsQ@mail.gmail.com>
References: <20230405201416.395840-1-daniel.almeida@collabora.com>
         <20230405201416.395840-3-daniel.almeida@collabora.com>
         <CANeycqoh+ePXODJ57UT1UdhHDgzDXr=KoQEo7HiSDJDHha2dsQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 
MIME-Version: 1.0
Precedence: bulk

Hi Wedson, Martin,

First of all, thanks for the review.


> > +=C2=A0=C2=A0=C2=A0 /// VirtIO driver remove.
> > +=C2=A0=C2=A0=C2=A0 ///
> > +=C2=A0=C2=A0=C2=A0 /// Called when a virtio device is removed.
> > +=C2=A0=C2=A0=C2=A0 /// Implementers should prepare the device for comp=
lete
> > removal here.
> > +=C2=A0=C2=A0=C2=A0 ///
> > +=C2=A0=C2=A0=C2=A0 /// In particular, implementers must ensure no buff=
ers are
> > left over in the
> > +=C2=A0=C2=A0=C2=A0 /// virtqueues in use by calling [`virtqueue::get_b=
uf()`]
> > until `None` is
> > +=C2=A0=C2=A0=C2=A0 /// returned.
>=20
> What happens if implementers don't do this?
>=20
> If this is a safety requirement, we need to find a different way to
> enforce it.
>=20
> >=20

This is the worst part of this patch by far, unfortunately. If one
doesn't do this, then s/he will leak the `data` field passed in through
into_foreign() here:


> +        // SAFETY: `self.ptr` is valid as per the type invariant.
> +        let res =3D unsafe {
> +            bindings::virtqueue_add_sgs(
> +                self.ptr,
> +                sgs.as_mut_ptr(),
> +                num_out as u32,
> +                num_in as u32,
> +                data.into_foreign() as _,
> +                gfp,
> +            )
> +        };
> +

Note the comment here:


> +            // SAFETY: if there is a buffer token, it came from
> +            // `into_foreign()` as called in `add_sgs()`.
> +            <T::PrivateData as ForeignOwnable>::from_foreign(buf)


To be honest, I tried coming up with something clever to solve this,
but couldn't. Ideally this should happen when this function is called:

> +    extern "C" fn remove_callback(virtio_device: *mut
bindings::virtio_device) {


But I did not find a way to iterate over the the `vqs` member from the
Rust side, i.e.:

```

struct virtio_device {
	int index;
	bool failed;
	bool config_enabled;
	bool config_change_pending;
	spinlock_t config_lock;
	spinlock_t vqs_list_lock; /* Protects VQs list access */
	struct device dev;
	struct virtio_device_id id;
	const struct virtio_config_ops *config;
	const struct vringh_config_ops *vringh_config;
	struct list_head vqs; <------------------
```

Is there any wrappers over list_for_each_entry(), etc, to be used from
Rust? If so, I could not find them.

Doing this cleanup from Virtqueue::Drop() is not an option either:
since we wrap over a pointer owned by the kernel, there's no guarantee
that the actual virtqueue is going away when drop is called on the
wrapper. In fact, this is never the case, as virtqueues are deleted
through this call:


> +void rust_helper_virtio_del_vqs(struct virtio_device *vdev)
> +{
> +       vdev->config->del_vqs(vdev);
> +}
> +


Suggestions welcome,

-- Daniel