hio: SSD data corruption under stress test

[Kamal Mostafa]

{forward from James Troup}:

Just to followup to this with a little more information, we have now
reproduced this in the following scenarios:

 * Ubuntu kernel 4.4 (i.e. 16.04) and kernel 4.8 (i.e. HWE-Y)
 * With and without Bcache involved
 * With both XFS and ext4
 * With HIO driver versions 2.1.0-23 and 2.1.0-25
 * With HIO Firmware 640 and 650
 * With and without the following two patches
  - https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/xenial/commit/?id=7290fa97b945c288d8dd8eb8f284b98cb495b35b
  - https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/xenial/commit/?id=901a3142db778ddb9ed6a9000ce8e5b0f66c48ba

In all cases, we applied the following two patches in order to get hio
to build at all with a 4.4 or later kernel:

  https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/xenial/commit/?id=0abbb90372847caeeedeaa9db0f21e05ad8e9c74
  https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/xenial/commit/?id=a0705c5ff3d12fc31f18f5d3c8589eaaed1aa577

We've confirmed that we can reproduce the corruption on any machine in
Tele2's Vienna facility.

We've confirmed that, other than 1 machine, the 'hio_info' command
says the health is 'OK'.

Our most common reproducer is one of two scenarios:

 a) http://paste.ubuntu.com/23405150/

 b) http://paste.ubuntu.com/23405234/

In the last example, it's possible to see corruption faster by
increasing the 'count' argument to dd and avoid it by lowering it.
e.g. on the machine I'm currently testing on count=52450 doesn't
appear to show corruption, but a count of even 53000 would show it
immediately every time.

I hope this helps - please let us know what further information we can
provide to debug this problem.

[Kamal Mostafa (kamalmostafa)]

Huawei's upstream hio driver version 2.1.0.26 introduces two changes relative to the current Ubuntu version:

#1. bio_endio() shim macro sets bi_error field {{ but note bug in implementation! }}
#2. blk_queue_split() call inserted ssd_make_request()

James' initial test results indicate that #2 appears to fix the data corruption. Both fixes should be applied (though #1 needs correction) after sufficient testing confirmation.

[James Troup (elmo)]

Sorry - for the avoidance of doubt, neither of these changes are in 2.1.0.26 - 2.1.0.26 doesn't even include your patches. I was testing 2.1.0.26 + our patches + the 2 changes you mention. #1 does come from Huawei, #2 comes from Ming Lei of Canonical.

[James Troup (elmo)]

(Also 2.1.0.26 is not public yet for reasons Huawei are trying to figure out, 2.1.0.25 is the last publicly available driver.)

[Kamal Mostafa (kamalmostafa)]

