[armel] non-ISO-C misaligned pointer punning causes slowness and SIGILLs

Bug #494667 reported by Dave Martin on 2009-12-09
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
squashfs (Ubuntu)
Critical
Unassigned
Lucid
Critical
Unassigned
squashfs-tools (Ubuntu)
Critical
Emmet Hikory
Lucid
Critical
Emmet Hikory

Bug Description

Binary package hint: squashfs-tools

mksquashfs.c currently accesses the following structures through misaligned pointers: at least the following:

squashfs_base_inode_header *
squashfs_dir_entry *
squashfs_dir_header *
squashfs_dir_inode_header *
squashfs_ldir_inode_header *
squashfs_reg_inode_header *
squashfs_symlink_inode_header *
unsigned short *

I haven't checked the rest of this package, but there are likely to be issues elsewhere as well.

Not all memory access instructions can access at unaligned addresses on ARM; GCC assumes that pointers are properly aligned for their type because attempts by a C program to access an lvalue via a pointer which is not properly aligned for the lvalue's type are not permitted under strict interpretation of the C language specification.

Particularly when doing block transfers of whole structures (*obj_p_dest = *obj_p_src) and equivalent operations, LDM or STM instructions are emitted by the compiler: these _always_ fault into the kernel if the base address is unaligned, and can cause a significant performance hit.

In addition, prior to Linux 2.6.31, the kernel cannot successfully emulate all misaligned LDM or STM instructions for Thumb ... leading to the SIGILLs which have been observed on the lucid buildds (2.6.28 kernel) when moving to v7.

Possible workarounds:
  * Build squashfs-tools with -marm
  * fix squashfs-tools to use strict C only (laborious... either build squashfs filesystem structures at true alignment and then copy them into place in the output, or
  * declare all the affected types with __attribute__ (( __packed__ )) so GCC knows the structures may be accessed at misaligned locations (care must be taken to make sure the structures' sizes and internal arrangement do not change as a result of this, and the resulting code may run significantly slower --- I've not checked for the implications of this. Some other check is needed for unsigned short, or it must be wrapped in a structure or union type, because GCC cannot tag basic types with the __packed__ attribute)
  * apply the appropriate LDM/STM emulation patches to the kernel used on the buildds (if feasible --- I'll try and feed back on this)

description: updated
Dave Martin (dave-martin-arm) wrote :

I tried experimentally buiding this package for -marm, and this seems to solve the SIGILLs on 2.6.28 kernels.

Running mksquash and unsquash on /etc now works without SIGILL, for example.

Alexander Sack (asac) wrote :

this seems to happen with 2.6.28 kernel, workaround until we have our builders on newer kernel is to build with -marm.

Assigning to David to prepare the debdiff for sponsoring.

Changed in squashfs (Ubuntu):
assignee: nobody → David Sugar (dyfet)
importance: Undecided → Critical
milestone: none → lucid-alpha-2
status: New → Triaged

This adds a conditional in the Makefile to use -marm when DEB_BUILD_ARCH_CPU is arm, which does for arm as suggested by David Martin. The lucid version of squashfs was still debian upstream. I am not sure if we wish to push this back to debian at all, or even if we later want to address this in a better way, so I assume we are for now keeping as a delta, at least for present production needs.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package squashfs - 1:4.0-3ubuntu1

---------------
squashfs (1:4.0-3ubuntu1) lucid; urgency=high

  * Temporary conditional -marm fix for arm7 gcc build issue (LP: #494667),
    required for current lucid production.
 -- David Sugar <email address hidden> Fri, 11 Dec 2009 14:44:00 -0500

Changed in squashfs (Ubuntu Lucid):
status: Triaged → Fix Released

The debdiff is invalid, build architecture for ARM is armel, not arm, which will fail to pass -marm on the resulting binaries.

Changed in squashfs (Ubuntu Lucid):
status: Fix Released → In Progress
Loïc Minier (lool) wrote :

Michael, the debdiff uses _ARCH_CPU which is arm on armel; the actual issue is using _BUILD instead of _HOST.

Loïc Minier (lool) wrote :

-marm is passed when building natively and I see it in the build log

Daniel Holbach (dholbach) wrote :

Unsubscribing ubuntu-core-dev.

Dave Martin (dave-martin-arm) wrote :

> This adds a conditional in the Makefile to use -marm when
> DEB_BUILD_ARCH_CPU is arm, which does for arm as suggested by
> David Martin. The lucid version of squashfs was still debian
> upstream. I am not sure if we wish to push this back to
> debian at all, or even if we later want to address this in a
> better way, so I assume we are for now keeping as a delta, at
> least for present production needs.

I think it makes sense to keep it as a delta--- the need for it should go away in the future once the kernels for all platforms are sufficiently new.

Loïc Minier (lool) wrote :

Nothing to sponsor right now, unsub-ing main-sponsors

Emmet Hikory (persia) wrote :

The attached patch applies a substantially similar fix to the squashfs-tools package (using -marm for arm)

Changed in squashfs-tools (Ubuntu Lucid):
status: New → Triaged
importance: Undecided → Critical
assignee: nobody → Emmet Hikory (persia)
milestone: none → lucid-alpha-2
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package squashfs-tools - 1:4.0-6ubuntu1

---------------
squashfs-tools (1:4.0-6ubuntu1) lucid; urgency=low

  * Build with -marm to avoid using Thumb2 instructions
      (Older kernels don't support these instructions: LP: #494667)
 -- Emmet Hikory <email address hidden> Thu, 17 Dec 2009 23:04:46 +0900

Changed in squashfs-tools (Ubuntu Lucid):
status: Triaged → Fix Released
Alexander Sack (asac) wrote :

was fix released for squashfs source ... the -marm flag was confirmed to work afaict.

Changed in squashfs (Ubuntu Lucid):
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers