std: sys: net: uefi: tcp: Initial TcpListener support by Ayush1325 · Pull Request #145339 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation37 Commits1 Checks11 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Add support for binding and accepting TCP4 connections.

While testing, the following network options were used with QEMU + OVMF: -nic user,hostfwd=tcp::12345-:12345

The default localhost address on qemu seems to be 10.0.2.15.

UEFI spec does not seem to state that the TCP Handle returned by the Accept method has a ServiceBinding Protocol. So have made the ServiceBinding Protocol optional.

cc @nicholasbishop

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Aug 13, 2025

Passing this along to someone else, who hopefully has more context. r? libs

Thom is off the review rotation
r? libs

Also gentle ping @nicholasbishop for a review since we usually defer UEFI to you

Comment on lines 111 to 114

// The spec does not seem to state if we need to call ServiceBinding->DestroyChild for
// this handle

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reference implementation that does or doesn't do this? Or a way to check whether or not the handle is valid after this call?

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment has been minimized.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the waiting-on-author label isn't actually accurate?

This LGTM, with the note that if you are following some sort of reference implementation for #145339 (comment) then it would be good to mention what that is in a comment.

You should probably rebase before merge and verify that it still builds, since sys::net has had some changes in recent weeks. Also cc @nicholasbishop if you'd like to double check this.

View changes since this review

let completion_token =
tcp4::CompletionToken { event: evt.as_ptr(), status: Status::SUCCESS };
let mut listen_token =
tcp4::ListenToken { completion_token, new_child_handle: crate::ptr::null_mut() };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably just import crate::ptr, it's used a lot of places in this module and will likely wind up in more in the future. (totally optional here ofc)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

This comment has been minimized.

This LGTM, with the note that if you are following some sort of reference implementation for #145339 (comment) then it would be good to mention what that is in a comment.

So, just looked at the internals of edk2 again, and it does seem like I need to use service binding protocol to clean it up. From what I gather of the implementation of TcpServiceBindingDestroyChild, it seems like at least edk2 assumes there to be only one service binding protocol for both tcp4 and tcp6. In fact, no data from service binding protocol is passed or used to destroy the socket (other than then service binding protocol not being null).

So will adjust the PR once I get time.

I am now using the parent service binding protocol to handle in any connections accepted by TCP4. On EDK2, this is not really required, since there seems to be an assumption of a single service binding protocol for both TCP4 and TCP6. However, best to be safe.

I have also refactored service binding wrapper a bit. It's best to cleanup tcp4 protcol handles in it's own drop.

@rustbot ready

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 694 to 700

pub(crate) fn destroy_child(&self, handle: NonNullcrate::ffi::c\_void\) -> io::Result<()> {
let sbp = open_protocol::<service_binding::Protocol>(self.handle, self.service_guid)?;

// SAFETY: Child handle must be allocated by the current service binding protocol.
let r = unsafe { ((*sbp.as_ptr()).destroy_child)(sbp.as_ptr(), handle.as_ptr()) };
if r.is_error() { Err(crate::io::Error::from_raw_os_error(r.as_usize())) } else { Ok(()) }
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be unsafe if the precondition of the destroy_child call depends on the validity of handle.

Also, is it safe to call destroy_child with the same handle more than once?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be unsafe if the precondition of the destroy_child call depends on the validity of handle.

Done

Also, is it safe to call destroy_child with the same handle more than once?

No, that is not valid.

Comment on lines 654 to +669

pub(crate) struct ServiceProtocol {
service_guid: r_efi::efi::Guid,
handle: NonNullcrate::ffi::c\_void\,
child_handle: NonNullcrate::ffi::c\_void\,
}

impl ServiceProtocol {
pub(crate) fn open(service_guid: r_efi::efi::Guid) -> io::Result<Self> {
/// Open a child handle on a service_binding protocol.
pub(crate) fn open(
service_guid: r_efi::efi::Guid,
) -> io::Result<(Self, NonNullcrate::ffi::c\_void\)> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type Handle = NonNull<crate::ffi::c_void> may be a worthwhile typedef since it's used a few times in this file and in Tcp4

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, a handle typedef already exists in r_efi. It is type Handle = *mut c_void. Maybe I should remove NonNull in all handles, but well, I started with NonNull 3 years ago, so kinda stuck with it.

Comment on lines 679 to 681

fn create_child(
sbp: NonNull<service_binding::Protocol>,
) -> io::Result<NonNullcrate::ffi::c\_void\> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think this needs to be unsafe too for the same reason

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 649 to 663

///
/// SAFETY: Copying ServiceProtocol is safe as long as handle is valid. For most service binding
/// protocols, at least in edk2 implementations, the lifetime of these handles will be till UEFI is
/// active, i.e. 'static
#[derive(Clone, Copy)]
pub(crate) struct ServiceProtocol {
service_guid: r_efi::efi::Guid,
handle: NonNullcrate::ffi::c\_void\,
child_handle: NonNullcrate::ffi::c\_void\,
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SAFETY comment shouldn't be here since duplicating this struct doesn't do anything by itself. If the handle is expected to be valid for 'static, this would be better expressed as an /// Invariant: ... comment on the handle field. Then:

For unsafe calls that use handle, you can say they are valid by the invariant
For places where ServiceProtocol is constructed, you can add an // INVARIANT: comment saying why they meet that invariant. And call out the assumption.

For most service binding protocols, at least in edk2 implementations, the lifetime of these handles will be till UEFI is active, i.e. 'static

Are there cases where a relevant handle here will disappear earlier that we need to worry about?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there cases where a relevant handle here will disappear earlier that we need to worry about?

The spec does not prevent it, but most implementations are based on edk2, which has it valid for UEFI lifetime, i.e. 'static.

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Oct 3, 2025

Add support for binding and accepting TCP4 connections.

While testing, the following network options were used with QEMU + OVMF: -nic user,hostfwd=tcp::12345-:12345

The default localhost address on qemu seems to be 10.0.2.15.

UEFI spec does not seem to state that the TCP Handle returned by the Accept method has a ServiceBinding Protocol. So have made the ServiceBinding Protocol optional.

Signed-off-by: Ayush Singh ayush@beagleboard.org

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Nov 3, 2025

Labels

O-UEFI

UEFI

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.