linux-kernel - Re: [RFC PATCH 1/2] rust: add initial scatterlist bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <44a70b0c-af38-4706-90e3-0e0a7f6ad6f8@gmail.com>
Date: Mon, 26 May 2025 16:04:09 +0300
From: Abdiel Janulgue <abdiel.janulgue@...il.com>
To: Lyude Paul <lyude@...hat.com>, dakr@...nel.org
Cc: 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>,
 Petr Tesarik <petr@...arici.cz>, 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: [RFC PATCH 1/2] rust: add initial scatterlist bindings

Hi Lyude,

On 15/05/2025 23:01, Lyude Paul wrote:
> 
> I think you might have made a mistake below
> 
>> +impl<'a> Iterator for SGTableIter<'a> {
>> +    type Item = &'a SGEntry;
>> +
>> +    fn next(&mut self) -> Option<Self::Item> {
>> +        self.pos.map(|entry| {
>> +            let sg = entry.as_raw();
> 
>                 ^ sg is the last iterator position
> 
>> +            // SAFETY: `sg` is guaranteed to be valid and non-NULL while inside this closure.
>> +            unsafe {
>> +                let next = bindings::sg_next(sg);
> 
>                     ^ and we fetch the next iterator position
> 
>> +                self.pos = (!next.is_null()).then(|| SGEntry::as_ref(next));
>> +                SGEntry::as_ref(sg)
> 
>                     ^ but then we return the previous iterator position, sg
>> +            }
> 

Just had a second look at this again.I think the previous code is okay. 
I integrated the iterator fixes you suggested, however the mapped 
sgtable crashes on dma_unmap_sgtable().

One hint is that the fix seems to skip the initial entry of the sgtable? 
The previous code doesn't skip the initial entry when iterating:

fn next(&mut self) -> Option<Self::Item> {

   self.pos.take().map(|entry| {
      let sg = entry.as_raw();
      ^ sg is the current iterator position from self.pos

      let next = bindings::sg_next(sg);
      self.pos = (!next.is_null()).then(|| SGEntry::as_mut(next));
      ^ update the iterator position for the next loop to sg_next()
        or set to null if sg_next() is the last entry

      SGEntry::as_mut(sg)
      ^ Return the current iterator position
    })
}


Regards,
Abdiel