Re: [PATCH v2 1/3] nvme-tcp: use an aligned buffer for the TLS record type

[Martin Belanger <martin.nvme.dev@gmail.com>]

On 11/12/26 17:37, David Laight wrote:
> Is that right?
> You are changing something that was CMSG_LEN(1) to one of CMSG_SPACE(1).
>
> I've a feeling it would be right if it was a struct, not a union.

I think the union is right. buf[] covers the whole control message:
the header, the data and the padding. CMSG_SPACE(1) already includes
sizeof(struct cmsghdr). The cmsghdr member only gives the buffer its
alignment. With a struct, the header would be counted twice.

On x86_64, CMSG_LEN(1) is 17 and CMSG_SPACE(1) is 24. The union is 24
bytes with 8-byte alignment. put_cmsg() writes CMSG_LEN(1) bytes and
sets cmsg_len to 17, as before. It then advances msg_control by
min(CMSG_SPACE(1), msg_controllen), so CMSG_SPACE() is the size it
expects.

This is the form that cmsg(3) documents, and the kernel already uses
it:

- net/sunrpc/svcsock.c, svc_tcp_recv_cmsg(), and
  net/sunrpc/xprtsock.c, xs_sock_recv_cmsg(): the same union, for
  the same TLS record type
- arch/um/os-Linux/file.c, arch/um/os-Linux/skas/process.c and
  arch/um/kernel/skas/stub.c: the same union for SCM_RIGHTS

The commit message of 1/3 names xs_tls_read_record(). The function
is xs_sock_recv_cmsg(). I will fix it in the next version, if there
is one.

> Did you test it?
> Or even compile it and compare the two object files.

Yes. I tested v2 and v3 with TLS records that carry no application
data, before the ICResp and on a live queue. These records go through
this buffer.

I also compared tcp.o before and after 1/3 (x86_64, gcc 15.2). Before,
cbuf is at %rsp+0x77 in nvme_tcp_recv_icresp() and at %rsp+0x7f in
nvme_tcp_skip_tls_record(). The stack pointer is only 8-byte aligned,
so the struct cmsghdr is not aligned, and put_cmsg() writes cmsg_len
to an unaligned address. After, the buffer is at %rsp+0x80 and the
stack is aligned to 16 bytes. msg_controllen changes from 17 to 24.
The other differences are register allocation and stack layout.

v3 has the same change in 1/3.

Martin