Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstractions of socket and related network entities #1066

Open
wants to merge 7 commits into
base: staging/rust-net
Choose a base branch
from

Conversation

micheledallerive
Copy link

Add Rust abstractions for basic network entities, socket and its wrappers (TcpStream and UdpSocket).

Specifically, it was added:

  • Ip address and Socket address wrappers (for in_addr, in6_addr, sockaddr_in, sockaddr_in6, sockaddr_storage).
  • Socket wrapper.
  • Socket flags and options enums.
  • TCP and UDP specific abstractions over the Rust Socket structure.

Currently, there is no explicit user for these abstractions.

The original patch series was sent as a RFC to the mailing list.
The mailing list patches contained some questions about some portions of the code; if it's relevant, I will promptly rewrite them in this PR as well.

I'm not sure how the rust-net workflow will be, but I'm super available and happy to discuss changes to the code and improve/correct it.
By the way, I am very glad this branch become a thing and hopefully Rust will be able to become more and more used in the net subsystem!

Michele Dalle Rive added 7 commits February 24, 2024 16:17
Create `net` module files and network headers in `bindings_helper.h`.
Add `IpProtocol`, `AddressFamily` and `Namespace`.

The wrappers added with this patch are shared across the whole network
subsystem. For this reason, they are placed in the `net.rs` module file.

The enum `IpProtocol`, however, is placed in an individual `ip.rs`
submodule, allowing to place together all the ip-related structures,
such as wrappers for `iphdr`, `ip_auth_hdr`, etc.

Signed-off-by: Michele Dalle Rive <dallerivemichele@gmail.com>
Create structures to handle addresses: `Ipv4Addr`, `Ipv6Addr`,
`SocketAddr`, `SocketAddrV4` and `SocketAddrV6`.

These structures are meant to be as similar as possible to the ones in
Rust `std::net`, while, at the same time, providing functionalities
available in the kernel.

Some extra structures are added, compared to `std`:
- `SocketAddrStorage`: wraps `struct sockaddr_storage` and is used to
  interact with the kernel functions when the type of socket address is
  unknown. Since it is only used for FFI, it is crate-public.
- `GenericSocketAddr`: trait that defines shared functions and traits
  amont all socket addresses.

Signed-off-by: Michele Dalle Rive <dallerivemichele@gmail.com>
Add enums representing flags related to sockets:
- `ReceiveFlag` to modify the behaviour of the socket receive operation.
- `SendFlag` to modify the behaviour of the socket send operation.
- `MessageFlag` to represent the flags in a `msghdr`.
- `SocketFlag` to represent the flags in the `socket` struct.

Introduce a `FlagSet` structure to offer a convenient way to handle the
flags.
Having an abstraction over the "raw" numerical value of the flags offers
many advantages:
- A `FlagSet` can be created in different ways: from an `IntoIterator`,
  a value, a single flag or using the defined macro `flag_set!(...)`.
- Custom operations can be defined, such as the bitwise or.
- Flags in the set can be set, tested, unset through functions instead
  of using bitwise operations.
- FlagSet implements the IntoIterator trait, allowing for iteration over
  the flags contained.

Signed-off-by: Michele Dalle Rive <dallerivemichele@gmail.com>
Create a `Socket` abstraction, which provides a Rust API to the kernel
socket functionalities.

The Socket structures tries to keep the same function signatures of the
Rust standard library; at the same time, functions are added or modified
in order to provide as much as possible of the C kernel functionalities.

Most of the internals of the C socket is not accessible by Rust, because
those structures are still to be wrapped. However, sockets are mainly
managed through the functions provided by the kernel; thus, even if some
fields are not accessible, since the functions are wrapped, most of the
kernel functionality should be available in Rust as well.

Specifically, the usage of `msghdr` is mostly abstracted away in the
Rust interface, because using it would mean having to deal, both in the
kernel and in modules, with Pinned instances (msghdr is self-referencing),
which would be a struggle that provides no particular advantage.
A `MessageHeader` object is actually created and returned when a message
is received, because at that point the structure is not really
self-referencing, as long as the source address is copied. The wrapper
is not used when a message is sent.
Anyways, some useful functionalities of `msghdr`, like `cmsghdr`s, are
missing and should be implemented in the future to provide a complete API.

Signed-off-by: Michele Dalle Rive <dallerivemichele@gmail.com>
Create socket `Option`s and `set_option` function in the `Socket`
abstraction.

These changes introduce wrappers and functions to handle socket options
in Rust, with compilation-time advantages compared to the C API:
- Type safety: A specific option accepts only a value of the correct
  type.
