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

socket::sockopt adding SOL_FILTER level options for illumos. #2611

Merged
merged 3 commits into from
Mar 25, 2025

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Mar 2, 2025

Respectively FIL_ATTACH/FIL_DETACH to bind/unbind a filter to a socket to for example delay connection acceptance until data (datafilt) is received.

devnexen added 2 commits March 2, 2025 07:43
Respectively FIL_ATTACH/FIL_DETACH to bind/unbind a filter
to a socket to for example delay connection acceptance
until data (datafilt) is received.
@devnexen devnexen marked this pull request as ready for review March 2, 2025 09:29
SetOnly,
libc::SOL_FILTER,
libc::FIL_ATTACH,
SetOsString<'static>
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking if this 'static lifetime would be too restrictive? I do not see a way to specify lifetime parameter using the macro, maybe we can implement the nix::sys::socket::SetSockOpt trait manually?

@devnexen devnexen force-pushed the illumos_filter_socket branch from f3c875b to 366ca97 Compare March 24, 2025 14:52
Comment on lines 1493 to 1505
type Val = [u8; libc::FILNAME_MAX as usize];

fn set<F: AsFd>(&self, fd: &F, val: &[u8; libc::FILNAME_MAX as usize]) -> Result<()> {
unsafe {
let res = libc::setsockopt(
fd.as_fd().as_raw_fd(),
libc::SOL_FILTER,
libc::FIL_ATTACH,
val.as_ptr().cast(),
val.len() as libc::socklen_t,
Copy link
Member

Choose a reason for hiding this comment

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

We are passing the whole buffer to the syscall, is this something expected?

BTW, the below code should mirror the original implementation:

#[derive(Clone)]
pub struct FilterAttach<'a> {
    _marker: std::marker::PhantomData<&'a ()>
}

impl<'a> FilterAttach<'a> {
    pub fn new() -> Self {
        Self {
            _marker: std::marker::PhantomData,
        }
    }
}

impl<'a> SetSockOpt for FilterAttach<'a> {
    type Val = &'a OsStr;

    fn set<F: std::os::unix::prelude::AsFd>(&self, fd: &F, val: &Self::Val) -> nix::Result<()> {
        unsafe {
            let res = libc::setsockopt(
                fd.as_fd().as_raw_fd(),
                libc::SOL_FILTER,
                libc::FIL_ATTACH,
                val.as_ptr().cast(),
                val.len() as libc::socklen_t,
            );
            Errno::result(res).map(drop)
        }
    }
}

@devnexen devnexen force-pushed the illumos_filter_socket branch 2 times, most recently from 915fcf8 to 21690ce Compare March 25, 2025 06:10
@@ -20,6 +20,10 @@ use std::os::unix::io::{AsFd, AsRawFd};
#[cfg(feature = "net")]
const TCP_CA_NAME_MAX: usize = 16;

#[cfg(target_os = "illumos")]
#[cfg(feature = "net")]
const FILNAME_MAX: usize = 32;
Copy link
Member

Choose a reason for hiding this comment

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

Please use the constant from the libc crate:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I tried to avoid the cast ;) but ok

@SteveLauC
Copy link
Member

Can we have tests for this?

@devnexen
Copy link
Contributor Author

devnexen commented Mar 25, 2025

Can we have tests for this?

thing is you need to register the filter(s) in the system prior otherwise it will fail (No such file or directory)

e.g. soconfig -F httpf httpf prog 2:2:0,2:2:6,26:2:0,26:2:6 (that is if the kernel module is present)

@devnexen devnexen force-pushed the illumos_filter_socket branch from 21690ce to 92f249d Compare March 25, 2025 07:03
@SteveLauC
Copy link
Member

thing is you need to register the filter(s) in the system prior otherwise it will fail (No such file or directory)

e.g. soconfig -F httpf httpf prog 2:2:0,2:2:6,26:2:0,26:2:6 (that is if the kernel module is present)

Having a test that would fail is also acceptable as we only test the binding/interface, not the libc/kernel implementation

@devnexen devnexen force-pushed the illumos_filter_socket branch 3 times, most recently from b09e24b to 410d712 Compare March 25, 2025 07:26
@SteveLauC
Copy link
Member

SteveLauC commented Mar 25, 2025

Hi, I just realized that the set() function takes a reference to Self::Val, which means we do not need to specify the lifetime parameter&'a ourselves as long as we remove the Sized bound from SetSockOpt::Val:

/// Represents a socket option that can be set.
pub trait SetSockOpt: Clone {
-     type Val: Sized;
+     type Val: ?Sized;

    /// Set the value of this socket option on the given socket.

Then, our socket option impl can be:

#[cfg(target_os = "illumos")]
#[derive(Copy, Clone, Debug)]
/// Attach a named filter to this socket to be able to
/// defer when anough byte had been buffered by the kernel
pub struct FilterAttach;

#[cfg(target_os = "illumos")]
impl SetSockOpt for FilterAttach {
    type Val = OsStr;

    fn set<F: AsFd>(&self, fd: &F, val: &Self::Val) -> Result<()> {
        if val.len() > libc::FILNAME_MAX as usize {
            return Err(Errno::EINVAL);
        }
        unsafe {
            let res = libc::setsockopt(
                fd.as_fd().as_raw_fd(),
                libc::SOL_FILTER,
                libc::FIL_ATTACH,
                val.as_bytes().as_ptr().cast(),
                val.len() as libc::socklen_t,
            );
            Errno::result(res).map(drop)
        }
    }
}

#[cfg(target_os = "illumos")]
#[derive(Copy, Clone, Debug)]
/// Detach a socket filter previously attached with FIL_ATTACH
pub struct FilterDetach;

#[cfg(target_os = "illumos")]
impl SetSockOpt for FilterDetach {
    type Val = OsStr;

    fn set<F: AsFd>(&self, fd: &F, val: &Self::Val) -> Result<()> {
        if val.len() > libc::FILNAME_MAX as usize {
            return Err(Errno::EINVAL);
        }
        unsafe {
            let res = libc::setsockopt(
                fd.as_fd().as_raw_fd(),
                libc::SOL_FILTER,
                libc::FIL_DETACH,
                val.as_bytes().as_ptr().cast(),
                val.len() as libc::socklen_t,
            );
            Errno::result(res).map(drop)
        }
    }
}

Note that I removed your use of to_string_lossy(), instead, we use OsStrExt::as_bytes.

Then, in our tests, note that I documented the reason why we accept ENOENT here, which could help maintenance a lot:

#[cfg(target_os = "illumos")]
#[test]
fn test_solfilter() {
    use nix::errno::Errno;
    let s = socket(
        AddressFamily::Inet,
        SockType::Stream,
        SockFlag::empty(),
        SockProtocol::Tcp,
    )
    .unwrap();
    let data = std::ffi::OsStr::new("httpf");
    let attach = sockopt::FilAttach;
    let detach = sockopt::FilterDetach;

    // These 2 options won't work unless the needed kernel model is installed:
    // https://github.com/nix-rust/nix/pull/2611#issuecomment-2750237782
    //
    // So we only test the binding here
    assert_eq!(Err(Errno::ENOENT), setsockopt(&s, attach, data));
    assert_eq!(Err(Errno::ENOENT), setsockopt(&s, detach, data));
}

@devnexen devnexen force-pushed the illumos_filter_socket branch 2 times, most recently from 69a1c8e to 6d5e426 Compare March 25, 2025 12:00
Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Thanks

@SteveLauC SteveLauC added this pull request to the merge queue Mar 25, 2025
Merged via the queue into nix-rust:master with commit ea012be Mar 25, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants