Messages in this thread | | | Subject | Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop | From | Jakob <> | Date | Wed, 23 Feb 2022 15:13:19 +0100 |
| |
> On 17. Feb 2022, at 20:28, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Thu, Feb 17, 2022 at 10:50 AM Jakob Koschel <jakobkoschel@gmail.com> wrote: >> >> It is unsafe to assume that &req->req != _req can only evaluate >> to false if the break within the list iterator is hit. > > I don't understand what problem you are trying to fix. > > Since "req" absolutely *has* to be stable for any of this code to be > valid, then "&req->req" is stable and unambiguous too. The *only* way > _req can point to it would be if we finished the iteration properly. > > So I don't see the unsafeness. > > Note that all this work with "speculative" execution fundamentally CAN > NOT affect semantics of the code, yet this patch makes statements > about exactly that.
I'm sorry for having created the confusion. I made this patch to support the speculative safe list_for_each_entry() version but it is not actually related to that. I do believe that this an actual bug and *could* *potentially* be misused. I'll follow up with an example to illustrate that.
I agree that this has nothing to do with the speculative execution iterator (apart from making it work) and should best be discussed separately.
I'll attach an example on how I think this code *can* become a problem. Note that this highly depends on the used compiler and how the struct offsets are laid out.
> > That's not how CPU speculation works. > > CPU speculation can expose hidden information that is not > "semantically important" (typically through cache access patterns, but > that's not the only way). So it might be exposing information it > shouldn't. > > But if speculation is actually changing semantics, then it's no longer > "speculation" - it's just a bug, plain and simple (either a software > bug due to insufficient serialization, or an actual hardware bug). > > IOW, I don't want to see these kinds of apparently pointless changes > to list walking. The patches should explain what that SECONDARY hidden > value you try to protect actually is for each case. > > This patch is basically not sensible. It just moves code around in a > way that the compiler could have done anyway (or the compiler could > decide to undo). It doesn't explain what the magic protected value is > that shouldn't be leaked, and it leaves the code just looking odd and > pointless. > > Linus
| |