- Read/write safety: A read-only option cannot be set.
- Coherence safety: An option of, for example, IP level cannot be set by
  specifying another level.

The downside of using options in the kernel is the lack of functions to
get the value of an option. For this reason, in Rust, kernel options can
only be set, but not retrieved.

Everything that can be done by socket options can actually be done
through helper functions, or by accessing directly the specific fields.
However, since the Rust-wrapped structures are few, it can be useful to
have options in order to still be able to modify the behaviour of the
socket.

As specified in the documentation of `opts.rs`, options could (and
should) be removed when the Rust API will be developed enough.

Signed-off-by: Michele Dalle Rive <dallerivemichele@gmail.com>
Add `TcpListener` and `TcpStream` wrappers around the Rust Socket.
They provide a convenient way to handle TCP sockets.

This interface is intended to be as close as possible to the one in `std::net`.

Signed-off-by: Michele Dalle Rive <dallerivemichele@gmail.com>
Add a UDP socket wrapper, which allows to handle UDP sockets conveniently.

This interface is intended to be as close as possible to the one in `std::net`.

Signed-off-by: Michele Dalle Rive <dallerivemichele@gmail.com>
@vobst vobst self-assigned this Feb 27, 2024
@vobst
Copy link
Member

vobst commented Feb 28, 2024

Hi Michele,

thanks a lot for posting this work! So far I've only had time to look at patches 01 and 02, but here are some things that I noticed.

