[PATCH] Fix some unaligned memory accesses

[Aurelien Jarno]

Hi all,

The current SVN version currently fails to work on machines that have
strict alignment requirements, for example the SPARC target. This is due
to the way the struct fat_bpb in fs/grubfs/fat.h is defined and accessed
(through FAT_CVT_U16) to avoid padding. Some 16 bits fields like 
bytes_per_sect are thus accessed unaligned.

The patch below takes the same approach as for other structures in
openbios, it declares the structure as packed, and lets GCC do the right
things to access those unaligned field.

Regards,
Aurelien

Index: fs/grubfs/fsys_fat.c
===================================================================
--- fs/grubfs/fsys_fat.c	(révision 116)
+++ fs/grubfs/fsys_fat.c	(copie de travail)
@@ -75,23 +75,23 @@
   if (bpb.sects_per_clust == 0)
     return 0;

-  FAT_SUPER->sectsize_bits = log2 (FAT_CVT_U16 (bpb.bytes_per_sect));
+  FAT_SUPER->sectsize_bits = log2 (bpb.bytes_per_sect);
   FAT_SUPER->clustsize_bits
     = FAT_SUPER->sectsize_bits + log2 (bpb.sects_per_clust);

/* Fill in info about super block */
-  FAT_SUPER->num_sectors = FAT_CVT_U16 (bpb.short_sectors) 
-    ? FAT_CVT_U16 (bpb.short_sectors) : bpb.long_sectors;
+  FAT_SUPER->num_sectors = bpb.short_sectors 
+    ? bpb.short_sectors : bpb.long_sectors;

/* FAT offset and length */
-  FAT_SUPER->fat_offset = FAT_CVT_U16 (bpb.reserved_sects);
+  FAT_SUPER->fat_offset = bpb.reserved_sects;
   FAT_SUPER->fat_length = 
     bpb.fat_length ? bpb.fat_length : bpb.fat32_length;

/* Rootdir offset and length for FAT12/16 */
   FAT_SUPER->root_offset = 
     FAT_SUPER->fat_offset + bpb.num_fats * FAT_SUPER->fat_length;
-  FAT_SUPER->root_max = FAT_DIRENTRY_LENGTH * FAT_CVT_U16(bpb.dir_entries);
+  FAT_SUPER->root_max = FAT_DIRENTRY_LENGTH * bpb.dir_entries;

/* Data offset and number of clusters */
   FAT_SUPER->data_offset = 
