libquic: serialize setsockopt() with handshake sendmsg steps - #64
Conversation
|
@metze-samba Please help review this change whenever you're available. Thanks! |
|
Ok, this is just a userspace change in order to force the correct order of syscalls, because there were possible Is that assumption correct? I'll port that change to samba in order to do some testing with it. |
|
While integrating it into samba I noticed that quic_handshake_step_sockopt doesn't have I guess for now this can be set hard coded to SOL_QUIC in quic_prepare_sockopt_step(). What I don't really understand yet is this: struct quic_smsg *last; seems to be unused. quic_smsg_append_list(ctx, smsg, ctx->completed); for rx_secret why is ctx->completed passed as 'uint8_t send' And the logic around this 'send' in quic_smsg_append_list() is not Is 'send' supposed to indicate that it's a sendmsg? If so we can already check this |
right, no kernel ABI is affected.
Sure, I can add it.
'send' is used to indicate this step will trigger data sending, it doesn't have to be sendmsg step. The APP TX secret setting will also trigger data sending in kernel. ctx->completed happens to be set in quic_set_secret() when APP TX secret setting sockopt step is added, then I reuse it, which seems not a nice idea. 'last' points to last node of the list, used to link the new node into the list more efficiently. send_last points to the last node (that was added with send = 1) of the list, used to set MSG_MORE flag for send_last if the new node to add will trigger data sending. I will add some comments for these. Please also check if there's other better way to implement this sockopt step. Thanks. |
|
Updated. Note without this patch, the packet series are: With this patch: Thanks. |
|
With the new patch it's a bit confusing that 'int level' is not used either for SOL_QUIC or the crypto level, But it's still confusing that we set MSG_MORE in these cases:
I did some tests with the first patch and it failed for some reason, So something broke between 846dddb In the kernel I applied the v9 patchset (or something between v8 and v9), Applying these (the top3) to Samba breaks it: Even the first one alone |
if SOCKOPT is added with 'send' = 1, setsockopt() will trigger kernel to send some control msgs like NCI frames. if SOCKOPT is added with 'send' = 0, it won't to the 'if (send)' branch to change the last one (SENDMSG) . So assuming the SOCKOPT you mentioned below are all added 'send' = 1,
The last one SENDMSG will send msg A with MSG_MORE, and the msg A in kernel will NOT scheduled to send due to the MSG_MORE flags. Then the new one SOCKOPT will call setsockopt(), which triggers kernel to send a msg B, then the kernel will try to bundle the A and B into one packet and send it out.
The last one SOCKOPT will ignore the MSG_MORE (although the flags is set), and trigger kernel to send a msg anyway, there will NOT be msg bundle with the msg sent in the new one SENDMSG. We actually think the SOCKOPT with 'send' = 1 as the last step of handshake, and the new one SENDMSG will be application data(not handshake message) after handshake is completed, so I didn't expose a field in Thanks. |
Could be this one missing in the v9 patchset: I add some reserved fields for the future extension in some structs, and work around it in kernel with this commit. It could break, if you're using the new uapi quic.h, but the kernel part is old. It should be fixed by using the v10 quic from https://github.com/lxin/net-next/commits/quic/ Thanks. |
Maybe calling it 'int flush' or 'int eor' would be better to understand. |
Ok, but will take a bit of time before I'm able to test it, maybe one or two weeks. In the meantime is there a userspace patch I can revert for now, so that I can test the setsockopt changes with the v9 kernel module? |
Yes, try is your code using /usr/include/linux/quic.h? Make sure the the same reserved field doesn't exists, although it shouldn't be there if it was installed from kernel tree. Let me know if it doesn't work. Thanks. |
And it that doesn't help I'll also try to revert fad9bfd.
No, it's using the custom one. |
Interesting, does the |
I guess it can't even build But removing it from quic_handshake_info in the Samba copy fixes the problem. But it means that new userspace would not work with old kernel, correct? |
Yes, I only considered the new kernel with the old userspace. Sorry for this inconvenience. |
This fixes it too: Please just do the commit with you as owner and handle all the related Also note that using offsetof(struct quic_stream_info, reserved) |
|
I noticed that 00d7b2b |
|
Maybe a bunch of checks like this: BUILD_BUG_ON(sizeof(struct io_uring_sqe) != 64); in io_uring_init() |
On 32 bits systems, sizeof(struct quic_stream_info) is 12 bytes, not 16 bytes. It was added to fix a failure when older userspace interacts with a newer kernel on 32 bits systems. Just note that these offsetofs are temporary work around, they will all be reverted when I post the 2nd patchset to upstream, before that I check and make sure all users to get quic.h to the latest version. Now I think it seems not a good idea to apply the commit "libquic: adjust cmsg_len for QUIC_HANDSHAKE_INFO", as after the revert, the userspace code need sto be reverted again, otherwise, the older userpsace can not interacts with the new kernel again. Is that okay to you that I remove the commit "libquic: adjust cmsg_len for QUIC_HANDSHAKE_INFO" that I just applied? And on your side, update the kernel part to v10 or add a temporary fix on userspace for the current test? |
Updated to int flush, please check. Thanks. |
I'd prefer to keep it, I will mean any possible existing installation will break. |
What's the problem with 2d7f826 ? |
The existing installation is fine if they don't rebuild the userspace code with the new quic.h fomr quic repo while using the QUIC <= v9. If we use that commit now, It will be really a pain after the QUIC get merged into kernel and we want to extend the quic_handshake_info with the reserved field in the future, because all old userspace will break after the extending. My plan was to expose this issue when the upstream kernel doesn't support QUIC yet. This is a hard one, and I need to think more about it. Thanks. |
It means I won't update Samba with the setsockopt stuff, as it means it needs the lastest kernel module
When the reserved stuff will be used by new userspace it expects the kernel to support it too Maybe it would be better to have a getsockopt() call to ask the kernel for features I also think it might be useful for quic_config and quic_transport_param to be handled with I have a patch for copy_struct_to_sockptr() here: |
|
What I want to say is, the problem of supporting growing structures for new features |
Feel free to use it, but please try to fix typos and missing words in the commit message. |
That's indeed a problem.
OK, this should work. I've brought 2d7f826 back to the repo.
Cool! |
c16c040 to
8ef560c
Compare
|
More more thing, I think QUIC_HANDSHAKE_STEP_OP_SOCKOPT should be QUIC_HANDSHAKE_STEP_OP_SETSOCKOPT, and s_sockopt; s_setsockopt; In future we may need a GETSOCKOPT... |
updated. |
|
The Samba selftests don't work with this using quic_ko_wrapper. |
You mean it doesn't work with SETSOCKOPT step, but it worked without SETSOCKOPT step?
Sure, leave me a message if anything needed. BTW, is it easy to deploy and run the Samba selftests? I'm think to add a github workflow in this repo to trigger it whenever a commit is submitted. Thanks. |
|
@metze-samba FYI, an issue was just reported in #66, which caused the failures in the interop testing with neqo userland QUIC, I just revert the buggy commit. No sure if this may fix the samba selftests failure. (Note the issue only existed on the QUIC repo kernel modules, NOT upstream v9 or v10 patches). |
Handshake messages and QUIC_SOCKOPT_CRYPTO_SECRET were previously queued
together and processed while handling a received message. However,
installing the application RX secret can immediately trigger
application-level frames in the kernel (e.g. NEW_CONNECTION_ID, NEW_TOKEN,
HANDSHAKE_DONE).
Because setsockopt() was not explicitly serialized with sendmsg(), those
application frames could be emitted before all pending handshake CRYPTO
frames were transmitted, violating the required ordering that handshake
messages precede any application-level frames.
Introduce QUIC_HANDSHAKE_STEP_OP_SETSOCKOPT and treat setsockopt()
operations as explicit handshake steps. This ensures:
- setsockopt() is ordered relative to sendmsg() operations
- all handshake CRYPTO frames are transmitted before installing the
application RX secret
- kernel-triggered application frames cannot overtake handshake data
The handshake engine now serializes all externally visible side effects
(sendmsg, recvmsg, setsockopt), guaranteeing correct QUIC packet
ordering and integrating setsockopt() into the handshake event loop.
Update the man page to document the new step type.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
2 participants
Handshake messages and QUIC_SOCKOPT_CRYPTO_SECRET were previously queued together and processed while handling a received message. However, installing the application RX secret can immediately trigger application-level frames in the kernel (e.g. NEW_CONNECTION_ID, NEW_TOKEN, HANDSHAKE_DONE).
Because setsockopt() was not explicitly serialized with sendmsg(), those application frames could be emitted before all pending handshake CRYPTO frames were transmitted, violating the required ordering that handshake messages precede any application-level frames.
Introduce QUIC_HANDSHAKE_STEP_OP_SOCKOPT and treat setsockopt() operations as explicit handshake steps. This ensures:
The handshake engine now serializes all externally visible side effects (sendmsg, recvmsg, setsockopt), guaranteeing correct QUIC packet ordering and integrating setsockopt() into the handshake event loop.
Update the man page to document the new step type.