Some general things:

  • It is good practice to wrap commit messages at 75 chars and code comment lines at 100 chars (./scripts/checkpatch.pl can be used to check this).
  • It is uncommon (to say the least) to end commit messages with a period. (That's different from Rust comments where we absolutely want to end everything with a period.)
  • The patches currently do not compile for two reasons: 1. changes in the code that bindgen generates and 2. your doctests. The changes in bindgen are quite trivial; for the doctests I'd recommend having a look at the other doctests in the kernel crate. Please note that there are some limitations to what is possible in doctests in the kernel. Below some truncated output of the failing compilation:
error[E0107]: struct takes 1 generic argument but 2 generic arguments were supplied
   --> rust/kernel/net/socket.rs:146:19
    |
146 |         bindings::__BindgenBitfieldUnit::<[u8; 8], u8>::new(self.flags().to_be_bytes())
    |                   ^^^^^^^^^^^^^^^^^^^^^            -- help: remove this generic argument
    |                   |
    |                   expected 1 generic argument
    |
note: struct defined here, with 1 generic parameter: `Storage`
   --> /mnt/build/rust-for-linux/rust4lx/rust/bindings/bindings_generated.rs:5:12
    |
5   | pub struct __BindgenBitfieldUnit<Storage> {
    |            ^^^^^^^^^^^^^^^^^^^^^ -------

error[E0107]: struct takes 1 generic argument but 2 generic arguments were supplied
   --> rust/kernel/net/socket.rs:166:23
    |
166 |             bindings::__BindgenBitfieldUnit::<[u8; 8], u8>::new(self.flags().to_be_bytes());
    |                       ^^^^^^^^^^^^^^^^^^^^^            -- help: remove this generic argument
    |                       |
    |                       expected 1 generic argument
    |
note: struct defined here, with 1 generic parameter: `Storage`
   --> /mnt/build/rust-for-linux/rust4lx/rust/bindings/bindings_generated.rs:5:12
    |
5   | pub struct __BindgenBitfieldUnit<Storage> {
    |            ^^^^^^^^^^^^^^^^^^^^^ -------

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0107`.
error: cannot find macro `println` in this scope
    --> rust/doctests_kernel_generated.rs:3846:4
     |
3846 |    println!("Flag: {:?}", flag);
     |    ^^^^^^^

error[E0599]: no function or associated item named `from_str` found for struct `rust_doctest_kernel_build_assert_rs_0::kernel::net::addr::SocketAddrV4` in the current scope
    --> rust/doctests_kernel_generated.rs:1534:26
     |
1534 | let addr = SocketAddrV4::from_str("1.2.3.4:5678").unwrap();
     |                          ^^^^^^^^ function or associated item not found in `SocketAddrV4`
     |
note: if you're trying to build a new `rust_doctest_kernel_build_assert_rs_0::kernel::net::addr::SocketAddrV4`, consider using `rust_doctest_kernel_build_assert_rs_0::kernel::net::addr::SocketAddrV4::new` which returns `rust_doctest_kernel_build_assert_rs_0::kernel::net::addr::SocketAddrV4`
    --> /mnt/build/rust-for-linux/rust4lx/rust/kernel/net/addr.rs:553:5
     |
553  |     pub const fn new(addr: Ipv4Addr, port: u16) -> Self {
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
     |
3    + use core::str::FromStr;
     |
[...]
error[E0283]: type annotations needed for `FlagSet<_>`
    --> rust/doctests_kernel_generated.rs:3902:5
     |
3902 | let mut empty_flags = flag_set!();
     |     ^^^^^^^^^^^^^^^
3903 | assert_eq!(empty_flags.value(), 0);
     |            ----------- ----- required by a bound introduced by this call
     |            |
     |            type must be known at this point
     |
     = note: cannot satisfy `_: Flag`
     = help: the following types implement trait `Flag`:
               SendFlag
               ReceiveFlag
               MessageFlag
note: required by a bound in `FlagSet::<T>::value`
    --> /mnt/build/rust-for-linux/rust4lx/rust/kernel/net/socket/flags.rs:257:9
     |
257  | impl<T: Flag> FlagSet<T> {
     |         ^^^^ required by this bound in `FlagSet::<T>::value`
...
341  |     pub fn value(&self) -> isize {
     |            ----- required by a bound in this associated function
help: consider giving `empty_flags` an explicit type, where the type for type parameter `T` is specified
     |
3902 | let mut empty_flags: FlagSet<_> = flag_set!();
     |                    ++++++++++++

error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
    --> rust/doctests_kernel_generated.rs:4206:83
     |
4201 | fn main() { #[allow(non_snake_case)] fn _doctest_main_rust_kernel_net_socket_rs_137_0() {
     |                                      -------------------------------------------------- this function should return `Result` or `Option` to accept `?`
...
4206 | let socket = Socket::new(AddressFamily::Inet, SockType::Datagram, IpProtocol::Udp)?;
     |                                                                                   ^ cannot use the `?` operator in a function that returns `()`
     |
     = help: the trait `FromResidual<core::result::Result<Infallible, rust_doctest_kernel_build_assert_rs_0::kernel::error::Error>>` is not implemented for `()`

Please note that:

  • Review comments that are prefixed with "Style:" are things that you do not necessarily have to fix in order to have the changes in merged into rust-net, but that would be required in mainline. They usually don't only apply to the place where I have posted them, it's rather a general pattern that also appears at other places. It would still be nice if you could address those, as long as it is not too much work. It might be a good idea to wait with those until we have sorted out the more high-level issues pointed out by Trevor in order to avoid unnecessary work.

Finally, some questions where I'd be interested in the opinion of you and others:

  • Currently nothing in this series depends on any kernel config options besides the top-level CONFIG_NET. However, many things do not make much sense without, e.g., TCP/IP or IPV6 support, i.e., CONFIG_INET or CONFIG_IPV6. I'd like to raise the question if it would make sense to put some modules behind an additional cfg gate?
  • Related to the above point: Would it make sense to introduce new CONFIG_*s that toggle the compilation of these abstractions, e.g., like there is the CONFIG_RUST_PHYLIB_ABSTRACTIONS for the phy module.

Copy link
Collaborator

@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.

Thanks for the PR! Didn't get the chance to review everything but made a few overall comments.

We will get Rust 1.77 in 3 weeks which has the ip and socket types in core which would mean a lot of the changes to rust/kernel/net/ip.rs and rust/kernel/net/socket.rs could be dropped. Miguel actually already posted the version bump patch https://lore.kernel.org/rust-for-linux/CANiq72nbr6qy1otJpdh3FVUN5cUfrkUPYEHJFm_QqfKvYEj-Xw@mail.gmail.com/T/#m654659d25b00821ef3c51941b1791398bf0602e0, maybe we could apply that and just say you need to build with beta until it merges.


/// The address family.
///
/// See [`man 7 address families`](https://man7.org/linux/man-pages/man7/address_families.7.html) for more information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line wrap


fn try_from(value: isize) -> Result<Self, Self::Error> {
let val = value as u32;
match val {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably better to make this impl TryFrom<u32> rather than casting

/// Wraps a `struct in_addr`.
#[derive(Default, Copy, Clone)]
#[repr(transparent)]
pub struct Ipv4Addr(pub(crate) bindings::in_addr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ip_in_core will be available in 1.77 (upcoming release March 21) and add core::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6}, so it probably makes sense to update as soon as it is available. That would just need a conversion to/from bindings::in_addr, rather than reimplementing the methods here.

Any additional constants could be in kernel::net or in kernel::net::{ipv4, ipv6} modules. Worth noting that Rust's Ipv4Addr is not ABI-compatible with in_addr, align 1 vs. align 4.

Comment on lines +381 to +384
pub(crate) fn into<T: GenericSocketAddr>(self) -> T {
// SAFETY: The `self.0` field is a `struct sockaddr_storage` which is guaranteed to be large enough to hold any socket address.
unsafe { *(&self.0 as *const _ as *const T) }
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't safe unless GenericSocketAddr is also unsafe or you do something like assert on size_of:::<T>. But what is the purpose of GenericSocketAddr anyway? I think that SocketAddr could be used without a trait, calling .into() is trivial if you have a SocketAddrV4 or V6

pub mod opts;

/// The socket type.
pub enum SockType {
Copy link
Collaborator

Choose a reason for hiding this comment

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

One of the most confusing things in the kernel has to be that sock and socket are different things. Unfortunately I don't have any better suggestions, but we probably want to make a more clear distinction on the Rust side.

Comment on lines +129 to +131
unsafe {
(*self.0).flags = flags;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
unsafe {
(*self.0).flags = flags;
}
unsafe { (*self.0).flags = flags };

Semi outside braces formats nicer. Also safety comment

/// ```
pub fn has_flag(&self, flag: SocketFlag) -> bool {
bindings::__BindgenBitfieldUnit::<[u8; 8], u8>::new(self.flags().to_be_bytes())
.get_bit(flag as _)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The type should be specified for as casts to make the conversion clear. Quite a few cases below too. Maybe some could be converted to .into / .cast

Comment on lines +171 to +189
/// Consumes the socket and returns the underlying pointer.
///
/// The pointer is valid for the lifetime of the wrapper.
///
/// # Safety
/// The caller must ensure that the pointer is not used after the wrapper is dropped.
pub unsafe fn into_inner(self) -> *mut bindings::socket {
self.0
}

/// Returns the underlying pointer.
///
/// The pointer is valid for the lifetime of the wrapper.
///
/// # Safety
/// The caller must ensure that the pointer is not used after the wrapper is dropped.
pub unsafe fn as_inner(&self) -> *mut bindings::socket {
self.0
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Creating a pointer isn't unsafe (e.g. str::as_ptr). This will clean up nicer after a lifetime is added to the type.

Comment on lines +93 to +103
($(#[$meta:meta])*
$opt:ident = $value:expr,
$level:expr,
unimplemented,
$($tr:ty),*) => {};

($(#[$meta:meta])*
$opt:ident = $value:expr,
$level:expr,
$rtyp:ty,
$($tr:ty),*) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
($(#[$meta:meta])*
$opt:ident = $value:expr,
$level:expr,
unimplemented,
$($tr:ty),*) => {};
($(#[$meta:meta])*
$opt:ident = $value:expr,
$level:expr,
$rtyp:ty,
$($tr:ty),*) => {
(
$(#[$meta:meta])*
$opt:ident = $value:expr,
$level:expr,
unimplemented,
$($tr:ty),*
) => {};
(
$(#[$meta:meta])*
$opt:ident = $value:expr,
$level:expr,
$rtyp:ty,
$($tr:ty),*
) => {

The macro headers are easier to read when indented. I don't think there is any point of having a specific unimplemented matcher that does nothing.

Named fields would make these macros more clear, and you could use repetition:

    impl_ip_opts!{
        /// Don't reserve a port when binding with port number 0.
        ///
        /// C value type: `int`
        BindAddressNoPort {
            value: bindings::IP_BIND_ADDRESS_NO_PORT,
            ctype: bool,
            options: [ReadableOption, WritableOption]
        },
        BlockSource {
            // ..
        }
    }

All that being said, the RFL team sometimes prefer having the code written out than using macros. I could see this going either way :)

#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub struct FlagSet<T: Flag> {
value: isize,
_phantom: core::marker::PhantomData<T>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is isize correct here? This would be ssize_t in C, and it looks like every call to .value() is being casted.

We might want to rethink FlagSet a bit anyway

@vobst vobst unassigned vobst Mar 1, 2024
@micheledallerive
Copy link
Author

@tgross35 @vobst Thanks for the comments, I'm working on the required changes. I am not sure how to commit the changes tho; should I simply commit and then when the PR is ready rebase everything into some patch-like commits or should I already integrate the changes in the previous commits and force-push? If I'm not mistaken the latter could mess up the reviews in Github.

@tgross35
Copy link
Collaborator

tgross35 commented Mar 6, 2024

Feel free to either keep pushing commits or squash and force push however you prefer. GH preserves comments across revisions so no concerns :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants