Re: [PATCH 1/7] tcp: Pass TCP header and payload separately to tcp_update_check_tcp[46]()

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 29 Oct 2024 20:26:25 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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@gibson.dropbear.id.au> wrote:
> >   
> > > 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@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'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.

Oh, hm, right. In the original case we were discussing in that thread
it was coming from an offset in a static struct, but if it's not the
case anymore, then we should check ourselves I guess (possibly with the
function + macro you mention below?).

> > > > > 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.    
> > > 
> > > 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.  
> > 
> > Right, and we can use the compiler to test for suitable alignment.  
> 
> 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.

-- 
Stefano