Side-note about bug in hio driver 2.1.0.26 (#1: bio_endio() shim macro sets bi_error field). This turns out NOT the cause of this data corruption bug per James' testing, but should be corrected (both in the upstream driver and Ubuntu).

The idea behind this change is correct (the macro shim should indeed set bi_error) but the implementation is unsafe:

 #if (LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0))
-#define bio_endio(bio, errors) bio_endio(bio)
+#define bio_endio(bio, errors) \
+ bio->bi_error = errors; \
+ bio_endio(bio)
 #endif

Instead, the macro content must resolve to a single expression, e.g.:

+#define bio_endio(bio, errors) \
+ do { bio->bi_error = errors; bio_endio(bio); } while (0)

[Kamal Mostafa (kamalmostafa)]

James, thanks for clarifying that. In that case, all my references to "hio driver 2.1.0.26" should actually say "the patch set James is testing", and the two fixes should still likely both be applied to the Ubuntu hio driver.

[Kamal Mostafa (kamalmostafa)]

Ming Lei comment #2 says you're the author of this patch to the hio driver:

+#if (LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0))
+ blk_queue_split(q, &bio, q->bio_split);
+#endif
+

Can you provide us with a short explanation for the git log, and also your Signed-off-by line for that patch?

[Kamal Mostafa (kamalmostafa)]

On Wed, Nov 2, 2016 at 2:08 PM, James Troup <email address hidden> wrote:

> I've confirmed the corruption is gone with the two fixes above on both 4.4 and 4.8.

[Ming Lei (tom-leiming)]

On Thu, Nov 3, 2016 at 5:42 AM, Kamal Mostafa <email address hidden> wrote:
> Ming Lei comment #2 says you're the author of this patch to the hio
> driver:
>
> +#if (LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0))
> + blk_queue_split(q, &bio, q->bio_split);
> +#endif
> +
>
> Can you provide us with a short explanation for the git log, and also
> your Signed-off-by line for that patch?

Sure, please see the attachment.

>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1638700
>
> Title:
> hio: SSD data corruption under stress test
>
> Status in linux package in Ubuntu:
> In Progress
> Status in linux source package in Xenial:
> In Progress
> Status in linux source package in Yakkety:
> In Progress
> Status in linux source package in Zesty:
> In Progress
>
> Bug description:
> {forward from James Troup}:
>
> Just to followup to this with a little more information, we have now
> reproduced this in the following scenarios:
>
> * Ubuntu kernel 4.4 (i.e. 16.04) and kernel 4.8 (i.e. HWE-Y)
> * With and without Bcache involved
> * With both XFS and ext4
> * With HIO driver versions 2.1.0-23 and 2.1.0-25
> * With HIO Firmware 640 and 650
> * With and without the following two patches
> - https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/xenial/commit/?id=7290fa97b945c288d8dd8eb8f284b98cb495b35b
> - https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/xenial/commit/?id=901a3142db778ddb9ed6a9000ce8e5b0f66c48ba
>
> In all cases, we applied the following two patches in order to get hio
> to build at all with a 4.4 or later kernel:
>
> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/xenial/commit/?id=0abbb90372847caeeedeaa9db0f21e05ad8e9c74
> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/xenial/commit/?id=a0705c5ff3d12fc31f18f5d3c8589eaaed1aa577
>
> We've confirmed that we can reproduce the corruption on any machine in
> Tele2's Vienna facility.
>
> We've confirmed that, other than 1 machine, the 'hio_info' command
> says the health is 'OK'.
>
> Our most common reproducer is one of two scenarios:
>
> a) http://paste.ubuntu.com/23405150/
>
> b) http://paste.ubuntu.com/23405234/
>
> In the last example, it's possible to see corruption faster by
> increasing the 'count' argument to dd and avoid it by lowering it.
> e.g. on the machine I'm currently testing on count=52450 doesn't
> appear to show corruption, but a count of even 53000 would show it
> immediately every time.
>
> I hope this helps - please let us know what further information we can
> provide to debug this problem.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1638700/+subscriptions

[Kamal Mostafa (kamalmostafa)]

https://lists.ubuntu.com/archives/kernel-team/2016-November/080705.html

[Luis Henriques (henrix)]

This bug is awaiting verification that the kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-xenial' to 'verification-done-xenial'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

[Luis Henriques (henrix)]

This bug is awaiting verification that the kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-yakkety' to 'verification-done-yakkety'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

[James Troup (elmo)]

Corruption no longer visible with 4.4.0-49-generic:

  https://pastebin.canonical.com/171039/

Testing with Yakkety proper would be messy - what's the timeline for
getting an updated 4.8 kernel into Xenial? I currently only see
4.8.0-27.29~16.04.1.

[JuanJo Ciarlante (jjo)]

FTR/FYI (as per chatter w/kamal) we're waiting for >= 4.8.0-28
to be available at https://launchpad.net/ubuntu/+source/linux-hwe-edge

[Steve Langasek (vorlon)]

The verification of the Stable Release Update for linux-lts-xenial has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package linux - 4.4.0-51.72

