> On 17. Feb 2022, at 20:28, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, Feb 17, 2022 at 10:50 AM Jakob Koschel <jakobkoschel@xxxxxxxxx> 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
