Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote:
> > This will be an error in a future compiler version
> > [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
> > 291 | struct mxser_port ports[] __counted_by(nports);
> > | ^
> >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.
>
> This looks like a compiler bug/deficiency.
I agree, why not just turn that option off in the compiler so that these
"warnings" will not show up?
> What does gcc say BTW?
>
> > Cc: sta...@vger.kernel.org
> > Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
> > Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")
>
> I would not say "Fixes" here. It only works around a broken compiler.
Agreed, don't add Fixes: for this, it's a compiler bug, not a kernel
issue.
thanks,
greg k-h
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
On 30. 05. 24, 10:12, Gustavo A. R. Silva wrote: On 30/05/24 09:40, Greg Kroah-Hartman wrote: On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote: This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size] 291 | struct mxser_port ports[] __counted_by(nports); | ^ 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. This looks like a compiler bug/deficiency. I agree, why not just turn that option off in the compiler so that these "warnings" will not show up? It's not a compiler bug. It is, provided the code compiles and runs. The flexible array is nested four struct layers deep, see: ports[].port.buf.sentinel.data[] The error report could be more specific, though. Ah, ok. The assumption is sentinel.data[] shall be unused. That's why it all works. The size is well known, [] is zero size, right? Still, fix the compiler, not the code. thanks, -- js suse labs
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
On 30. 05. 24, 10:33, Jiri Slaby wrote: On 30. 05. 24, 10:12, Gustavo A. R. Silva wrote: On 30/05/24 09:40, Greg Kroah-Hartman wrote: On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote: This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size] 291 | struct mxser_port ports[] __counted_by(nports); | ^ 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. This looks like a compiler bug/deficiency. I agree, why not just turn that option off in the compiler so that these "warnings" will not show up? It's not a compiler bug. It is, provided the code compiles and runs. The flexible array is nested four struct layers deep, see: ports[].port.buf.sentinel.data[] The error report could be more specific, though. Ah, ok. The assumption is sentinel.data[] shall be unused. That's why it all works. The size is well known, [] is zero size, right? Still, fix the compiler, not the code. Or fix the code (properly). Flex arrays (even empty) in the middle of structs (like ports[].port.buf.sentinel.data[] above is) are deprecated since gcc 14: https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626516.html So we should get rid of all those. Sooner than later. thanks, -- -- js suse labs
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
On Thu, May 30, 2024 at 1:41 AM Jiri Slaby wrote: > > On 30. 05. 24, 10:33, Jiri Slaby wrote: > > On 30. 05. 24, 10:12, Gustavo A. R. Silva wrote: > >> > >> > >> On 30/05/24 09:40, Greg Kroah-Hartman wrote: > >>> On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote: > > This will be an error in a future compiler version > > [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size] > > 291 | struct mxser_port ports[] __counted_by(nports); > > | ^ > > 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. > > This looks like a compiler bug/deficiency. > >>> > >>> I agree, why not just turn that option off in the compiler so that these > >>> "warnings" will not show up? > >> > >> It's not a compiler bug. > > > > It is, provided the code compiles and runs. > > > >> The flexible array is nested four struct layers deep, see: > >> > >> ports[].port.buf.sentinel.data[] > >> > >> The error report could be more specific, though. > > > > Ah, ok. The assumption is sentinel.data[] shall be unused. That's why it > > all works. The size is well known, [] is zero size, right? > > > > Still, fix the compiler, not the code. > > Or fix the code (properly). > > Flex arrays (even empty) in the middle of structs (like > ports[].port.buf.sentinel.data[] above is) are deprecated since gcc 14: > https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626516.html > > So we should get rid of all those. Sooner than later. > Yes! Please do this. -bw
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
Ah, ok. The assumption is sentinel.data[] shall be unused. This snippet is useful information. Thanks -- Gustavo
Re: [PATCH] codetag: avoid race at alloc_slab_obj_exts
On 5/27/24 8:30 PM, Thadeu Lima de Souza Cascardo wrote:
> When CONFIG_MEM_ALLOC_PROFILING_DEBUG is enabled, the following warning may
> be noticed:
>
> [ 48.299584] [ cut here ]
> [ 48.300092] alloc_tag was not set
> [ 48.300528] WARNING: CPU: 2 PID: 1361 at include/linux/alloc_tag.h:130
> alloc_tagging_slab_free_hook+0x84/0xc7
> [ 48.301305] Modules linked in:
> [ 48.301553] CPU: 2 PID: 1361 Comm: systemd-udevd Not tainted
> 6.10.0-rc1-3-gac8755535862 #176
> [ 48.302196] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.15.0-1 04/01/2014
> [ 48.302752] RIP: 0010:alloc_tagging_slab_free_hook+0x84/0xc7
> [ 48.303169] Code: 8d 1c c4 48 85 db 74 4d 48 83 3b 00 75 1e 80 3d 65 02 86
> 04 00 75 15 48 c7 c7 11 48 1d 85 c6 05 55 02 86 04 01 e8 64 44 a5 ff <0f> 0b
> 48 8b 03 48 85 c0 74 21 48 83 f8 01 74 14 48 8b 50 20 48 f7
> [ 48.304411] RSP: 0018:8880111b7d40 EFLAGS: 00010282
> [ 48.304916] RAX: RBX: 88800fcc9008 RCX:
>
> [ 48.305455] RDX: 8000 RSI: 88801406 RDI:
> ed1002236f97
> [ 48.305979] RBP: 1100 R08: fbfff0aa73a1 R09:
>
> [ 48.306473] R10: 814515e5 R11: 0003 R12:
> 88800fcc9000
> [ 48.306943] R13: 88800b2e5cc0 R14: 8880111b7d90 R15:
>
> [ 48.307529] FS: 7faf5d1908c0() GS:88806cf0()
> knlGS:
> [ 48.308223] CS: 0010 DS: ES: CR0: 80050033
> [ 48.308710] CR2: 58fb220c9118 CR3: 110cc000 CR4:
> 00750ef0
> [ 48.309274] PKRU: 5554
> [ 48.309804] Call Trace:
> [ 48.310029]
> [ 48.310290] ? show_regs+0x84/0x8d
> [ 48.310722] ? alloc_tagging_slab_free_hook+0x84/0xc7
> [ 48.311298] ? __warn+0x13b/0x2ff
> [ 48.311580] ? alloc_tagging_slab_free_hook+0x84/0xc7
> [ 48.311987] ? report_bug+0x2ce/0x3ab
> [ 48.312292] ? handle_bug+0x8c/0x107
> [ 48.312563] ? exc_invalid_op+0x34/0x6f
> [ 48.312842] ? asm_exc_invalid_op+0x1a/0x20
> [ 48.313173] ? this_cpu_in_panic+0x1c/0x72
> [ 48.313503] ? alloc_tagging_slab_free_hook+0x84/0xc7
> [ 48.313880] ? putname+0x143/0x14e
> [ 48.314152] kmem_cache_free+0xe9/0x214
> [ 48.314454] putname+0x143/0x14e
> [ 48.314712] do_unlinkat+0x413/0x45e
> [ 48.315001] ? __pfx_do_unlinkat+0x10/0x10
> [ 48.315388] ? __check_object_size+0x4d7/0x525
> [ 48.315744] ? __sanitizer_cov_trace_pc+0x20/0x4a
> [ 48.316167] ? __sanitizer_cov_trace_pc+0x20/0x4a
> [ 48.316757] ? getname_flags+0x4ed/0x500
> [ 48.317261] __x64_sys_unlink+0x42/0x4a
> [ 48.317741] do_syscall_64+0xe2/0x149
> [ 48.318171] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 48.318602] RIP: 0033:0x7faf5d8850ab
> [ 48.318891] Code: fd ff ff e8 27 dd 01 00 0f 1f 80 00 00 00 00 f3 0f 1e fa
> b8 5f 00 00 00 0f 05 c3 0f 1f 40 00 f3 0f 1e fa b8 57 00 00 00 0f 05 <48> 3d
> 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 41 2d 0e 00 f7 d8
> [ 48.320649] RSP: 002b:7ffc44982b38 EFLAGS: 0246 ORIG_RAX:
> 0057
> [ 48.321182] RAX: ffda RBX: 5ba344a44680 RCX:
> 7faf5d8850ab
> [ 48.321667] RDX: RSI: 5ba344a44430 RDI:
> 7ffc44982b40
> [ 48.322139] RBP: 7ffc44982c00 R08: R09:
> 0007
> [ 48.322598] R10: 5ba344a44430 R11: 0246 R12:
>
> [ 48.323071] R13: 7ffc44982b40 R14: R15:
>
> [ 48.323596]
>
> This is due to a race when two objects are allocated from the same slab,
> which did not have an obj_exts allocated for.
>
> In such a case, the two threads will notice the NULL obj_exts and after one
> assigns slab->obj_exts, the second one will happily do the exchange if it
> reads this new assigned value.
>
> In order to avoid that, verify that the read obj_exts does not point to an
> allocated obj_exts before doing the exchange.
>
> Fixes: 09c46563ff6d ("codetag: debug: introduce OBJEXTS_ALLOC_FAIL to mark
> failed slab_ext allocations")
> Signed-off-by: Thadeu Lima de Souza Cascardo
> Cc: Suren Baghdasaryan
Acked-by: Vlastimil Babka
Thanks!
> ---
> mm/slub.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 0809760cf789..1373ac365a46 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1952,7 +1952,7 @@ int alloc_slab_obj_exts(struct slab *slab, struct
> kmem_cache *s,
> #ifdef CONFIG_MEMCG
> new_exts |= MEMCG_DATA_OBJEXTS;
> #endif
> - old_exts = slab->obj_exts;
> + old_exts = READ_ONCE(slab->obj_exts);
> handle_failed_objexts_alloc(old_exts, vec, objects);
> if (new_slab) {
> /*
> @@ -1961,7 +1961,8 @@ int alloc_slab_obj_exts(struct slab *slab, struct
> kmem_cache *s,
>* be simply assigned.
>*/
> slab->obj_exts = new_exts;
> - } else if (
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
On 30/05/24 09:40, Greg Kroah-Hartman wrote:
On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote:
This will be an error in a future compiler version
[-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
291 | struct mxser_port ports[] __counted_by(nports);
| ^
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.
This looks like a compiler bug/deficiency.
I agree, why not just turn that option off in the compiler so that these
"warnings" will not show up?
It's not a compiler bug.
The flexible array is nested four struct layers deep, see:
ports[].port.buf.sentinel.data[]
The error report could be more specific, though.
--
Gustavo
What does gcc say BTW?
Cc: sta...@vger.kernel.org
Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")
I would not say "Fixes" here. It only works around a broken compiler.
Agreed, don't add Fixes: for this, it's a compiler bug, not a kernel
issue.
thanks,
greg k-h
Re: [PATCH] RDMA/irdma: Annotate flexible array with __counted_by() in struct irdma_qvlist_info
On Sun, 19 May 2024 09:02:15 +0200, Christophe JAILLET wrote: > 'num_vectors' is used to count the number of elements in the 'qv_info' > flexible array in "struct irdma_qvlist_info". > > So annotate it with __counted_by() to make it explicit and enable some > additional checks. > > This allocation is done in irdma_save_msix_info(). > > [...] Applied, thanks! [1/1] RDMA/irdma: Annotate flexible array with __counted_by() in struct irdma_qvlist_info https://git.kernel.org/rdma/rdma/c/38c02d813aa321 Best regards, -- Leon Romanovsky
Re: [PATCH net-next v2] net: mana: Allow variable size indirection table
On Tue, May 28, 2024 at 10:35:55PM -0700, Shradha Gupta wrote:
> Allow variable size indirection table allocation in MANA instead
> of using a constant value MANA_INDIRECT_TABLE_SIZE.
> The size is now derived from the MANA_QUERY_VPORT_CONFIG and the
> indirection table is allocated dynamically.
>
> Signed-off-by: Shradha Gupta
> Reviewed-by: Dexuan Cui
> Reviewed-by: Haiyang Zhang
> ---
> Changes in v2:
> * Rebased to latest net-next tree
> * Rearranged cleanup code in mana_probe_port to avoid extra operations
> ---
> drivers/infiniband/hw/mana/qp.c | 10 +--
Jakub,
Once you are ok with this patch, let me create shared branch for it.
It is -rc1 and Konstantin already submitted some changes to qp.c
https://lore.kernel.org/all/1716366242-558-1-git-send-email-kotara...@linux.microsoft.com/
This specific patch applies on top of Konstantin's changes cleanly.
Thanks
> drivers/net/ethernet/microsoft/mana/mana_en.c | 68 ---
> .../ethernet/microsoft/mana/mana_ethtool.c| 20 --
> include/net/mana/gdma.h | 4 +-
> include/net/mana/mana.h | 9 +--
> 5 files changed, 84 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> index ba13c5abf8ef..2d411a16a127 100644
> --- a/drivers/infiniband/hw/mana/qp.c
> +++ b/drivers/infiniband/hw/mana/qp.c
> @@ -21,7 +21,7 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev
> *dev,
>
> gc = mdev_to_gc(dev);
>
> - req_buf_size = struct_size(req, indir_tab, MANA_INDIRECT_TABLE_SIZE);
> + req_buf_size = struct_size(req, indir_tab,
> MANA_INDIRECT_TABLE_DEF_SIZE);
> req = kzalloc(req_buf_size, GFP_KERNEL);
> if (!req)
> return -ENOMEM;
> @@ -41,18 +41,18 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev
> *dev,
> if (log_ind_tbl_size)
> req->rss_enable = true;
>
> - req->num_indir_entries = MANA_INDIRECT_TABLE_SIZE;
> + req->num_indir_entries = MANA_INDIRECT_TABLE_DEF_SIZE;
> req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
>indir_tab);
> req->update_indir_tab = true;
> req->cqe_coalescing_enable = 1;
>
> /* The ind table passed to the hardware must have
> - * MANA_INDIRECT_TABLE_SIZE entries. Adjust the verb
> + * MANA_INDIRECT_TABLE_DEF_SIZE entries. Adjust the verb
>* ind_table to MANA_INDIRECT_TABLE_SIZE if required
>*/
> ibdev_dbg(&dev->ib_dev, "ind table size %u\n", 1 << log_ind_tbl_size);
> - for (i = 0; i < MANA_INDIRECT_TABLE_SIZE; i++) {
> + for (i = 0; i < MANA_INDIRECT_TABLE_DEF_SIZE; i++) {
> req->indir_tab[i] = ind_table[i % (1 << log_ind_tbl_size)];
> ibdev_dbg(&dev->ib_dev, "index %u handle 0x%llx\n", i,
> req->indir_tab[i]);
> @@ -137,7 +137,7 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp,
> struct ib_pd *pd,
> }
>
> ind_tbl_size = 1 << ind_tbl->log_ind_tbl_size;
> - if (ind_tbl_size > MANA_INDIRECT_TABLE_SIZE) {
> + if (ind_tbl_size > MANA_INDIRECT_TABLE_DEF_SIZE) {
> ibdev_dbg(&mdev->ib_dev,
> "Indirect table size %d exceeding limit\n",
> ind_tbl_size);
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index d087cf954f75..851e1b9761b3 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -481,7 +481,7 @@ static int mana_get_tx_queue(struct net_device *ndev,
> struct sk_buff *skb,
> struct sock *sk = skb->sk;
> int txq;
>
> - txq = apc->indir_table[hash & MANA_INDIRECT_TABLE_MASK];
> + txq = apc->indir_table[hash & (apc->indir_table_sz - 1)];
>
> if (txq != old_q && sk && sk_fullsock(sk) &&
> rcu_access_pointer(sk->sk_dst_cache))
> @@ -962,7 +962,16 @@ static int mana_query_vport_cfg(struct mana_port_context
> *apc, u32 vport_index,
>
> *max_sq = resp.max_num_sq;
> *max_rq = resp.max_num_rq;
> - *num_indir_entry = resp.num_indirection_ent;
> + if (resp.num_indirection_ent > 0 &&
> + resp.num_indirection_ent <= MANA_INDIRECT_TABLE_MAX_SIZE &&
> + is_power_of_2(resp.num_indirection_ent)) {
> + *num_indir_entry = resp.num_indirection_ent;
> + } else {
> + netdev_warn(apc->ndev,
> + "Setting indirection table size to default %d for
> vPort %d\n",
> + MANA_INDIRECT_TABLE_DEF_SIZE, apc->port_idx);
> + *num_indir_entry = MANA_INDIRECT_TABLE_DEF_SIZE;
> + }
>
> apc->port_handle = resp.vport;
> ether_addr_copy(apc->mac_addr, resp.mac_addr);
> @@ -1054,14 +1063,13 @@ static int mana_cfg_vport_steering(struct
> mana_port_context *apc,
>
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
So we should get rid of all those. Sooner than later. Yes! Please do this. Definitely, we are working on that already[1]. :) -- Gustavo [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=-Wflex-array-member-not-at-end
Re: [PATCH net-next v2] net: mana: Allow variable size indirection table
On Tue, 28 May 2024 22:35:55 -0700 Shradha Gupta wrote:
> + save_table = kcalloc(apc->indir_table_sz, sizeof(u32), GFP_KERNEL);
> + if (!save_table)
> + return -ENOMEM;
> +
> if (rxfh->indir) {
> - for (i = 0; i < MANA_INDIRECT_TABLE_SIZE; i++)
> + for (i = 0; i < apc->indir_table_sz; i++)
> if (rxfh->indir[i] >= apc->num_queues)
> return -EINVAL;
leaks save_table
Re: [PATCH] x86/boot: add prototype for __fortify_panic()
On 29.05.24 г. 21:09 ч., Jeff Johnson wrote: As discussed in [1] add a prototype for __fortify_panic() to fix the 'make W=1 C=1' warning: arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static? Actually doesn't it make sense to have this defined under ../string.h ? Actually given that we don't have any string fortification under the boot/ why have the fortify _* functions at all ? Link: https://lore.kernel.org/all/79653cc7-6e59-4657-9c0a-76f49f49d...@quicinc.com/ [1] Signed-off-by: Jeff Johnson --- arch/x86/boot/compressed/misc.h | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h index b353a7be380c..3a56138484a9 100644 --- a/arch/x86/boot/compressed/misc.h +++ b/arch/x86/boot/compressed/misc.h @@ -68,6 +68,7 @@ void __putdec(unsigned long value); #define error_putstr(__x) __putstr(__x) #define error_puthex(__x) __puthex(__x) #define error_putdec(__x) __putdec(__x) +void __fortify_panic(const u8 reason, size_t avail, size_t size); #ifdef CONFIG_X86_VERBOSE_BOOTUP --- base-commit: e0cce98fe279b64f4a7d81b7f5c3a23d80b92fbc change-id: 20240529-fortify_panic-325601efe71d
Re: [PATCH net-next v2] net: mana: Allow variable size indirection table
On Thu, 30 May 2024 17:37:02 +0300 Leon Romanovsky wrote: > Once you are ok with this patch, let me create shared branch for it. > It is -rc1 and Konstantin already submitted some changes to qp.c > https://lore.kernel.org/all/1716366242-558-1-git-send-email-kotara...@linux.microsoft.com/ > > This specific patch applies on top of Konstantin's changes cleanly. Yeah, once it's not buggy shared branch SG! Just to be sure, on top of -rc1, right?
Re: [PATCH net-next v2] net: mana: Allow variable size indirection table
On Thu, May 30, 2024 at 08:42:55AM -0700, Jakub Kicinski wrote: > On Thu, 30 May 2024 17:37:02 +0300 Leon Romanovsky wrote: > > Once you are ok with this patch, let me create shared branch for it. > > It is -rc1 and Konstantin already submitted some changes to qp.c > > https://lore.kernel.org/all/1716366242-558-1-git-send-email-kotara...@linux.microsoft.com/ > > > > This specific patch applies on top of Konstantin's changes cleanly. > > Yeah, once it's not buggy shared branch SG! Just to be sure, on top > of -rc1, right? Yes, it will be based on clean -rc1. Thanks
Re: [PATCH] x86/boot: add prototype for __fortify_panic()
On 5/30/2024 8:42 AM, Nikolay Borisov wrote: > > > On 29.05.24 г. 21:09 ч., Jeff Johnson wrote: >> As discussed in [1] add a prototype for __fortify_panic() to fix the >> 'make W=1 C=1' warning: >> >> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was >> not declared. Should it be static? > > Actually doesn't it make sense to have this defined under ../string.h ? > Actually given that we don't have any string fortification under the > boot/ why have the fortify _* functions at all ? I'll let Kees answer these questions since I just took guidance from him :) /jeff
Re: [PATCH] x86/boot: add prototype for __fortify_panic()
On May 30, 2024 6:23:36 PM GMT+02:00, Jeff Johnson wrote: >On 5/30/2024 8:42 AM, Nikolay Borisov wrote: >> >> >> On 29.05.24 г. 21:09 ч., Jeff Johnson wrote: >>> As discussed in [1] add a prototype for __fortify_panic() to fix the >>> 'make W=1 C=1' warning: >>> >>> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' >>> was not declared. Should it be static? >> >> Actually doesn't it make sense to have this defined under ../string.h ? >> Actually given that we don't have any string fortification under the >> boot/ why have the fortify _* functions at all ? > >I'll let Kees answer these questions since I just took guidance from him :) The more important question is how does the decompressor build even know of this symbol? And then make it forget it again instead of adding silly prototypes... -- Sent from a small device: formatting sucks and brevity is inevitable.
Re: [PATCH] nvmet-fc: Remove __counted_by from nvmet_fc_tgt_queue.fod[]
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;
> boolactive;
> boolabort;
> boolaborted;
> boolwritedataactive;
> spinlock_t flock;
>
> struct nvmet_reqreq;
> struct work_struct defer_work;
>
> struct nvmet_fc_tgtport *tgtport;
> struct nvmet_fc_tgt_queue *queue;
>
> struct list_headfcp_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_reqreq;
...
};
struct nvmet_req {
...
struct bio_vec inline_bvec[NVMET_MAX_INLINE_BIOVEC];
union {
struct {
struct bio inline_bio;
} b;
struct {
boolmpool_alloc;
struct kiocbiocb;
struct bio_vec *bvec;
struct work_struct work;
} f;
struct {
struct bio inline_bio;
struct request *rq;
struct work_struct work;
booluse_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] tty: mxser: Remove __counted_by from mxser_board.ports[]
On Thu, May 30, 2024 at 09:40:13AM +0200, Greg Kroah-Hartman wrote: > On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote: > > > This will be an error in a future compiler version > > > [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size] > > > 291 | struct mxser_port ports[] __counted_by(nports); > > > | ^ > > >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. > > > > This looks like a compiler bug/deficiency. I apologize. As I commented on the nvmet-fc patch, I should have included where the flexible array member was within 'struct mxser_port', especially since I already knew it from https://github.com/ClangBuiltLinux/linux/issues/2026. I hope we can all agree now that this is not a compiler bug or issue. > I agree, why not just turn that option off in the compiler so that these > "warnings" will not show up? I think the only part of the warning text that got quoted above should clarify why this is not a real solution. For the record, if this was a true issue on the compiler side, I would have made that very clear in the commit message (or perhaps not even sent the patch in the first place and worked to get it fixed on the compiler side, as ClangBuiltLinux has always tried to do first since the beginning). > > What does gcc say BTW? GCC does not have any support for __counted_by merged yet. I suspect that its current implementation won't say anything either, otherwise Kees should have noticed it in his testing. As you and Gustavo note further down thread, sentinel is flagged by -Wflex-array-member-not-at-end. In file included from include/linux/tty_port.h:8, from include/linux/tty.h:11, from drivers/tty/mxser.c:24: include/linux/tty_buffer.h:40:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] 40 | struct tty_buffer sentinel; | ^~~~ Gustavo has a suggested diff to resolve the issue at the GitHub issue I linked above that seems like a reasonable fix for both issues that is small enough to go into stable trees that contain f34907ecca71 like this one? Cheers, Nathan