---------------
linux (4.4.0-51.72) xenial; urgency=low

  [ Luis Henriques ]

  * Release Tracking Bug
    - LP: #1644611

  * 4.4.0-1037-snapdragon #41: kernel panic on boot (LP: #1644596)
    - Revert "dma-mapping: introduce the DMA_ATTR_NO_WARN attribute"
    - Revert "powerpc: implement the DMA_ATTR_NO_WARN attribute"
    - Revert "nvme: use the DMA_ATTR_NO_WARN attribute"

linux (4.4.0-50.71) xenial; urgency=low

  [ Luis Henriques ]

  * Release Tracking Bug
    - LP: #1644169

  * xenial 4.4.0-49.70 kernel breaks LXD userspace (LP: #1644165)
    - Revert "UBUNTU: SAUCE: (namespace) fuse: Allow user namespace mounts by
      default"
    - Revert "UBUNTU: SAUCE: (namespace) fs: Don't remove suid for CAP_FSETID for
      userns root"
    - Revert "(namespace) Revert "UBUNTU: SAUCE: fs: Don't remove suid for
      CAP_FSETID in s_user_ns""
    - Revert "UBUNTU: SAUCE: (namespace) fs: Allow superblock owner to change
      ownership of inodes"
    - Revert "(namespace) Revert "UBUNTU: SAUCE: fs: Allow superblock owner to
      change ownership of inodes with unmappable ids""
    - Revert "UBUNTU: SAUCE: (namespace) security/integrity: Harden against
      malformed xattrs"
    - Revert "(namespace) Revert "UBUNTU: SAUCE: ima/evm: Allow root in s_user_ns
      to set xattrs""
    - Revert "(namespace) dquot: For now explicitly don't support filesystems
      outside of init_user_ns"
    - Revert "(namespace) quota: Handle quota data stored in s_user_ns in
      quota_setxquota"
    - Revert "(namespace) quota: Ensure qids map to the filesystem"
    - Revert "(namespace) Revert "UBUNTU: SAUCE: quota: Convert ids relative to
      s_user_ns""
    - Revert "(namespace) Revert "UBUNTU: SAUCE: quota: Require that qids passed
      to dqget() be valid and map into s_user_ns""
    - Revert "(namespace) vfs: Don't create inodes with a uid or gid unknown to
      the vfs"
    - Revert "(namespace) vfs: Don't modify inodes with a uid or gid unknown to
      the vfs"
    - Revert "UBUNTU: SAUCE: (namespace) fuse: Translate ids in posix acl xattrs"
    - Revert "UBUNTU: SAUCE: (namespace) posix_acl: Export
      posix_acl_fix_xattr_userns() to modules"
    - Revert "(namespace) vfs: Verify acls are valid within superblock's
      s_user_ns."
    - Revert "(namespace) Revert "UBUNTU: SAUCE: fs: Update posix_acl support to
      handle user namespace mounts""
    - Revert "(namespace) fs: Refuse uid/gid changes which don't map into
      s_user_ns"
    - Revert "(namespace) Revert "UBUNTU: SAUCE: fs: Refuse uid/gid changes which
      don't map into s_user_ns""
    - Revert "(namespace) mnt: Move the FS_USERNS_MOUNT check into sget_userns"

linux (4.4.0-49.70) xenial; urgency=low

  [ Luis Henriques ]

  * Release Tracking Bug
    - LP: #1640921

  * Infiniband driver (kernel module) needed for Azure (LP: #1641139)
    - SAUCE: RDMA Infiniband for Windows Azure
    - [Config] CONFIG_HYPERV_INFINIBAND_ND=m
    - SAUCE: Makefile RDMA infiniband driver for Windows Azure
    - [Config] Add hv_network_direct.ko to generic inclusion list
    - SAUCE: RDMA Infiniband for Windows Azure is dependent on amd64...

[Launchpad Janitor (janitor)]

This bug was fixed in the package linux - 4.8.0-28.30

---------------
linux (4.8.0-28.30) yakkety; urgency=low

  [ Luis Henriques ]

  * Release Tracking Bug
    - LP: #1641083

  * lxc-attach to malicious container allows access to host (LP: #1639345)
    - Revert "UBUNTU: SAUCE: (noup) ptrace: being capable wrt a process requires
      mapped uids/gids"
    - (upstream) mm: Add a user_ns owner to mm_struct and fix ptrace permission
      checks

  * [Feature] AVX-512 new instruction sets (avx512_4vnniw, avx512_4fmaps)
    (LP: #1637526)
    - x86/cpufeature: Add AVX512_4VNNIW and AVX512_4FMAPS features

  * zfs: importing zpool with vdev on zvol hangs kernel (LP: #1636517)
    - SAUCE: (noup) Update zfs to 0.6.5.8-0ubuntu4.1

  * Move some device drivers build from kernel built-in to modules
    (LP: #1637303)
    - [Config] CONFIG_TIGON3=m for all arches
    - [Config] CONFIG_VIRTIO_BLK=m, CONFIG_VIRTIO_NET=m

  * I2C touchpad does not work on AMD platform (LP: #1612006)
    - pinctrl/amd: Configure GPIO register using BIOS settings

  * guest experiencing Transmit Timeouts on CX4 (LP: #1636330)
    - powerpc/64: Re-fix race condition between going idle and entering guest
    - powerpc/64: Fix race condition in setting lock bit in idle/wakeup code

  * QEMU throws failure msg while booting guest with SRIOV VF (LP: #1630554)
    - KVM: PPC: Always select KVM_VFIO, plus Makefile cleanup

  * [Feature] KBL - New device ID for Kabypoint(KbP) (LP: #1591618)
    - SAUCE: mfd: lpss: Fix Intel Kaby Lake PCH-H properties

  * hio: SSD data corruption under stress test (LP: #1638700)
    - SAUCE: hio: set bi_error field to signal an I/O error on a BIO
    - SAUCE: hio: splitting bio in the entry of .make_request_fn

  * cleanup primary tree for linux-hwe layering issues (LP: #1637473)
    - [Config] switch Vcs-Git: to yakkety repository
    - [Packaging] handle both linux-lts* and linux-hwe* as backports
    - [Config] linux-tools-common and linux-cloud-tools-common are one per series
    - [Config] linux-source-* is in the primary linux namespace
    - [Config] linux-tools -- always suggest the base package

  * SRU: sync zfsutils-linux and spl-linux changes to linux (LP: #1635656)
    - SAUCE: (noup) Update spl to 0.6.5.8-2, zfs to 0.6.5.8-0ubuntu4 (LP:
      #1635656)

  * [Feature] SKX: perf uncore PMU support (LP: #1591810)
    - perf/x86/intel/uncore: Add Skylake server uncore support
    - perf/x86/intel/uncore: Remove hard-coded implementation for Node ID mapping
      location
    - perf/x86/intel/uncore: Handle non-standard counter offset

  * [Feature] Purley: Memory Protection Keys (LP: #1591804)
    - x86/pkeys: Add fault handling for PF_PK page fault bit
    - mm: Implement new pkey_mprotect() system call
    - x86/pkeys: Make mprotect_key() mask off additional vm_flags
    - x86/pkeys: Allocation/free syscalls
    - x86: Wire up protection keys system calls
    - generic syscalls: Wire up memory protection keys syscalls
    - pkeys: Add details of system call use to Documentation/
    - x86/pkeys: Default to a restrictive init PKRU
    - x86/pkeys: Allow configuration of init_pkru
    - x86/pkeys: Add self-tests

  * kernel invalid ...

[Launchpad Janitor (janitor)]

This bug was fixed in the package linux - 4.8.0-30.32

---------------
linux (4.8.0-30.32) yakkety; urgency=low

  * CVE-2016-8655 (LP: #1646318)
    - packet: fix race condition in packet_set_ring

 -- Brad Figg <email address hidden> Thu, 01 Dec 2016 08:02:53 -0800