[PATCH] nvmet-fc: Remove __counted_by from nvmet_fc_tgt_queue.fod[]

[PATCH] nvmet-fc: Remove __counted_by from nvmet_fc_tgt_queue.fod[]

[Nathan Chancellor]

Work for __counted_by on generic pointers in structures (not just
flexible array members) has started landing in Clang 19 (current tip of
tree). During the development of this feature, a restriction was added
to __counted_by to prevent the flexible array member's element type from
including a flexible array member itself such as:

  struct foo {
    int count;
    char buf[];
  };

  struct bar {
    int count;
    struct foo data[] __counted_by(count);
  };

because the size of data cannot be calculated with the standard array
size formula:

  sizeof(struct foo) * count

This restriction was downgraded to a warning but due to CONFIG_WERROR,
it can still break the build. The application of __counted_by on the fod
member of 'struct nvmet_fc_tgt_queue' triggers this restriction,
resulting in:

  drivers/nvme/target/fc.c:151:2: error: 'counted_by' should not be applied to an array with element of unknown size because 'struct nvmet_fc_fcp_iod' is a struct type with a flexible array member. This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
    151 |         struct nvmet_fc_fcp_iod         fod[] __counted_by(sqsize);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  1 error generated.

Remove this use of __counted_by to fix the warning/error. However,
rather than remove it altogether, leave it commented, as it may be
possible to support this in future compiler releases.

Cc: stable@vger.kernel.org
Closes: https://github.com/ClangBuiltLinux/linux/issues/2027
Fixes: ccd3129aca28 ("nvmet-fc: Annotate struct nvmet_fc_tgt_queue with __counted_by")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/nvme/target/fc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 337ee1cb09ae..381b4394731f 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -148,7 +148,7 @@ struct nvmet_fc_tgt_queue {
 	struct workqueue_struct		*work_q;
 	struct kref			ref;
 	/* array of fcp_iods */
-	struct nvmet_fc_fcp_iod		fod[] __counted_by(sqsize);
+	struct nvmet_fc_fcp_iod		fod[] /* __counted_by(sqsize) */;
 } __aligned(sizeof(unsigned long long));
 
 struct nvmet_fc_hostport {

---
base-commit: c758b77d4a0a0ed3a1292b3fd7a2aeccd1a169a4
change-id: 20240529-drop-counted-by-fod-nvmet-fc-tgt-queue-50edd2f8d60e

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH] nvmet-fc: Remove __counted_by from nvmet_fc_tgt_queue.fod[]

[Jiri Slaby]

On 29. 05. 24, 23:42, Nathan Chancellor wrote:
>    drivers/nvme/target/fc.c:151:2: error: 'counted_by' should not be applied to an array with element of unknown size because 'struct nvmet_fc_fcp_iod' is a struct type with a flexible array member.

The same as for mxser_port:

struct nvmet_fc_fcp_iod {
         struct nvmefc_tgt_fcp_req       *fcpreq;

         struct nvme_fc_cmd_iu           cmdiubuf;
         struct nvme_fc_ersp_iu          rspiubuf;
         dma_addr_t                      rspdma;
         struct scatterlist              *next_sg;
         struct scatterlist              *data_sg;
         int                             data_sg_cnt;
         u32                             offset;
         enum nvmet_fcp_datadir          io_dir;
         bool                            active;
         bool                            abort;
         bool                            aborted;
         bool                            writedataactive;
         spinlock_t                      flock;

         struct nvmet_req                req;
         struct work_struct              defer_work;

         struct nvmet_fc_tgtport         *tgtport;
         struct nvmet_fc_tgt_queue       *queue;

         struct list_head                fcp_list;       /* 
tgtport->fcp_list */
};

The error appears to be invalid.

> This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
>      151 |         struct nvmet_fc_fcp_iod         fod[] __counted_by(sqsize);
>          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    1 error generated.
-- 
-- 
js
suse labs

Re: [PATCH] nvmet-fc: Remove __counted_by from nvmet_fc_tgt_queue.fod[]

[Nathan Chancellor]

Hi Jiri,

On Thu, May 30, 2024 at 08:41:18AM +0200, Jiri Slaby wrote:
> On 29. 05. 24, 23:42, Nathan Chancellor wrote:
> >    drivers/nvme/target/fc.c:151:2: error: 'counted_by' should not be applied to an array with element of unknown size because 'struct nvmet_fc_fcp_iod' is a struct type with a flexible array member.
> 
> The same as for mxser_port:
> 
> struct nvmet_fc_fcp_iod {
>         struct nvmefc_tgt_fcp_req       *fcpreq;
> 
>         struct nvme_fc_cmd_iu           cmdiubuf;
>         struct nvme_fc_ersp_iu          rspiubuf;
>         dma_addr_t                      rspdma;
>         struct scatterlist              *next_sg;
>         struct scatterlist              *data_sg;
>         int                             data_sg_cnt;
>         u32                             offset;
>         enum nvmet_fcp_datadir          io_dir;
>         bool                            active;
>         bool                            abort;
>         bool                            aborted;
>         bool                            writedataactive;
>         spinlock_t                      flock;
> 
>         struct nvmet_req                req;
>         struct work_struct              defer_work;
> 
>         struct nvmet_fc_tgtport         *tgtport;
>         struct nvmet_fc_tgt_queue       *queue;
> 
>         struct list_head                fcp_list;       /* tgtport->fcp_list
> */
> };
> 
> The error appears to be invalid.
> 
> > This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
> >      151 |         struct nvmet_fc_fcp_iod         fod[] __counted_by(sqsize);
> >          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    1 error generated.

