Skip to content

Commit

Permalink
Replace enumflags2 with bitflags
Browse files Browse the repository at this point in the history
This avoids a proc macro and replaces the BitFlags wrapper type with
directly implementing all methods on AccessFs

This saves about 25% off the 11.5s compiling from scratch took
before this PR on a big machine. This likely has a bigger impact when
compiling as part of a bigger project where there is less available
parallelism to mask the compilation of enumflags2_derive.

Signed-off-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
  • Loading branch information
bjorn3 committed Jan 15, 2023
1 parent 99628b1 commit 6e5a800
Show file tree
Hide file tree
Showing 7 changed files with 173 additions and 171 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ exclude = [".gitignore"]
readme = "README.md"

[dependencies]
enumflags2 = "0.7"
bitflags = "1.3.2"
libc = "0.2.133"
thiserror = "1.0"

Expand Down
8 changes: 4 additions & 4 deletions examples/sandboxer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

use anyhow::{anyhow, bail};
use landlock::{
Access, AccessFs, BitFlags, PathBeneath, PathFd, Ruleset, RulesetAttr, RulesetCreatedAttr,
RulesetStatus, ABI,
Access, AccessFs, PathBeneath, PathFd, Ruleset, RulesetAttr, RulesetCreatedAttr, RulesetStatus,
ABI,
};
use std::env;
use std::ffi::OsStr;
Expand All @@ -17,7 +17,7 @@ const ENV_FS_RW_NAME: &str = "LL_FS_RW";

struct PathEnv {
paths: Vec<u8>,
access: BitFlags<AccessFs>,
access: AccessFs,
}

