Re: [PATCH] scsi: qla2xxx: Remove problematic BUILD_BUG_ON() assertion

[Geert Uytterhoeven]

Hi Finn,

On Fri, 6 Mar 2026 at 00:03, Finn Thain <fthain@xxxxxxxxxxxxxx> wrote:
> The LKP bot reported a build failure with CONFIG_COLDFIRE=y together with
> CONFIG_SCSI_QLA_FC=y, that's attributable to the BUILD_BUG_ON() in
> qlt_queue_unknown_atio().
>
> That function uses kzalloc() to obtain memory for the following struct,
> plus some extra bytes at the end.
>
> struct qla_tgt_sess_op {
>         struct scsi_qla_host *vha;
>         uint32_t chip_reset;
>         struct work_struct work;
>         struct list_head cmd_list;
>         bool aborted;
>         struct rsp_que *rsp;
>
>         struct atio_from_isp atio;
>         /* DO NOT ADD ANYTHING ELSE HERE - atio must be last member */
> };
>
> The location of the 'atio' member is subsequently used as the destination
> for a memcpy() that's expected to fill in the extra bytes beyond the end
> of the struct.
>
> That explains the loud warning in the comment above, which ought to be
> sufficient to prevent some newly-added member from accidentally getting
> clobbered. But, in case that warning was missed somehow, we also have the
> failing assertion,
>
> BUILD_BUG_ON(offsetof(struct qla_tgt_sess_op, atio) + sizeof(u->atio) !=
>              sizeof(*u));
>
> Unfortunately, this size assertion doesn't guarantee that 'atio' is the
> last member. Indeed, adding a zero-length array member at the end does
> not increase the struct size.
>
> Moreover, this assertion can fail even when 'atio' really is the last
> member, and that's what happened with commit e428b013d9df ("atomic:
> specify alignment for atomic_t and atomic64_t"), which added 2 bytes of
> harmless padding to the end of the struct.
>
> Remove the problematic assertion. The comments alone should be enough to
> prevent mistakes.
>
> Cc: Tony Battersby <tonyb@xxxxxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: linux-m68k@xxxxxxxxxxxxxxxxxxxx
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-kbuild-all/202603030747.VX0v4otS-lkp@xxxxxxxxx/
> Signed-off-by: Finn Thain <fthain@xxxxxxxxxxxxxx>

Thanks for your patch!

> ---
> I don't know of a good way to encode an invariant like "the last member of
> struct qla_tgt_sess_op is named atio" such that it might be statically
> checked. But perhaps there is a good way to do that (?)

Keeping the BUILD_BUG_ON(), but adding "__aligned(8);" to
the ratio member, as suggested by Arnd, would do that?

> There's no Fixes tag here because there's no need to backport.
> The BUILD_BUG_ON() comes from commit 091719c21d5a ("scsi: qla2xxx: target:
> Fix invalid memory access with big CDBs") which appeared in v6.19-rc1.
> The build failure first appeared in v7.0-rc1 with commit e428b013d9df
> ("atomic: specify alignment for atomic_t and atomic64_t").

I would add both in Fixes, just in case anyone ever wants to backport
e428b013d9df (which looks like a valid bugfix to me).

> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -213,7 +213,6 @@ static void qlt_queue_unknown_atio(scsi_qla_host_t *vha,
>         unsigned int add_cdb_len = 0;
>
>         /* atio must be the last member of qla_tgt_sess_op for add_cdb_len */
> -       BUILD_BUG_ON(offsetof(struct qla_tgt_sess_op, atio) + sizeof(u->atio) != sizeof(*u));

Iff you remove the BUILD_BUG_ON(), you should remove the comment, too.

>
>         if (tgt->tgt_stop) {
>                 ql_dbg(ql_dbg_async, vha, 0x502c,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds