Skip to content

if_tun.h ioctls for android #4379

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 10, 2025
Merged

if_tun.h ioctls for android #4379

merged 1 commit into from
Apr 10, 2025

Conversation

jjanowsk
Copy link
Contributor

@jjanowsk jjanowsk commented Apr 3, 2025 •
edited
Loading

Description

Add missing constants from linux/if_tun.h header on android platform. Mainly ioctl operation codes

Sources

https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/kernel/uapi/linux/if_tun.h;drc=180edefbd287c39caeb9d48784a9a10ac35f3636

Checklist

  • Relevant tests in libc-test/semver have been updated
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2025

r? @tgross35

rustbot has assigned @tgross35.
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
Copy link
Collaborator

rustbot commented Apr 3, 2025

Some changes occurred in the Android module

cc @maurer

@jjanowsk
Copy link
Contributor Author

jjanowsk commented Apr 3, 2025

@rustbot label stable-nominated

@rustbot rustbot added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Apr 3, 2025
@jjanowsk jjanowsk mentioned this pull request Apr 4, 2025
Merged
maurer
maurer reviewed Apr 4, 2025
View reviewed changes
Copy link

@maurer maurer left a comment

Choose a reason for hiding this comment

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

Main thing is we should probably use IOW/R to mimic what's in the bionic header. This will help us make sure things are synced up + structs people expect are available.

src/unix/linux_like/android/b32/mod.rs Outdated
@@ -232,6 +232,12 @@ pub const UT_HOSTSIZE: usize = 16;
pub const SIGSTKSZ: size_t = 8192;
pub const MINSIGSTKSZ: size_t = 2048;

// linux/if_tun.h
// Ioctl operation codes
pub const TUNATTACHFILTER: c_int = 0x400854d5;
Copy link

Choose a reason for hiding this comment

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

These should be declared via _IOW/_IOR, as they are in the source of truth.

We have this functionality in libc already, so we should use it rather than pre-expanding things like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I was not aware we already had this functionality. I've updated the PR.

src/unix/linux_like/android/b64/mod.rs Outdated
@@ -288,6 +288,12 @@ pub const UT_LINESIZE: usize = 32;
pub const UT_NAMESIZE: usize = 32;
pub const UT_HOSTSIZE: usize = 256;

// linux/if_tun.h
// Ioctl operation codes
pub const TUNATTACHFILTER: c_int = 0x401054d5;
Copy link

Choose a reason for hiding this comment

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

As above, IOW/R

src/unix/linux_like/android/mod.rs
@@ -2646,6 +2647,36 @@ pub const TUN_F_TSO_ECN: c_uint = 0x08;
pub const TUN_F_UFO: c_uint = 0x10;
pub const TUN_F_USO4: c_uint = 0x20;
pub const TUN_F_USO6: c_uint = 0x40;
// Protocol info prepended to the packets (when IFF_NO_PI is not set)
Copy link

Choose a reason for hiding this comment

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

IOW/R

src/unix/linux_like/android/mod.rs Outdated
@@ -2623,7 +2623,7 @@ pub const SND_CNT: usize = SND_MAX as usize + 1;
pub const UINPUT_VERSION: c_uint = 5;
pub const UINPUT_MAX_NAME_SIZE: usize = 80;

// bionic/libc/kernel/uapi/linux/if_tun.h
// linux/if_tun.h
Copy link

Choose a reason for hiding this comment

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

This should ideally remain sourced from bionic, as these are libc bindings, not kernel bindings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

@jjanowsk jjanowsk force-pushed the android_ioctls branch 3 times, most recently from dca2ef4 to f48364d Compare April 9, 2025 13:02
@jjanowsk
if_tun.h ioctls for android
610eb02 
Add missing constants from linux/if_tun.h header on android platform.
Mainly ioctl operation codes
@jjanowsk
Copy link
Contributor Author

jjanowsk commented Apr 9, 2025

@maurer Updated. Please re-review

tgross35
tgross35 reviewed Apr 9, 2025
View reviewed changes
src/unix/linux_like/linux/mod.rs
Comment on lines +5948 to 5950
pub const fn _IO(ty: u32, nr: u32) -> u32 {
super::_IOC(super::_IOC_NONE, ty, nr, 0)
}
Copy link
Contributor

@tgross35 tgross35 Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Could these just move to src/unix/linux_like/mod.rs next to _IOC? Rather than duplicating between linux and android.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not strictly duplicates. They differ when it comes to the return type. Of course I could put them in some non public module (e.g. mod internal) inside of linux_like/mod.rs and then reexport it in e.g. linux and wrap in android to cast the returned value. I thought about it but in the end I thought that that's just cleaner to duplicate those two small pieces. Let me know what you prefer. I can do it as I described here.

BTW using those _IO* macros are nice because the remove duplication when it comes IOCTL definitions on different architectures. I could use them for if_tun.h ioctl numbers in linux tree as well in separate PR if you're interested.

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed the return type difference, as-is is fine then 👍

Updating any hardcoded ioctl values would be appreciated if you are interested in doing that. A lot of these definitions come from before we had const fn available, it would be great to clean that up.

tgross35
tgross35 approved these changes Apr 10, 2025
View reviewed changes
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR and thanks @maurer for reviewing

@tgross35 tgross35 added this pull request to the merge queue Apr 10, 2025
Merged via the queue into rust-lang:main with commit 821a7e6 Apr 10, 2025
44 of 45 checks passed
jjanowsk added a commit to jjanowsk/libc that referenced this pull request Apr 11, 2025
@jjanowsk
if_tun.h ioctls for android
61fd993 
Add missing constants from linux/if_tun.h header on android platform.
Mainly ioctl operation codes

(backport <rust-lang#4379>)
(cherry picked from commit 610eb02)
@jjanowsk jjanowsk mentioned this pull request Apr 11, 2025
Merged
3 tasks
tgross35 pushed a commit to jjanowsk/libc that referenced this pull request Apr 11, 2025
@jjanowsk @tgross35
if_tun.h ioctls for android
8b54b5c 
Add missing constants from linux/if_tun.h header on android platform.
Mainly ioctl operation codes

(backport <rust-lang#4379>)
(cherry picked from commit 610eb02)
@tgross35 tgross35 removed the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Apr 11, 2025
@tgross35 tgross35 added the stable-applied This PR has been cherry-picked to libc's stable release branch label Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maurer maurer maurer approved these changes

@tgross35 tgross35 tgross35 approved these changes

Assignees

@tgross35 tgross35

Labels
O-android O-linux O-linux-like O-unix S-waiting-on-review stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jjanowsk @rustbot @maurer @tgross35