impl PathEnv {
Expand All @@ -29,7 +29,7 @@ impl PathEnv {
/// allowed. Paths are separated with ":", e.g. "/bin:/lib:/usr:/proc". In case an empty
/// string is provided, NO restrictions are applied.
/// * `access`: Set of access-rights allowed for each of the parsed paths.
fn new<'a>(name: &'a str, access: BitFlags<AccessFs>) -> anyhow::Result<Self> {
fn new<'a>(name: &'a str, access: AccessFs) -> anyhow::Result<Self> {
Ok(Self {
paths: env::var_os(name)
.ok_or(anyhow!("missing environment variable {name}"))?
Expand Down
75 changes: 41 additions & 34 deletions src/access.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
use std::ops::{BitAnd, BitOr};

use crate::{
AccessError, AddRuleError, AddRulesError, BitFlags, CompatError, CompatState, Compatibility,
AccessError, AddRuleError, AddRulesError, CompatError, CompatState, Compatibility,
HandleAccessError, HandleAccessesError, Ruleset, TryCompat, ABI,
};
use enumflags2::BitFlag;

#[cfg(test)]
use crate::{make_bitflags, AccessFs};
use crate::AccessFs;

pub trait Access: PrivateAccess {
/// Gets the access rights defined by a specific [`ABI`].
/// Union of [`from_read()`](Access::from_read) and [`from_write()`](Access::from_write).
fn from_all(abi: ABI) -> BitFlags<Self> {
fn from_all(abi: ABI) -> Self {
// An empty access-right would be an error if passed to the kernel, but because the kernel
// doesn't support Landlock, no Landlock syscall should be called. try_compat() should
// also return RestrictionStatus::Unrestricted when called with unsupported/empty
Expand All @@ -20,17 +21,29 @@ pub trait Access: PrivateAccess {

/// Gets the access rights identified as read-only according to a specific ABI.
/// Exclusive with [`from_write()`](Access::from_write).
fn from_read(abi: ABI) -> BitFlags<Self>;
fn from_read(abi: ABI) -> Self;

/// Gets the access rights identified as write-only according to a specific ABI.
/// Exclusive with [`from_read()`](Access::from_read).
fn from_write(abi: ABI) -> BitFlags<Self>;
fn from_write(abi: ABI) -> Self;
}

pub trait PrivateAccess: BitFlag {
pub trait PrivateAccess: Copy + Eq + BitOr<Output = Self> + BitAnd<Output = Self> {
fn is_empty_flags(self) -> bool
where
Self: Access;

fn all() -> Self
where
Self: Access;

fn known_unknown_flags(self, all: Self) -> (Self, Self)
where
Self: Access;

fn ruleset_handle_access(
ruleset: &mut Ruleset,
access: BitFlags<Self>,
access: Self,
) -> Result<(), HandleAccessesError>
where
Self: Access;
Expand All @@ -44,62 +57,56 @@ pub trait PrivateAccess: BitFlag {
Self: Access;
}

// Creates an illegal/overflowed BitFlags<T> with all its bits toggled, including undefined ones.
fn full_negation<T>(flags: BitFlags<T>) -> BitFlags<T>
where
T: Access,
{
unsafe { BitFlags::<T>::from_bits_unchecked(!flags.bits()) }
}

#[test]
fn bit_flags_full_negation() {
let scoped_negation = !BitFlags::<AccessFs>::all();
assert_eq!(scoped_negation, BitFlags::<AccessFs>::empty());
let scoped_negation = !AccessFs::all();
assert_eq!(scoped_negation, AccessFs::empty());
// !BitFlags::<AccessFs>::all() could be equal to full_negation(BitFlags::<AccessFs>::all()))
// if all the 64-bits would be used, which is not currently the case.
assert_ne!(scoped_negation, full_negation(BitFlags::<AccessFs>::all()));
assert_ne!(scoped_negation.bits(), !AccessFs::all().bits());
}

impl<T> TryCompat<T> for BitFlags<T>
impl<T> TryCompat<T> for T
where
T: Access,
{
fn try_compat(self, compat: &mut Compatibility) -> Result<Self, CompatError<T>> {
let (state, new_access) = if self.is_empty() {
let (_known_flags, unknown_flags) = self.known_unknown_flags(Self::all());
let (state, new_access) = if self.is_empty_flags() {
// Empty access-rights would result to a runtime error.
return Err(AccessError::Empty.into());
} else if !Self::all().contains(self) {
} else if !unknown_flags.is_empty_flags() {
// Unknown access-rights (at build time) would result to a runtime error.
// This can only be reached by using the unsafe BitFlags::from_bits_unchecked().
return Err(AccessError::Unknown {
access: self,
unknown: self & full_negation(Self::all()),
unknown: unknown_flags,
}
.into());
} else {
let compat_bits = self & T::from_all(compat.abi);
if compat_bits.is_empty() {
let (compatible_flags, incompatible_flags) =
self.known_unknown_flags(T::from_all(compat.abi));
if compatible_flags.is_empty_flags() {
if compat.is_best_effort {
// TODO: This creates an empty access-right and could be an issue with
// future ABIs. This method should return Result<Option<Self>,
// CompatError> instead, and in this case Ok(None).
(CompatState::No, compat_bits)
(CompatState::No, compatible_flags)
} else {
return Err(AccessError::Incompatible { access: self }.into());
}
} else if compat_bits != self {
} else if !incompatible_flags.is_empty_flags() {
if compat.is_best_effort {
(CompatState::Partial, compat_bits)
(CompatState::Partial, compatible_flags)
} else {
return Err(AccessError::PartiallyCompatible {
access: self,
incompatible: self & full_negation(compat_bits),
incompatible: incompatible_flags,
}
.into());
}
} else {
(CompatState::Full, compat_bits)
(CompatState::Full, compatible_flags)
}
};
compat.state.update(state);
Expand All @@ -113,22 +120,22 @@ fn compat_bit_flags() {

let mut compat = ABI::V1.into();

let ro_access = make_bitflags!(AccessFs::{Execute | ReadFile | ReadDir});
let ro_access = AccessFs::Execute | AccessFs::ReadFile | AccessFs::ReadDir;
assert_eq!(ro_access, ro_access.try_compat(&mut compat).unwrap());

let empty_access = BitFlags::<AccessFs>::empty();
let empty_access = AccessFs::empty();
assert!(matches!(
empty_access.try_compat(&mut compat).unwrap_err(),
CompatError::Access(AccessError::Empty)
));

let all_unknown_access = unsafe { BitFlags::<AccessFs>::from_bits_unchecked(1 << 63) };
let all_unknown_access = unsafe { AccessFs::from_bits_unchecked(1 << 63) };
assert!(matches!(
all_unknown_access.try_compat(&mut compat).unwrap_err(),
CompatError::Access(AccessError::Unknown { access, unknown }) if access == all_unknown_access && unknown == all_unknown_access
));

let some_unknown_access = unsafe { BitFlags::<AccessFs>::from_bits_unchecked(1 << 63 | 1) };
let some_unknown_access = unsafe { AccessFs::from_bits_unchecked(1 << 63 | 1) };
assert!(matches!(
some_unknown_access.try_compat(&mut compat).unwrap_err(),
CompatError::Access(AccessError::Unknown { access, unknown }) if access == some_unknown_access && unknown == all_unknown_access
Expand Down
23 changes: 7 additions & 16 deletions src/errors.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{Access, AccessFs, BitFlags};
use crate::{Access, AccessFs};
use std::io;
use std::path::PathBuf;
use thiserror::Error;
Expand Down Expand Up @@ -82,10 +82,7 @@ where
AddRuleCall { source: io::Error },
/// The rule's access-rights are not all handled by the (requested) ruleset access-rights.
#[error("access-rights not handled by the ruleset: {incompatible:?}")]
UnhandledAccess {
access: BitFlags<T>,
incompatible: BitFlags<T>,
},
UnhandledAccess { access: T, incompatible: T },
#[error(transparent)]
Compat(#[from] CompatError<T>),
}
Expand Down Expand Up @@ -138,8 +135,8 @@ pub enum PathBeneathError {
/// whereas the file descriptor doesn't point to a directory.
#[error("incompatible directory-only access-rights: {incompatible:?}")]
DirectoryAccess {
access: BitFlags<AccessFs>,
incompatible: BitFlags<AccessFs>,
access: AccessFs,
incompatible: AccessFs,
},
}

Expand All @@ -156,21 +153,15 @@ where
/// The access-rights set was forged with the unsafe `BitFlags::from_bits_unchecked()` and it
/// contains unknown bits.
#[error("unknown access-rights (at build time): {unknown:?}")]
Unknown {
access: BitFlags<T>,
unknown: BitFlags<T>,
},
Unknown { access: T, unknown: T },
/// The best-effort approach was (deliberately) disabled and the requested access-rights are
/// fully incompatible with the running kernel.
#[error("fully incompatible access-rights: {access:?}")]
Incompatible { access: BitFlags<T> },
Incompatible { access: T },
/// The best-effort approach was (deliberately) disabled and the requested access-rights are
/// partially incompatible with the running kernel.
#[error("partially incompatible access-rights: {incompatible:?}")]
PartiallyCompatible {
access: BitFlags<T>,
incompatible: BitFlags<T>,
},
PartiallyCompatible { access: T, incompatible: T },
}

#[derive(Debug, Error)]
Expand Down
Loading

0 comments on commit 6e5a800

Please sign in to comment.