[PATCH] block: use 32-bit blk_status_t on Alpha

[PATCH] block: use 32-bit blk_status_t on Alpha

[Mikulas Patocka]

Early alpha processors cannot write a single byte or word; they read 8
bytes, modify the value in registers and write back 8 bytes.

The type blk_status_t is defined as one byte, it is often written
asynchronously by I/O completion routines, this asynchronous modification
can corrupt content of nearby bytes if these nearby bytes can be written
simultaneously by another CPU.

- one example of such corruption is the structure dm_io where
  "blk_status_t status" is written by an asynchronous completion routine
  and "atomic_t io_count" is modified synchronously
- another example is the structure dm_buffer where "unsigned hold_count"
  is modified synchronously from process context and "blk_status_t
  write_error" is modified asynchronously from bio completion routine

This patch fixes the bug by changing the type blk_status_t to 32 bits if
we are on Alpha and if we are compiling for a processor that doesn't have
the byte-word-extension.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org	# 4.13+

---
 include/linux/blk_types.h |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux-2.6/include/linux/blk_types.h
===================================================================
--- linux-2.6.orig/include/linux/blk_types.h	2018-02-14 20:24:42.038255000 +0100
+++ linux-2.6/include/linux/blk_types.h	2018-03-21 15:04:54.969999000 +0100
@@ -20,8 +20,13 @@ typedef void (bio_end_io_t) (struct bio
 
 /*
  * Block error status values.  See block/blk-core:blk_errors for the details.
+ * Alpha cannot write a byte atomically, so we need to use 32-bit value.
  */
+#if defined(CONFIG_ALPHA) && !defined(__alpha_bwx__)
+typedef u32 __bitwise blk_status_t;
+#else
 typedef u8 __bitwise blk_status_t;
+#endif
 #define	BLK_STS_OK 0
 #define BLK_STS_NOTSUPP		((__force blk_status_t)1)
 #define BLK_STS_TIMEOUT		((__force blk_status_t)2)

Re: [PATCH] block: use 32-bit blk_status_t on Alpha

[Jens Axboe]

On 3/21/18 10:42 AM, Mikulas Patocka wrote:
> Early alpha processors cannot write a single byte or word; they read 8
> bytes, modify the value in registers and write back 8 bytes.
> 
> The type blk_status_t is defined as one byte, it is often written
> asynchronously by I/O completion routines, this asynchronous modification
> can corrupt content of nearby bytes if these nearby bytes can be written
> simultaneously by another CPU.
> 
> - one example of such corruption is the structure dm_io where
>   "blk_status_t status" is written by an asynchronous completion routine
>   and "atomic_t io_count" is modified synchronously
> - another example is the structure dm_buffer where "unsigned hold_count"
>   is modified synchronously from process context and "blk_status_t
>   write_error" is modified asynchronously from bio completion routine
> 
> This patch fixes the bug by changing the type blk_status_t to 32 bits if
> we are on Alpha and if we are compiling for a processor that doesn't have
> the byte-word-extension.

That's nasty. Is alpha the only problematic arch here?

As to the patch in question, normally I'd just say we should make it
unconditionally u32. But we pack so nicely in the bio, and I don't think
the bio itself has this issue as the rest of the members that share this
word are all set before the bio is submitted. But callers embedding
the status var in other structures don't necessarily have that
guarantee, as your dm examples show.

-- 
Jens Axboe

Re: [PATCH] block: use 32-bit blk_status_t on Alpha

[Mikulas Patocka]

On Wed, 21 Mar 2018, Jens Axboe wrote:

> On 3/21/18 10:42 AM, Mikulas Patocka wrote:
> > Early alpha processors cannot write a single byte or word; they read 8
> > bytes, modify the value in registers and write back 8 bytes.
> > 
> > The type blk_status_t is defined as one byte, it is often written
> > asynchronously by I/O completion routines, this asynchronous modification
> > can corrupt content of nearby bytes if these nearby bytes can be written
> > simultaneously by another CPU.
> > 
> > - one example of such corruption is the structure dm_io where
> >   "blk_status_t status" is written by an asynchronous completion routine
> >   and "atomic_t io_count" is modified synchronously
> > - another example is the structure dm_buffer where "unsigned hold_count"
> >   is modified synchronously from process context and "blk_status_t
> >   write_error" is modified asynchronously from bio completion routine
> > 
> > This patch fixes the bug by changing the type blk_status_t to 32 bits if
> > we are on Alpha and if we are compiling for a processor that doesn't have
> > the byte-word-extension.
> 
> That's nasty. Is alpha the only problematic arch here?

Yes. All the other architectures supported by Linux have byte writes.

> As to the patch in question, normally I'd just say we should make it
> unconditionally u32. But we pack so nicely in the bio, and I don't think
> the bio itself has this issue as the rest of the members that share this
> word are all set before the bio is submitted. But callers embedding
> the status var in other structures don't necessarily have that
> guarantee, as your dm examples show.
> 
> -- 
> Jens Axboe

Keeping blk_status_t 8-bit for most architectures will save a few bytes in 
some of device mapper structures.

Mikulas

Re: [PATCH] block: use 32-bit blk_status_t on Alpha

[Mikulas Patocka]

On Wed, 21 Mar 2018, Jens Axboe wrote:

> On 3/21/18 10:42 AM, Mikulas Patocka wrote:
> > Early alpha processors cannot write a single byte or word; they read 8
> > bytes, modify the value in registers and write back 8 bytes.
> > 
> > The type blk_status_t is defined as one byte, it is often written
> > asynchronously by I/O completion routines, this asynchronous modification
> > can corrupt content of nearby bytes if these nearby bytes can be written
> > simultaneously by another CPU.
> > 
> > - one example of such corruption is the structure dm_io where
> >   "blk_status_t status" is written by an asynchronous completion routine
> >   and "atomic_t io_count" is modified synchronously
> > - another example is the structure dm_buffer where "unsigned hold_count"
> >   is modified synchronously from process context and "blk_status_t
> >   write_error" is modified asynchronously from bio completion routine
> > 
> > This patch fixes the bug by changing the type blk_status_t to 32 bits if
> > we are on Alpha and if we are compiling for a processor that doesn't have
> > the byte-word-extension.
> 
> That's nasty. Is alpha the only problematic arch here?

Yes. All the other architectures supported by Linux have byte writes.

> As to the patch in question, normally I'd just say we should make it
> unconditionally u32. But we pack so nicely in the bio, and I don't think
> the bio itself has this issue as the rest of the members that share this
> word are all set before the bio is submitted. But callers embedding
> the status var in other structures don't necessarily have that
> guarantee, as your dm examples show.
> 
> -- 
> Jens Axboe

Keeping blk_status_t 8-bit for most architectures will save a few bytes in 
some of device mapper structures.

Mikulas

Re: [PATCH] block: use 32-bit blk_status_t on Alpha

[Jens Axboe]

On 3/21/18 11:00 AM, Mikulas Patocka wrote:
> 
> 
> On Wed, 21 Mar 2018, Jens Axboe wrote:
> 
>> On 3/21/18 10:42 AM, Mikulas Patocka wrote:
>>> Early alpha processors cannot write a single byte or word; they read 8
>>> bytes, modify the value in registers and write back 8 bytes.
>>>
>>> The type blk_status_t is defined as one byte, it is often written
>>> asynchronously by I/O completion routines, this asynchronous modification
>>> can corrupt content of nearby bytes if these nearby bytes can be written
>>> simultaneously by another CPU.
>>>
>>> - one example of such corruption is the structure dm_io where
>>>   "blk_status_t status" is written by an asynchronous completion routine
>>>   and "atomic_t io_count" is modified synchronously
>>> - another example is the structure dm_buffer where "unsigned hold_count"
>>>   is modified synchronously from process context and "blk_status_t
>>>   write_error" is modified asynchronously from bio completion routine
>>>
>>> This patch fixes the bug by changing the type blk_status_t to 32 bits if
>>> we are on Alpha and if we are compiling for a processor that doesn't have
>>> the byte-word-extension.
>>
>> That's nasty. Is alpha the only problematic arch here?
> 
> Yes. All the other architectures supported by Linux have byte writes.
> 
>> As to the patch in question, normally I'd just say we should make it
>> unconditionally u32. But we pack so nicely in the bio, and I don't think
>> the bio itself has this issue as the rest of the members that share this
>> word are all set before the bio is submitted. But callers embedding
>> the status var in other structures don't necessarily have that
>> guarantee, as your dm examples show.
>>
>> -- 
>> Jens Axboe
> 
> Keeping blk_status_t 8-bit for most architectures will save a few bytes in 
> some of device mapper structures.

And more importantly, it won't screw up the bio layout, I'm somewhat more
concerned about that than random driver structures.

If alpha is the odd one out here, then I think your patch is fine as-is.

-- 
Jens Axboe