On Tue, Oct 29, 2024 at 10:09:54AM +0100, Stefano Brivio wrote:On Tue, 29 Oct 2024 15:07:56 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:I'm not exactly sure what you're suggesting with this. I don't think the compiler will catch it in this case, because we're constructing the (possibly) misaligned pointer as a (void *), then implicitly casting it by passing it to a (tcp_payload_t *) argument. (void *) is explicitly allowed to be cast to any pointer type, so I think the compiler will take this as asserting we know what we're doing. More fool it.On Tue, Oct 29, 2024 at 02:02:25PM +1100, David Gibson wrote: > On Mon, Oct 28, 2024 at 07:42:54PM +0100, Stefano Brivio wrote: > > On Mon, 28 Oct 2024 20:40:44 +1100 > > David Gibson <david(a)gibson.dropbear.id.au> wrote: > > > > > Currently these expects both the TCP header and payload in a single IOV, > > > and goes to some trouble to locate the checksum field within it. In the > > > current caller we've already know where the TCP header is, so we might as > > > well just pass it in. This will need to work a bit differently for > > > vhost-user, but that code already needs to locate the TCP header for other > > > reasons, so again we can just pass it in. > > > > We couldn't do this, and also what you're now doing in 5/7, because > > with vhost-user the TCP header is not aligned, so we can't pass it > > around as a pointer, see: > > > > <ZeUpxEY-sn64NLE5@zatzit> > > https://archives.passt.top/passt-dev/ZeUpxEY-sn64NLE5@zatzit/ > > > > and following. That one is about IP headers, but the same applies to > > TCP and UDP headers. > > Hrm. I'm aware it theoretically need not be aligned, but I thought it > was in practice.. and that we were already relying on that. > > In fact, I'm pretty sure the second part is true, although more subtly > than here. v8 of the vhost-user patches calls tcp_fill_headers[46]() > with the bp parameter set to the offset of the TCP header. If > creating a tcphdr * there is a problem, then creating a tcp_payload_t > * can't be any better.Ah, okay, I missed that. Still, I think we should ask gcc for an opinion (with the vhost-user series on top of this series), because those build-time pointer alignment checks are pretty reliable.I do see allowing the compiler to check this in more cases as an advantage of using explicitly typed pointers where we can. Btw, I didn't find a use for it just yet, but I also have a draft patch which adds a function+macro that extracts a typed pointer from a given offset into a IO vector, verifying that it's contiguous and properly aligned. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibsonRight, and we can use the compiler to test for suitable alignment.I had a closer look at that earlier message now. I believe at the time I was aiming for fully robust handling of misaligned user buffers. AIUI, we've given up on that for the time being: instead we'll just *test* for suitable alignment and we can do the hard work of handling it if it ever arises in practice.Of course the current solution is not elegant and it would be nice to find another way to deal with it, but we couldn't come up with anything better back then. The rest of the series looks good to me, but I'm afraid that without this one and 5/7 the other changes will be a bit more complicated to implement (if at all possible).Definitely. I have so ideas for approaches more robust to misalignment, but they're substantially more complicated. I was hoping we could avoid it at least for now.