[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250529161424.5cd95308@meshulam.tesarici.cz>
Date: Thu, 29 May 2025 16:14:24 +0200
From: Petr Tesařík <petr@...arici.cz>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: Abdiel Janulgue <abdiel.janulgue@...il.com>, acourbot@...dia.com,
dakr@...nel.org, lyude@...hat.com, Miguel Ojeda <ojeda@...nel.org>, Alex
Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo
<gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, Andreas
Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor
Gross <tmgross@...ch.edu>, Valentin Obst <kernel@...entinobst.de>, open
list <linux-kernel@...r.kernel.org>, Marek Szyprowski
<m.szyprowski@...sung.com>, Robin Murphy <robin.murphy@....com>,
airlied@...hat.com, rust-for-linux@...r.kernel.org, "open list:DMA MAPPING
HELPERS" <iommu@...ts.linux.dev>, Andrew Morton
<akpm@...ux-foundation.org>, Herbert Xu <herbert@...dor.apana.org.au>, Sui
Jingfeng <sui.jingfeng@...ux.dev>, Randy Dunlap <rdunlap@...radead.org>,
Michael Kelley <mhklinux@...look.com>
Subject: Re: [PATCH 1/2] rust: add initial scatterlist bindings
On Wed, 28 May 2025 21:45:50 -0300
Jason Gunthorpe <jgg@...pe.ca> wrote:
> On Thu, May 29, 2025 at 01:14:05AM +0300, Abdiel Janulgue wrote:
> > +impl SGEntry<Unmapped> {
> > + /// Set this entry to point at a given page.
> > + pub fn set_page(&mut self, page: &Page, length: u32, offset: u32) {
> > + let c: *mut bindings::scatterlist = self.0.get();
> > + // SAFETY: according to the `SGEntry` invariant, the scatterlist pointer is valid.
> > + // `Page` invariant also ensures the pointer is valid.
> > + unsafe { bindings::sg_set_page(c, page.as_ptr(), length, offset) };
> > + }
> > +}
>
> Wrong safety statement. sg_set_page captures the page.as_ptr() inside
> the C datastructure so the caller must ensure it holds a reference on
> the page while it is contained within the scatterlist.
>
> Which this API doesn't force to happen.
>
> Most likely for this to work for rust you have to take a page
> reference here and ensure the page reference is put back during sg
> destruction. A typical normal pattern would 'move' the reference from
> the caller into the scatterlist.
>
> I also think set_page should not be exposed to rust at all, it should
> probably only build scatterlists using the append APIs inside scatter
> tables where the entire model is much cleaner.
>
> Because this is also wrong in the sense that it destroys whatever
> sg_page was already there, which may have been holding a page refcount
> and thus it would leak it.
Thank you, Jason. You already made this suggestion for the RFC version,
and it begs the question if perhaps the underlying C API should also be
deprecated and eventually removed from the kernel.
A quick grep shows about 200 call sites, which may take some time to
remove, but if the API is apparently broken without people knowing, we
should start doing something about it.
Petr T
Powered by blists - more mailing lists