My apologies, I should have done the work to fully uncover the flexible
array member within 'struct nvmet_fc_fcp_iod' from the beginning and put
it in the commit message. I did not think of using pahole to make my
life easier until just now and I knew from the other examples that I had
and clang's code that it was not incorrect. Sure enough, it comes from
'struct bio' within 'struct nvmet_req'.

struct nvmet_fc_fcp_iod {
...
	struct nvmet_req		req;
...
};

struct nvmet_req {
...
	struct bio_vec		inline_bvec[NVMET_MAX_INLINE_BIOVEC];
	union {
		struct {
			struct bio      inline_bio;
		} b;
		struct {
			bool			mpool_alloc;
			struct kiocb            iocb;
			struct bio_vec          *bvec;
			struct work_struct      work;
		} f;
		struct {
			struct bio		inline_bio;
			struct request		*rq;
			struct work_struct      work;
			bool			use_workqueue;
		} p;
#ifdef CONFIG_BLK_DEV_ZONED
		struct {
			struct bio		inline_bio;
			struct work_struct	zmgmt_work;
		} z;
#endif /* CONFIG_BLK_DEV_ZONED */
	};
	int			sg_cnt;
...
};

struct bio {
...
	struct bio_set		*bi_pool;

	/*
	 * We can inline a number of vecs at the end of the bio, to avoid
	 * double allocations for a small number of bio_vecs. This member
	 * MUST obviously be kept at the very end of the bio.
	 */
	struct bio_vec		bi_inline_vecs[];
};

It sounds like it is already on Gustavo's radar to look into for
-Wflexible-array-member-not-at-end, so he said he would take a look. It
may not be a quick fix though (I'll let him comment on it further if he
is so inclined). It will be needed in stable because the patch that
added __counted_by to this structure is there, so considering this patch
for that sake may still be worthwhile, then it could be reverted with
Gustavo's changes.

I would really like to avoid leaving the build with tip of tree Clang
broken for a long period of time, as we qualify it against the kernel
continously so that any fixes needed on the kernel side are merged and
ready by the time the toolchain is actually releases (such as this one).
I am fine with waiting some time to see how this plays out but I don't
want it to be forgotten about.

Cheers,
Nathan

Re: [PATCH] nvmet-fc: Remove __counted_by from nvmet_fc_tgt_queue.fod[]

[Nathan Chancellor]

Ping? This is still relevant and I don't think this is a compiler bug
that would justify withholding this change.

On Wed, May 29, 2024 at 02:42:40PM -0700, Nathan Chancellor wrote:
> Work for __counted_by on generic pointers in structures (not just
> flexible array members) has started landing in Clang 19 (current tip of
> tree). During the development of this feature, a restriction was added
> to __counted_by to prevent the flexible array member's element type from
> including a flexible array member itself such as:
> 
>   struct foo {
>     int count;
>     char buf[];
>   };
> 
>   struct bar {
>     int count;
>     struct foo data[] __counted_by(count);
>   };
> 
> because the size of data cannot be calculated with the standard array
> size formula:
> 
>   sizeof(struct foo) * count
> 
> This restriction was downgraded to a warning but due to CONFIG_WERROR,
> it can still break the build. The application of __counted_by on the fod
> member of 'struct nvmet_fc_tgt_queue' triggers this restriction,
> resulting in:
> 
>   drivers/nvme/target/fc.c:151:2: error: 'counted_by' should not be applied to an array with element of unknown size because 'struct nvmet_fc_fcp_iod' is a struct type with a flexible array member. This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
>     151 |         struct nvmet_fc_fcp_iod         fod[] __counted_by(sqsize);
>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   1 error generated.
> 
> Remove this use of __counted_by to fix the warning/error. However,
> rather than remove it altogether, leave it commented, as it may be
> possible to support this in future compiler releases.
> 
> Cc: stable@vger.kernel.org
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2027
> Fixes: ccd3129aca28 ("nvmet-fc: Annotate struct nvmet_fc_tgt_queue with __counted_by")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/nvme/target/fc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index 337ee1cb09ae..381b4394731f 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -148,7 +148,7 @@ struct nvmet_fc_tgt_queue {
>  	struct workqueue_struct		*work_q;
>  	struct kref			ref;
>  	/* array of fcp_iods */
> -	struct nvmet_fc_fcp_iod		fod[] __counted_by(sqsize);
> +	struct nvmet_fc_fcp_iod		fod[] /* __counted_by(sqsize) */;
>  } __aligned(sizeof(unsigned long long));
>  
>  struct nvmet_fc_hostport {
> 
> ---
> base-commit: c758b77d4a0a0ed3a1292b3fd7a2aeccd1a169a4
> change-id: 20240529-drop-counted-by-fod-nvmet-fc-tgt-queue-50edd2f8d60e
> 
> Best regards,
> -- 
> Nathan Chancellor <nathan@kernel.org>
> 

Re: [PATCH] nvmet-fc: Remove __counted_by from nvmet_fc_tgt_queue.fod[]

[Keith Busch]

On Wed, Jun 26, 2024 at 10:06:05AM -0700, Nathan Chancellor wrote:
> Ping? This is still relevant and I don't think this is a compiler bug
> that would justify withholding this change.

Sorry, I misunderstood the discussion to "wait" on this. Queued up in
nvme-6.10 now.

Re: [PATCH] nvmet-fc: Remove __counted_by from nvmet_fc_tgt_queue.fod[]

[Nathan Chancellor]

On Wed, Jun 26, 2024 at 11:19:24AM -0600, Keith Busch wrote:
> On Wed, Jun 26, 2024 at 10:06:05AM -0700, Nathan Chancellor wrote:
> > Ping? This is still relevant and I don't think this is a compiler bug
> > that would justify withholding this change.
> 
> Sorry, I misunderstood the discussion to "wait" on this. Queued up in
> nvme-6.10 now.

No worries, thanks a lot for the quick response!

Cheers,
Nathan