Hi Kent, Andrew pointed this out to me, and it's a FORTIFY issue under a W=1 build: On Sat, Sep 30, 2023 at 01:25:34PM +0800, kernel test robot wrote: > tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master > branch HEAD: df964ce9ef9fea10cf131bf6bad8658fde7956f6 Add linux-next specific files for 20230929 > > Error/Warning reports: > > [...] > https://lore.kernel.org/oe-kbuild-all/202309192314.VBsjiIm5-lkp@xxxxxxxxx fs/bcachefs/extents.c: In function 'bch2_bkey_append_ptr': include/linux/fortify-string.h:57:33: warning: writing 8 bytes into a region of size 0 [-Wstringop-overflow=] 57 | #define __underlying_memcpy __builtin_memcpy | ^ include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy' 648 | __underlying_##op(p, q, __fortify_size); \ | ^~~~~~~~~~~~~ include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk' 693 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \ | ^~~~~~~~~~~~~~~~~~~~ fs/bcachefs/extents.c:235:17: note: in expansion of macro 'memcpy' 235 | memcpy((void *) &k->v + bkey_val_bytes(&k->k), | ^~~~~~ fs/bcachefs/bcachefs_format.h:287:33: note: destination object 'v' of size 0 287 | struct bch_val v; | ^ The problem here is the struct bch_val is explicitly declared as a zero-sized array, so the compiler becomes unhappy. :) Converting bch_val to a flexible array will just kick the can down the road, since this is going to run into -Wflex-array-member-not-at-end soon too since bch_val overlaps with other structures: struct bch_inode_v3 { struct bch_val v; __le64 bi_journal_seq; ... }; As a container_of() target, this is fine -- leave it a zero-sized array. The problem is using it as a destination for memcpy, etc, since the compiler will believe it to be 0 sized. Instead, we need to impart a type of some kind so that the compiler can actually unambiguously reason about sizes. The memcpy() in the warning is targeting bch_val, so I think the best fix is to correctly handle the different types. So just to have everything in front of me, here's a summary of what I'm seeing in the code: struct bkey { /* Size of combined key and value, in u64s */ __u8 u64s; ... }; /* Empty placeholder struct, for container_of() */ struct bch_val { __u64 __nothing[0]; }; struct bkey_i { __u64 _data[0]; struct bkey k; struct bch_val v; }; static inline void bch2_bkey_append_ptr(struct bkey_i *k, struct bch_extent_ptr ptr) { EBUG_ON(bch2_bkey_has_device(bkey_i_to_s(k), ptr.dev)); switch (k->k.type) { case KEY_TYPE_btree_ptr: case KEY_TYPE_btree_ptr_v2: case KEY_TYPE_extent: EBUG_ON(bkey_val_u64s(&k->k) >= BKEY_EXTENT_VAL_U64s_MAX); ptr.type = 1 << BCH_EXTENT_ENTRY_ptr; memcpy((void *) &k->v + bkey_val_bytes(&k->k), &ptr, sizeof(ptr)); k->k.u64s++; break; default: BUG(); } } So this is appending u64s into the region that start with bkey_i. Could this gain a u64 flexible array? struct bkey_i { __u64 _data[0]; struct bkey k; struct bch_val v; __u64 ptrs[]; }; Then the memcpy() could be just a direct assignment: k->ptrs[bkey_val_u64s(&k->k)] = (u64)ptr; k->k.u64s++; Alternatively, perhaps struct bkey could be the one to grow this flexible array, and then it could eventually be tracked with __counted_by (but not today since it expects to count the array element count, not a whole struct size): struct bkey { /* Size of combined key and value, in u64s */ __u8 u64s; ... __u64 data[] __counted_by(.u64s - BKEY_U64s); }; And bch_val could be dropped... Then the memcpy becomes: k->k.u64s++; k->k.data[bkey_val_u64s(&k->k)] = (u64)ptr; It just seems like there is a lot of work happening that could really just type casting or unions... What do you think? -- Kees Cook
