mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: "Eric W. Biederman" <ebiederm@xmission.com>
To: "Maciej W. Rozycki" <macro@orcam.me.uk>
Cc: Arnd Bergmann <arnd@arndb.de>,
	 Linus Torvalds <torvalds@linux-foundation.org>,
	 Richard Henderson <richard.henderson@linaro.org>,
	 Ivan Kokshaysky <ink@unseen.parts>,
	 Matt Turner <mattst88@gmail.com>,
	 John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>,
	 Magnus Lindholm <linmag7@gmail.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	 Alexander Viro <viro@zeniv.linux.org.uk>,
	 linux-alpha@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Alpha: Emulate unaligned LDx_L/STx_C for data consistency
Date: Wed, 09 Apr 2025 23:37:34 -0500	[thread overview]
Message-ID: <874iywtywh.fsf@email.froward.int.ebiederm.org> (raw)
In-Reply-To: <alpine.DEB.2.21.2504092019200.18515@angie.orcam.me.uk> (Maciej W. Rozycki's message of "Wed, 9 Apr 2025 21:59:59 +0100 (BST)")

"Maciej W. Rozycki" <macro@orcam.me.uk> writes:

> On Wed, 9 Apr 2025, Eric W. Biederman wrote:
>
>> >> So unless you actually *see* the unaligned faults, I really think you
>> >> shouldn't emulate them.
>> >>
>> >> And I'd like to know where they are if you do see them
>> 
>> I was nerd sniped by this so I took a look.
>> 
>> I have a distinct memory that even the ipv4 stack can generate unaligned
>> loads.  Looking at the code in net/ipv4/ip_input.c:ip_rcv_finish_core
>> there are several unprotected accesses to iph->daddr.
>> 
>> Which means that if the lower layers ever give something that is not 4
>> byte aligned for ipv4 just reading the destination address will be an
>> unaligned read.
>> 
>> There are similar unprotected accesses to the ipv6 destination address
>> but it is declared as an array of bytes.  So that address can not
>> be misaligned.
>> 
>> There is a theoretical path through 802.2 that adds a 3 byte sap
>> header that could cause problems.  We have LLC_SAP_IP defined
>> but I don't see anything calling register_8022_client that would
>> be needed to hook that up to the ipv4 stack.
>> 
>> As long as the individual ethernet drivers have the hardware deliver
>> packets 2 bytes into an aligned packet buffer the 14 byte ethernet
>> header will end on a 16 byte aligned location, I don't think there
>> is a way to trigger unaligned behavior with ipv4 or ipv6.
>> 
>> Hmm.  Looking appletalk appears to be built on top of SNAP.
>> So after the ethernet header processing the code goes through
>> net/llc/llc_input.c:llc_rcv and then net/802/snap_rcv before
>> reaching any of the appletalk protocols.
>> 
>> I think the common case for llc would be 3 bytes + 5 bytes for snap,
>> for 8 bytes in the common case.  But the code seems to be reading
>> 4 or 5 bytes for llc so I am confused.  In either case it definitely
>> appears there are cases where the ethernet headers before appletalk
>> can be an odd number of bytes which has the possibility of unaligning
>> everything.
>> 
>> Both of the appletalk protocols appear to make unguarded 16bit reads
>> from their headers.  So having a buffer that is only 1 byte aligned
>> looks like it will definitely be a problem.
>
>  Thank you for your analysis, really insightful.
>
>> > FWIW, all the major architectures that have variants without
>> > unaligned load/store (arm32, mips, ppc, riscv) trap and emulate
>> > them for both user and kernel access for normal memory, but
>> > they don't emulate it for atomic ll/sc type instructions.
>> > These instructions also trap and kill the task on the
>> > architectures that can do hardware unaligned access (x86
>> > cmpxchg8b being a notable exception).
>
>  But all those architectures have 1-byte and 2-byte memory access machine 
> instructions as well, and consequently none requires an RMW sequence to 
> update such data quantities that implies the data consistency issue that 
> we have on non-BWX Alpha.
>
>> I don't see anything that would get atomics involved in the networking
>> stack.  No READ_ONCE on packet data or anything like that.  I believe
>> that is fairly fundamental as well.  Whatever is processing a packet is
>> the only code processing that packet.
>> 
>> So I would be very surprised if the kernel needed emulation of any
>> atomics, just emulation of normal unaligned reads.  I haven't looked to
>> see if the transmission paths do things that will result in unaligned
>> writes.
>
>  The problem we have on the non-BWX Alpha target is that hardware has no 
> memory access instructions narrower than 4 bytes.  Consequently to write a 
> 1- or 2-byte quantity an RMW instruction sequence is required, in the way 
> of reading the whole 4-byte quantity, inserting the bytes to be modified, 
> and writing the whole 4-byte quantity back to memory.  However such a 
> sequence is not safe for concurrent writes, as described below.
>
>  A pair of concurrent RMW sequences targetting the same part of an aligned 
> 4-byte data quantity is not an issue: it's just an execution race and 
> software may be prepared for it (or otherwise either prevent the race via 
> a mutex or alternatively use an atomic data type along with the associated 
> accessors, which will move data locations in memory suitably apart).
>
>  The issue is a pair of concurrent RMW sequences targetting different 
> parts of the same aligned 4-byte data quantity: software can legitimately 
> expect that writes to disjoint memory locations (e.g. adjacent struct 
> members, except for bit-fields) won't affect each other.  But here where a 
> pair of such RMW sequences runs interleaved, the later write to one 
> location will clobber the value written previously to the other.  So we 
> have a data race.  Note that no atomicity is concerned here, we are 
> talking plain memory writes, such as with ordinary assignments to regular 
> variables in C code.
>
>  So I have come up with a solution where such RMW sequences are actually 
> emitted by GCC as an LDL_L/STL_C atomic access loop which ensures that no 
> intervening write has changed the aligned 4-byte data quantity containing 
> the 1- or 2-byte quantity accessed.  This guarantees consistency of the 
> part(s) of the aligned 4-byte data quantity *outside* the 1- or 2-byte 
> quantity written.  Atomicity is guaranteed by hardware as a side effect, 
> but not a part of this Alpha/Linux psABI extension (i.e. not in our 
> contract).
>
>  For known-unaligned 2-byte quantities (such as packed structure members) 
> the compiler knows that they may span 2 aligned 4-byte data quantities and 
> produces two LDL_L/STL_C loops with suitable address adjustments and data 
> masking.  This still guarantess consistency of data *outside* the 2-byte 
> quantity written.  No atomicity is guaranteed, because parts of the 2-byte 
> quantity may be stored by pieces (if the 2-byte quantity is in the middle 
> of an aligned 4-byte quantity, then it'll be written twice).
>
>  The problem is with the case where the compiler has been told to produce 
> code to write an aligned 2-byte quantity, but at run time it turns out 
> unaligned.  Now we have to emulate the LDL_L and STL_C instructions of the 
> atomic access loop or otherwise the code will crash.
>
>  My approach for this scenario is simple: LDL_L emulation remembers the 
> address accessed and data present in the 2 aligned 4-byte data quantities 
> spanned, and STL_C emulation returns failure in the case of an address 
> mismatch and otherwise uses two LDL_L/STL_C loops to load the the 2 
> aligned 4-byte data quantities by piece, compare each with data retrieved 
> previously at LDL_L emulation time, returning failure in the case of a 
> mismatch, insert the requested value and then store the resulting 
> quantity.  Again this guarantees consistency of the parts of the 2 aligned 
> 4-byte data quantities *outside* the unaligned 2-byte quantity written.  
> And again, no atomicity is guaranteed.
>
>  So while there are no atomic operations in our code at the C language 
> level, we get them sneaked in by the compiler under our feet to solve the 
> data consistency issue.  Now if we can ascertain the code paths concerned 
> won't ever exercise concurrency, we could tell the compiler not to produce 
> these atomics for 1-byte and 2-byte accesses, on a file-by-file or even 
> function-by-function basis, but it seems to me like the very maintenance 
> effort we want to avoid for a legacy platform.  Whereas if we build the 
> kernel with the atomics enabled universally, we won't have to be bothered 
> with analysing individual cases (at performance cost, but that's assumed).
>
>  I've left 8-byte data quantities out for clarity from the consideration 
> above; they're used by the compiler as suitable and handled accordingly.
>
>  Let me know if you find anything here unclear.

The emulation you are doing makes sense.

Just a few more points.  I am not current but I have never seen
concurrency (inside of a packet) at the network layer.

I don't recall ever hearing the write paths in the network stack
were ever a problem.

I suspect the write side you can verify fairly easily by simply
compiling in appletalk and opening a PF_APPLETALK socket, and sending a
message.  If that doesn't trigger emulation I can't image any other
write path in the kernel will.

Eric

  reply	other threads:[~2025-04-10  4:38 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-02-19 12:46 Maciej W. Rozycki
2025-02-19 17:38 ` Magnus Lindholm
2025-02-19 23:40 ` Linus Torvalds
2025-02-20 22:09   ` Maciej W. Rozycki
2025-04-07 20:46     ` Maciej W. Rozycki
2025-04-08  0:34       ` Linus Torvalds
2025-04-08  8:37         ` Arnd Bergmann
2025-04-09 15:52           ` Eric W. Biederman
2025-04-09 20:59             ` Maciej W. Rozycki
2025-04-10  4:37               ` Eric W. Biederman [this message]
2025-02-20 14:25 ` Magnus Lindholm
2025-02-20 16:46 ` Matt Turner
2025-02-20 16:52   ` Matt Turner
2025-02-20 19:57     ` Maciej W. Rozycki
2025-02-20 17:54 ` Richard Henderson
2025-02-20 17:59   ` Linus Torvalds
2025-02-20 18:12     ` Richard Henderson
2025-02-20 19:07   ` Matt Turner
2025-02-20 20:09     ` Maciej W. Rozycki
2025-02-20 21:05   ` Maciej W. Rozycki
2025-02-25 20:44 ` Maciej W. Rozycki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=874iywtywh.fsf@email.froward.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=arnd@arndb.de \
    --cc=glaubitz@physik.fu-berlin.de \
    --cc=ink@unseen.parts \
    --cc=linmag7@gmail.com \
    --cc=linux-alpha@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=macro@orcam.me.uk \
    --cc=mattst88@gmail.com \
    --cc=paulmck@kernel.org \
    --cc=richard.henderson@linaro.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®