lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZS6WNXABUDTh8NZA@bfoster> Date: Tue, 17 Oct 2023 10:12:05 -0400 From: Brian Foster <bfoster@...hat.com> To: Kees Cook <keescook@...omium.org> Cc: Kent Overstreet <kent.overstreet@...ux.dev>, linux-bcachefs@...r.kernel.org, kernel test robot <lkp@...el.com>, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org Subject: Re: [PATCH] bcachefs: Refactor bkey_i to use a flexible array On Mon, Oct 16, 2023 at 02:18:19PM -0700, Kees Cook wrote: > On Mon, Oct 16, 2023 at 08:41:58AM -0400, Brian Foster wrote: > > On Fri, Oct 13, 2023 at 04:44:21PM -0700, Kees Cook wrote: > > > On Fri, Oct 13, 2023 at 07:26:11AM -0400, Brian Foster wrote: > > > > On Tue, Oct 10, 2023 at 04:56:12PM -0700, Kees Cook wrote: ... > > > > > > > > Hi Kees, > > > > > > > > I'm curious if this is something that could be buried in bch_val given > > > > it's already kind of a fake structure..? If not, my only nitty comment > > > > > > I was thinking it would be best to keep the flexible array has "high" in > > > the struct as possible, as in the future more refactoring will be needed > > > to avoid having flex arrays overlap with other members in composite > > > structures. So instead of pushing into bch_val, I left it at the highest > > > level possible, bch_i, as that's the struct being used by the memcpy(). > > > > > > Eventually proper unions will be needed instead of overlapping bch_i > > > with other types, as in: > > > > > > struct btree_root { > > > struct btree *b; > > > > > > /* On disk root - see async splits: */ > > > __BKEY_PADDED(key, BKEY_BTREE_PTR_VAL_U64s_MAX); > > > u8 level; > > > u8 alive; > > > s8 error; > > > }; > > > > > > But that's all for the future. Right now I wanted to deal with the more > > > pressing matter of a 0-sized array not being zero sized. :) > > > > > > > Ok, but I'm not really following how one approach vs. the other relates > > to this particular example of an embedded bkey_i. I'm probably just not > > familiar enough with the current issues with 0-sized arrays and the > > anticipated path forward. Can you elaborate for somebody who is more > > focused on trying to manage the design/complexity of these various key > > data structures? For example, what's the practical difference here (for > > future work) if the flex array lives in bch_val vs. bkey_i? > > I was looking strictly at the layer it was happening: the function that > calls memcpy() is working on a bkey_i, so I figured that was the place > for it currently. > > > Note that I don't necessarily have a strong opinion on this atm, but if > > there's a "for future reasons" aspect to this approach I'd like to at > > least understand it a little better. ;) > > The future work here is about making sure flexible arrays don't overlap > with non-flexible array members[1], and that will require giving some > thought to how the structures are arranged. > ... > The use of "struct header" effectively says "we have some number of bytes, > but we don't know *what* it is yet". Is it cmd_one, or cmd_two? Instead > of combining the flex array with the header, we can either split it off: > > struct header { > u32 long flags; > struct other things; > size_t byte_count; > }; > > struct cmd_unknown { > struct header; > u8 bytes[]; > }; > > Or we can merge all the structs together: > > struct everything { > u32 long flags; > struct other things; > size_t byte_count; > union { > struct cmd_one { > u64 detail; > u8 bits; > }; > struct cmd_two { > u32 foo, bar; > u64 baz; > }; > struct unknown { > u8 bytes[]; > }; > }; > }; > > In the first style, the flexible array is distinctly separate. In the > second style the overlap is explicitly shown via the union. > > I expect it will take a long time to make the kernel "flex array overlap > clean", so while I don't feel any rush, I've been generally trying to > avoid seeing new instances of ambiguous overlap _added_ to the kernel. :) > Got it. This helps explain potential wonkiness of a variant with the flex array buried in bch_val. > bcachefs is in a unique places where because it's been out of tree > its code patterns aren't "new", but it's just been "added" upstream. > *shrug* So we'll deal with the existing warnings we've already got, > and prepare for the future warnings as we can. > Ack. > Hopefully that helps! > Indeed it does, thanks! Brian > -Kees > > [1] See "-Wflex-array-member-not-at-end": > https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html > [2] Going all the way back to 76497732932f ("cxgb3/l2t: Fix undefined behaviour") > > > > > > > is that memcpy(k->bytes[], ...) makes it kind of read like we're copying > > > > in opaque key data rather than value data, so perhaps a slightly more > > > > descriptive field name would be helpful. But regardless I'd wait until > > > > Kent has a chance to comment before changing anything.. > > > > > > How about "v_bytes" instead of "bytes"? Or if it really is preferred, > > > I can move the flex array into bch_val -- it just seems like the wrong > > > layer... > > > > > > > Yeah.. v_bytes, value_bytes, etc. etc. Anything that avoids misleading > > code when using the field is good with me. Thanks. > > > > Brian > > > > > -Kees > > > > > > > > > > > Brian > > > > > > > > > > > > > > #define KEY(_inode, _offset, _size) \ > > > > > diff --git a/fs/bcachefs/extents.h b/fs/bcachefs/extents.h > > > > > index 7ee8d031bb6c..6248e17bbac5 100644 > > > > > --- a/fs/bcachefs/extents.h > > > > > +++ b/fs/bcachefs/extents.h > > > > > @@ -642,7 +642,7 @@ static inline void bch2_bkey_append_ptr(struct bkey_i *k, struct bch_extent_ptr > > > > > > > > > > ptr.type = 1 << BCH_EXTENT_ENTRY_ptr; > > > > > > > > > > - memcpy((void *) &k->v + bkey_val_bytes(&k->k), > > > > > + memcpy(&k->bytes[bkey_val_bytes(&k->k)], > > > > > &ptr, > > > > > sizeof(ptr)); > > > > > k->k.u64s++; > > > > > -- > > > > > 2.34.1 > > > > > > > > > > > > > > > -- > > > Kees Cook > > > > > > > -- > Kees Cook >
Powered by blists - more mailing lists