linux-kernel - Re: [PATCH] hpsa: fix boot on ia64 (atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a3JYmhbN3TXB2cWGfXGKgsUa9Hg=ZvWckTaL_31OMgbtQ@mail.gmail.com>
Date:   Tue, 16 Mar 2021 19:28:22 +0100
From:   Arnd Bergmann <arnd@...nel.org>
To:     Don.Brace@...rochip.com
Cc:     Sergei Trofimovich <slyfox@...too.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-ia64@...r.kernel.org, storagedev@...rochip.com,
        linux-scsi <linux-scsi@...r.kernel.org>, jszczype@...hat.com,
        Scott.Benesh@...rochip.com, Scott.Teel@...rochip.com,
        thenzl@...hat.com,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
Subject: Re: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

On Tue, Mar 16, 2021 at 6:12 PM <Don.Brace@...rochip.com> wrote:

>  drivers/scsi/hpsa_cmd.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h index d126bb877250..617bdae9a7de 100644
> --- a/drivers/scsi/hpsa_cmd.h
> +++ b/drivers/scsi/hpsa_cmd.h
> @@ -20,6 +20,9 @@
>  #ifndef HPSA_CMD_H
>  #define HPSA_CMD_H
>
> +#include <linux/build_bug.h> /* static_assert */ #include
> +<linux/stddef.h> /* offsetof */
> +
>  /* general boundary defintions */
>  #define SENSEINFOBYTES          32 /* may vary between hbas */
>  #define SG_ENTRIES_IN_CMD      32 /* Max SG entries excluding chain blocks */
> @@ -448,11 +451,20 @@ struct CommandList {
>          */
>         struct hpsa_scsi_dev_t *phys_disk;
>
> -       bool retry_pending;
> +       int retry_pending;
>         struct hpsa_scsi_dev_t *device;
>         atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */  } __aligned(COMMANDLIST_ALIGNMENT);
>
> +/*
> + * Make sure our embedded atomic variable is aligned. Otherwise we
> +break atomic
> + * operations on architectures that don't support unaligned atomics like IA64.
> + *
> + * Ideally this header should be cleaned up to only mark individual
> +structs as
> + * packed.
> + */
> +static_assert(offsetof(struct CommandList, refcount) %
> +__alignof__(atomic_t) == 0);
> +

Actually that still feels wrong: the annotation of the struct is to pack
every member, which causes the access to be done in byte units
on architectures that do not have hardware unaligned load/store
instructions, at least for things like atomic_read() that does not
go through a cmpxchg() or ll/sc cycle.

This change may fix itanium, but it's still not correct. Other
architectures would have already been broken before the recent
change, but that's not a reason against fixing them now.

I'd recommend marking the entire structure as having default
alignment, by adding the appropriate pragmas before and after it.

         Arnd