@@ -105,7 +105,7 @@
   if (!bpb.fat_length)
     {
       /* This is a FAT32 */
-      if (FAT_CVT_U16(bpb.dir_entries))
+      if (bpb.dir_entries)
    return 0;

if (bpb.flags & 0x0080)
@@ -144,8 +144,8 @@

/* Now do some sanity checks */

-  if (FAT_CVT_U16(bpb.bytes_per_sect) != (1 << FAT_SUPER->sectsize_bits)
-      || FAT_CVT_U16(bpb.bytes_per_sect) != SECTOR_SIZE
+  if (bpb.bytes_per_sect != (1 << FAT_SUPER->sectsize_bits)
+      || bpb.bytes_per_sect != SECTOR_SIZE
       || bpb.sects_per_clust != (1 << (FAT_SUPER->clustsize_bits
    			       - FAT_SUPER->sectsize_bits))
       || FAT_SUPER->num_clust <= 2
Index: fs/grubfs/fat.h
===================================================================
--- fs/grubfs/fat.h	(révision 116)
+++ fs/grubfs/fat.h	(copie de travail)
@@ -37,12 +37,12 @@
    __s8	ignored[3];	/* Boot strap short or near jump */
    __s8	system_id[8];	/* Name - can be used to special case
    			   partition manager volumes */
-	__u8	bytes_per_sect[2];	/* bytes per logical sector */
+	__u16	bytes_per_sect;	/* bytes per logical sector */
    __u8	sects_per_clust;/* sectors/cluster */
-	__u8	reserved_sects[2];	/* reserved sectors */
+	__u16	reserved_sects;	/* reserved sectors */
    __u8	num_fats;	/* number of FATs */
-	__u8	dir_entries[2];	/* root directory entries */
-	__u8	short_sectors[2];	/* number of sectors */
+	__u16	dir_entries;	/* root directory entries */
+	__u16	short_sectors;	/* number of sectors */
    __u8	media;		/* media code (unused) */
    __u16	fat_length;	/* sectors/FAT */
    __u16	secs_track;	/* sectors per track */
@@ -53,15 +53,13 @@
    /* The following fields are only used by FAT32 */
    __u32	fat32_length;	/* sectors/FAT */
    __u16	flags;		/* bit 8: fat mirroring, low 4: active fat */
-	__u8	version[2];	/* major, minor filesystem version */
+	__u16	version;	/* major, minor filesystem version */
    __u32	root_cluster;	/* first cluster in root directory */
    __u16	info_sector;	/* filesystem info sector */
    __u16	backup_boot;	/* backup boot sector */
    __u16	reserved2[6];	/* Unused */
-};
+} __attribute__ ((packed));

-#define FAT_CVT_U16(bytarr) (* (__u16*)(bytarr))
-
 /*
  *  Defines how to differentiate a 12-bit and 16-bit FAT.
  */

[Stefan Reinauer]

* Aurelien Jarno  [070401 22:05]:
> Hi all,
>
> The current SVN version currently fails to work on machines that have
> strict alignment requirements, for example the SPARC target. This is due
> to the way the struct fat_bpb in fs/grubfs/fat.h is defined and accessed
> (through FAT_CVT_U16) to avoid padding. Some 16 bits fields like 
> bytes_per_sect are thus accessed unaligned.

Why is this causing trouble? bytes_per_sect was defined as an array of
u8, which you would not need to access aligned anyways. Is gcc doing
some assumptions here?
> The patch below takes the same approach as for other structures in
> openbios, it declares the structure as packed, and lets GCC do the right
> things to access those unaligned field.

Thanks, and sorry the patch is taking so long to get in. It will be
checked in in a couple of days.
> Index: fs/grubfs/fsys_fat.c
>
> --- fs/grubfs/fsys_fat.c	(révision 116)
> +++ fs/grubfs/fsys_fat.c	(copie de travail)
> @@ -75,23 +75,23 @@
>    if (bpb.sects_per_clust == 0)
>      return 0;
> FAT_SUPER->sectsize_bits = log2 (FAT_CVT_U16 (bpb.bytes_per_sect));
>
> FAT_SUPER->sectsize_bits = log2 (bpb.bytes_per_sect);
> FAT_SUPER->clustsize_bits
>   = FAT_SUPER->sectsize_bits + log2 (bpb.sects_per_clust);
>
> /* Fill in info about super block */
> FAT_SUPER->num_sectors = FAT_CVT_U16 (bpb.short_sectors)
> ? FAT_CVT_U16 (bpb.short_sectors) : bpb.long_sectors;
>
> FAT_SUPER->num_sectors = bpb.short_sectors
>
> ? bpb.short_sectors : bpb.long_sectors;
>
> /* FAT offset and length */
> FAT_SUPER->fat_offset = FAT_CVT_U16 (bpb.reserved_sects);
>
> FAT_SUPER->fat_offset = bpb.reserved_sects;
> FAT_SUPER->fat_length = 
>   bpb.fat_length ? bpb.fat_length : bpb.fat32_length;
>
> /* Rootdir offset and length for FAT12/16 */
> FAT_SUPER->root_offset = 
>   FAT_SUPER->fat_offset + bpb.num_fats * FAT_SUPER->fat_length;
> FAT_SUPER->root_max = FAT_DIRENTRY_LENGTH * FAT_CVT_U16(bpb.dir_entries);
>
> FAT_SUPER->root_max = FAT_DIRENTRY_LENGTH * bpb.dir_entries;
>
> /* Data offset and number of clusters */
> FAT_SUPER->data_offset =
>
> @@ -105,7 +105,7 @@
>    if (!bpb.fat_length)
>      {
>        /* This is a FAT32 */
>  if (FAT_CVT_U16(bpb.dir_entries))
>  if (bpb.dir_entries)
>
> return 0;
>  if (bpb.flags & 0x0080)
>
> @@ -144,8 +144,8 @@
>
> /* Now do some sanity checks */
> if (FAT_CVT_U16(bpb.bytes_per_sect) != (1 << FAT_SUPER->sectsize_bits)
>  || FAT_CVT_U16(bpb.bytes_per_sect) != SECTOR_SIZE
> if (bpb.bytes_per_sect != (1 << FAT_SUPER->sectsize_bits)
>  || bpb.bytes_per_sect != SECTOR_SIZE
>  || bpb.sects_per_clust != (1 << (FAT_SUPER->clustsize_bits
> 	       - FAT_SUPER->sectsize_bits))
>  || FAT_SUPER->num_clust <= 2
> Index: fs/grubfs/fat.h
>
> --- fs/grubfs/fat.h	(révision 116)
> +++ fs/grubfs/fat.h	(copie de travail)
> @@ -37,12 +37,12 @@
>    __s8	ignored[3];	/* Boot strap short or near jump */
>    __s8	system_id[8];	/* Name - can be used to special case
>    			   partition manager volumes */
> __u8	bytes_per_sect[2];	/* bytes per logical sector */
> __u16	bytes_per_sect;	/* bytes per logical sector */
> __u8	sects_per_clust;/* sectors/cluster */
> __u8	reserved_sects[2];	/* reserved sectors */
> __u16	reserved_sects;	/* reserved sectors */
> __u8	num_fats;	/* number of FATs */
> __u8	dir_entries[2];	/* root directory entries */
> __u8	short_sectors[2];	/* number of sectors */
> __u16	dir_entries;	/* root directory entries */
> __u16	short_sectors;	/* number of sectors */
> __u8	media;		/* media code (unused) */
> __u16	fat_length;	/* sectors/FAT */
> __u16	secs_track;	/* sectors per track */
>
> @@ -53,15 +53,13 @@
>    /* The following fields are only used by FAT32 */
>    __u32	fat32_length;	/* sectors/FAT */
>    __u16	flags;		/* bit 8: fat mirroring, low 4: active fat */
> __u8	version[2];	/* major, minor filesystem version */
> __u16	version;	/* major, minor filesystem version */
> __u32	root_cluster;	/* first cluster in root directory */
> __u16	info_sector;	/* filesystem info sector */
> __u16	backup_boot;	/* backup boot sector */
> __u16	reserved2[6];	/* Unused */
>
> -};
> +} __attribute__ ((packed));
>
> -#define FAT_CVT_U16(bytarr) (* (__u16*)(bytarr))
>
> /*
> Defines how to differentiate a 12-bit and 16-bit FAT.
>
> */

[Paul Brook]

On Sunday 15 April 2007 01:23, Stefan Reinauer wrote:
> Aurelien Jarno  [070401 22:05]:
> > Hi all,
> >
> > The current SVN version currently fails to work on machines that have
> > strict alignment requirements, for example the SPARC target. This is due
> > to the way the struct fat_bpb in fs/grubfs/fat.h is defined and accessed
> > (through FAT_CVT_U16) to avoid padding. Some 16 bits fields like
> > bytes_per_sect are thus accessed unaligned.
>
> Why is this causing trouble? bytes_per_sect was defined as an array of
> u8, which you would not need to access aligned anyways. Is gcc doing
> some assumptions here?

It was accessed through a (uint16_t *) though.

gcc only generates special code to handle misaligned accesses when the access 
if a member of an explicitly packed (ie. misaligned) structure.

Because compensating for a misaligned access is so expensive the only sane 
option is to assume that normal pointers are properly aligned.

By taking the address and casting to (uint16_t *) you're hiding the member 
access, and allowing gcc to assume it is a properly aligned 16-bit value.

Paul

[Stefan Reinauer]

* Paul Brook  [070415 02:38]:
> It was accessed through a (uint16_t *) though.

Ooops. I didn't check FAT_CVT_U16 and assumed it would read bytes.
> By taking the address and casting to (uint16_t *) you're hiding the member 
> access, and allowing gcc to assume it is a properly aligned 16-bit value.

Ok. No further tests needed to see the issue. Just checked it in.

Stefan