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@cybernetics.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-m68k@lists.linux-m68k.org
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202603030747.VX0v4otS-lkp@intel.com/
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
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 (?)

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").
---
 drivers/scsi/qla2xxx/qla_target.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index d772136984c9..06c1f3b577c4 100644
--- 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));
 
 	if (tgt->tgt_stop) {
 		ql_dbg(ql_dbg_async, vha, 0x502c,
-- 
2.49.1

On 3/5/26 18:01, Finn Thain 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.
...
> 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 (?)

It might work better to add a flex array:

struct qla_tgt_sess_op {
	...

	struct atio_from_isp atio;
	/*
	atio.u.isp24.fcp_cmnd.add_cdb may extend past end of atio;
	DO NOT DELETE; DO NOT ADD ANYTHING ELSE HERE.
	*/
	uint8_t atio_isp24_fcp_cmnd_add_cdb[];
};

/* atio_isp24_fcp_cmnd_add_cdb must come immediately after atio */
BUILD_BUG_ON(offsetof(struct qla_tgt_sess_op, atio) +
             sizeof(struct atio_from_isp) !=
             offsetof(struct qla_tgt_sess_op, atio_isp24_fcp_cmnd_add_cdb));

Tony Battersby

On Fri, 6 Mar 2026, Tony Battersby wrote:

> On 3/5/26 18:01, Finn Thain wrote:
> ...
> > 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 (?)
> 
> It might work better to add a flex array:
> 
> struct qla_tgt_sess_op {
> 	...
> 
> 	struct atio_from_isp atio;
> 	/*
> 	atio.u.isp24.fcp_cmnd.add_cdb may extend past end of atio;
> 	DO NOT DELETE; DO NOT ADD ANYTHING ELSE HERE.
> 	*/
> 	uint8_t atio_isp24_fcp_cmnd_add_cdb[];
> };
> 
> /* atio_isp24_fcp_cmnd_add_cdb must come immediately after atio */
> BUILD_BUG_ON(offsetof(struct qla_tgt_sess_op, atio) +
>              sizeof(struct atio_from_isp) !=
>              offsetof(struct qla_tgt_sess_op, atio_isp24_fcp_cmnd_add_cdb));
> 

I think that makes sense because the flex array member reflects what the 
algorithm actually does (whereas artificial struct padding would not).

And it works better than the present code, because the compiler prohibits 
any new member at the end of the struct.

It is more complex than the patch I sent but maintainers may still prefer 
it, so I will put it into a formal patch submission.

BTW, I thought it would make more sense to add a flex array in struct 
isp24 (in struct atio_from_isp) but it doesn't work because the compiler 
doesn't prohibit aggregation:

struct s1 {
        int i;
        int a[];
};

struct s2 {
        struct s1 s;
        int x;        /* this is not prohibited */
};

Hi Finn,

On Fri, 6 Mar 2026 at 00:03, Finn Thain <fthain@linux-m68k.org> 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@cybernetics.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-m68k@lists.linux-m68k.org
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202603030747.VX0v4otS-lkp@intel.com/
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>

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@linux-m68k.org

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

On Fri, 6 Mar 2026, Geert Uytterhoeven wrote:

> > 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?
> 

No, that would merely limit the possibilities for tail padding in the 
future. Again, the tail padding is harmless. The assertion is bogus, not 
the struct layout.

Like I said to Arnd in that thread, the kind of twisted logic that would 
add an alignment rule here requires a comment to explain it (there is an 
example of this elsewhere in the same driver).

So now we're writing comments to explain code that exists solely to 
support a spurious assertion, which itself exists solely to allow an 
inadequate checker to prevent accidents that were already prevented by the 
warning in the comments. Why? Fear of regression.

It's true what they say -- "fear is the mind killer".

> > 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).
> 

Good point. I will add both.

> > --- 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.
> 

Again, the comment refers to an assertion about the name of the last 
member. The BUILD_BUG_ON is an assertion about the size of the struct. 
These are not the same thing. If they were, I could agree with you: 
"remove both or neither".

Thanks for your review.