File descriptor safety for program fd + prog attaching #723

nrxus · 2023-08-05T03:52:19Z

This PR is divided into two commits:

Use OwnedFd inside ProgramData to ensure file descriptor safety.
Use AsFd when attaching programs externally and save them as OwnedFd internally.

This PR tries to limit the amount of breaking changes as much as possible, in that the signatures are changing but in practice none or very little changes would be required as a user. This is highlighted by the fact that none of the tests and only one of the doctests had be be touched. The one doctest that was touched was due to the fact that the doctest was explicitely calling for as_raw_fd() on a TcpStream for attaching to a socket filter program but this function now accepts anything that implemented AsFd so the change was just to remove the explicit call to as_raw_fd() and instead pass a reference to the stream directly.

This PRs reveal some existing unsafeties/leaks but does not address them as that would require a larger api change that I'd need some feedback from the maintainers to know what you all would want to do. Namely:

SockMapFd wraps a RawFd but there is no compile time nor runtime check that the file descriptor remains valid when passed back to attaching it to a SkMsg or a SkSkb. This is now highlighted by the fact that SockMapFd now implements AsFd which required use of unsafe { BorrowedFd::borrow_raw }. Adding a lifetime to this struct would fix the issue but it'd require some thinking on how to handle mutably borrowing a Program while borrowing a Map at the same time.
ProgramFd wraps a RawFd but there is no compile time nor runtime check that the file descriptor remains valid when passed back to a method in an aya program. This is now highlighted by the fact that this struct now implement AsFd which required use of unsafe { BorrowedFd::borrow_raw }. Adding a lifetime to this struct would fix the issue but it'd require some thinking on how to handle mutably borrowing two programs at the same time.
LircMode2::query gets a vector of owned file descriptors internally and then returns a list of LircLinks with those file descriptors in them. However, LircLink does not drop its file descriptor on close however because the other way to make it is from LircMode2::attach by re-using the file descriptor from the already loaded program. Possible solutions:
1. We could make LircMode2::attach duplicate the file descriptor so that the link has an OwnedFd that it will close on drop by default. This wouldn't be a breaking change from an API perspective but I am not sure if cloning a file descriptor would be a nono in this case for some reason I don't know about (I don't think it matters much performance-wise fwiw).
2. We could also make ProgramData use an Arc<OwnedFd> instead that we can clone very cheaply but that to me feels like a weird bandaid since in this case there should only be ONE owner and also affecting all programs when this is only an issue for LircMode2 feels like too big of a hammer.
3. We could add a lifetime to LircLink and then have it internally store some sort of Cow<BorrowedFd> thing so it can have an Owned or BorrowedFd based on whether we got it from the attach or the query. I am not sure how LircLinks are used so I don't know if this would turn out awkward to use
4. Completely rework or split query into multiple functions so it can return OwnedFd of the programs separately from making the LircLink.

Relates to: #612

This change is

netlify · 2023-08-05T03:52:30Z

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`6895b1e`
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/64edf99180cfd7000851140e
😎 Deploy Preview	https://deploy-preview-723--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mergify · 2023-08-05T03:52:52Z

Hey @alessandrod, this pull request changes the Aya Public API and requires your review.

tamird

Reviewed 10 of 10 files at r1, 19 of 19 files at r2, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @alessandrod and @nrxus)

-- commits line 15 at r2:
s/descriptors/descriptor/

aya/src/maps/sock/mod.rs line 22 at r2 (raw file):

impl AsFd for SockMapFd {
    fn as_fd(&self) -> BorrowedFd<'_> {
        // SAFETY: We trust that the sock map fd is still valid. TODO: Ensure that

please always TODO(#612)

I think this safety comment is bogus. We should just say this isn't obviously safe, which it seems not to be.

aya/src/programs/cgroup_device.rs line 96 at r2 (raw file):

                    ProgAttachLink::new(
                        prog_fd,
                        cgroup_fd.try_clone_to_owned()?,

do this fallible thing before the attach? what happens if this fails?

aya/src/programs/cgroup_skb.rs line 120 at r2 (raw file):

                .links
                .insert(CgroupSkbLink::new(CgroupSkbLinkInner::ProgAttach(
                    ProgAttachLink::new(prog_fd, cgroup_fd.try_clone_to_owned()?, attach_type),

ditto, this should happen before the attach, I think, and we should probably be using the duplicated fd in the attach call, maybe? what are the detach semantics?

aya/src/programs/cgroup_sock.rs line 95 at r2 (raw file):

                .links
                .insert(CgroupSockLink::new(CgroupSockLinkInner::ProgAttach(
                    ProgAttachLink::new(prog_fd, cgroup_fd.try_clone_to_owned()?, attach_type),

ditto

aya/src/programs/cgroup_sock_addr.rs line 96 at r2 (raw file):

                CgroupSockAddrLinkInner::ProgAttach(ProgAttachLink::new(
                    prog_fd,
                    cgroup_fd.try_clone_to_owned()?,

you know where this is going

aya/src/programs/cgroup_sockopt.rs line 93 at r2 (raw file):

                .links
                .insert(CgroupSockoptLink::new(CgroupSockoptLinkInner::ProgAttach(
                    ProgAttachLink::new(prog_fd, cgroup_fd.try_clone_to_owned()?, attach_type),

hi

aya/src/programs/cgroup_sysctl.rs line 96 at r2 (raw file):

                    ProgAttachLink::new(
                        prog_fd,
                        cgroup_fd.try_clone_to_owned()?,

another

aya/src/programs/lirc_mode2.rs line 111 at r1 (raw file):

                LircLink::new(
                    // SAFETY: The file descriptor will stay valid because we are leaking it
                    // TODO: Do not leak it?

TODO(#612): ...

and maybe write in here why this can't just take an owned fd?

aya/src/programs/lirc_mode2.rs line 77 at r2 (raw file):

        self.data
            .links
            .insert(LircLink::new(prog_fd, lircdev_fd.try_clone_to_owned()?))

another

aya/src/programs/lirc_mode2.rs line 113 at r2 (raw file):

                    // TODO: Do not leak it?
                    unsafe { BorrowedFd::borrow_raw(prog_fd.into_raw_fd()) },
                    target_fd.as_fd().try_clone_to_owned()?,

I think this can be simpler and more efficient:

let prog_ids = query..;
prog_ids.map(|prog_id| {
  let prog_fd = bpf_prog_get_fd_by_id(id)?;
  let target_fd = target_fd.as_fd().try_clone_to_owned()?;
  Ok(LircLink::new(...))
}).collect::<Result<_>>().map_err(Into::into())

aya/src/programs/lirc_mode2.rs line 141 at r2 (raw file):

    /// Get ProgramInfo from this link
    pub fn info(&self) -> Result<ProgramInfo, ProgramError> {
        bpf_prog_get_info_by_fd(unsafe { BorrowedFd::borrow_raw(self.prog_fd) })

safety comment (probably a bogus one with a TODO)

aya/src/programs/mod.rs line 944 at r1 (raw file):

    pub fn fd(&self) -> Result<OwnedFd, ProgramError> {
        let Self(info) = self;
        bpf_prog_get_fd_by_id(info.id).map_err(ProgramError::from)

this can probably be map_err(Into::into) to avoid repeating the type

aya/src/programs/mod.rs line 225 at r2 (raw file):

impl AsFd for ProgramFd {
    fn as_fd(&self) -> BorrowedFd<'_> {
        // SAFETY: We trust that the program fd is still valid. TODO: Enforce that

bogus, needs a reference to issue

aya/src/programs/sk_msg.rs line 91 at r2 (raw file):

            }
        })?;
        let map_fd = map_fd.try_clone_to_owned()?;

ditto

aya/src/programs/sk_skb.rs line 86 at r2 (raw file):

            io_error,
        })?;
        let map_fd = map_fd.try_clone_to_owned()?;

ditto

aya/src/programs/sock_ops.rs line 71 at r2 (raw file):

            }
        })?;
        let cgroup_fd = cgroup_fd.try_clone_to_owned()?;

uh huh

aya/src/sys/bpf.rs line 682 at r1 (raw file):

    if let Err((_, e)) =
        // Uses an invalid target FD so we get EBADF if supported.
        bpf_link_create(fd.as_fd(), -1, bpf_attach_type::BPF_PERF_EVENT, None, 0)

nit: if let ... else { return false } for consistency?

nrxus

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @alessandrod and @tamird)

-- commits line 15 at r2:

Previously, tamird (Tamir Duberstein) wrote…

s/descriptors/descriptor/

Done.

aya/src/maps/sock/mod.rs line 22 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

please always TODO(#612)

I think this safety comment is bogus. We should just say this isn't obviously safe, which it seems not to be.

Done.

aya/src/programs/cgroup_device.rs line 96 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

do this fallible thing before the attach? what happens if this fails?

Done.

aya/src/programs/cgroup_skb.rs line 120 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto, this should happen before the attach, I think, and we should probably be using the duplicated fd in the attach call, maybe? what are the detach semantics?

Done.

aya/src/programs/cgroup_sock.rs line 95 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

Done.

aya/src/programs/cgroup_sock_addr.rs line 96 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

you know where this is going

Done.

aya/src/programs/cgroup_sockopt.rs line 93 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

hi

Done.

aya/src/programs/cgroup_sysctl.rs line 96 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

another

Done.

aya/src/programs/lirc_mode2.rs line 111 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

TODO(#612): ...

and maybe write in here why this can't just take an owned fd?

Done.

aya/src/programs/lirc_mode2.rs line 77 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

another

Done.

aya/src/programs/lirc_mode2.rs line 113 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I think this can be simpler and more efficient:

let prog_ids = query..;
prog_ids.map(|prog_id| {
  let prog_fd = bpf_prog_get_fd_by_id(id)?;
  let target_fd = target_fd.as_fd().try_clone_to_owned()?;
  Ok(LircLink::new(...))
}).collect::<Result<_>>().map_err(Into::into())

Done.

aya/src/programs/lirc_mode2.rs line 141 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

safety comment (probably a bogus one with a TODO)

Done.

aya/src/programs/mod.rs line 944 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this can probably be map_err(Into::into) to avoid repeating the type

Done.

aya/src/programs/mod.rs line 225 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

bogus, needs a reference to issue

Done.

aya/src/programs/sk_msg.rs line 91 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

Done.

aya/src/programs/sk_skb.rs line 86 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

Done.

aya/src/programs/sock_ops.rs line 71 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

uh huh

Done.

tamird

@dave-tucker can you also TAL please?

Reviewed 19 of 19 files at r3, 19 of 19 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alessandrod and @nrxus)

aya/src/programs/lirc_mode2.rs line 103 at r4 (raw file):

                let target_fd = target_fd.as_fd().try_clone_to_owned()?;
                let prog_fd = unsafe { BorrowedFd::borrow_raw(prog_fd.into_raw_fd()) };
                // SAFETY: The file descriptor will stay valid because

shouldn't this be above the unsafe line?

aya/src/programs/mod.rs line 214 at r4 (raw file):

/// A [`Program`] file descriptor.
#[derive(Copy, Clone, Debug)]

should we back this out?

nrxus

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @tamird)

aya/src/programs/lirc_mode2.rs line 103 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

shouldn't this be above the unsafe line?

Done.

aya/src/programs/mod.rs line 214 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

should we back this out?

Can't do that because as part of these changes I made ProgramData's attach_prog_fd be a ProgramFd rather than a RawFd (it is always set from a ProgramFd) and ProgramData already implements Debug.

tamird

Reviewed 19 of 19 files at r6, 19 of 19 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alessandrod and @dave-tucker)

mergify · 2023-08-09T16:48:07Z

@nrxus, this pull request is now in conflict and requires a rebase.

tamird

Reviewed 19 of 19 files at r8, 19 of 19 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod, @dave-tucker, and @nrxus)

aya/src/sys/bpf.rs line 703 at r9 (raw file):

        0,
    ) else {
        return false;

should we crash here? if we ever reach this, we've leaked kernel resources

aya/src/sys/bpf.rs line 755 at r9 (raw file):

    u.prog_type = bpf_prog_type::BPF_PROG_TYPE_SOCKET_FILTER as u32;

    bpf_prog_load(&mut attr).is_ok()

not for this PR: this looks like a resource leak? does this program ever get unloaded? cc @dave-tucker

dave-tucker

Great work as usual on making things safer!

Some comments where you're using .try_clone_to_owned on FDs that are later stored in a Link type would be useful to make it easier to reason around why that's needed.

The few other things I'd like to see in this PR:

always calling try_clone_to_owned on any ProgramFd's that get stored in a Link type. This ensures the program fd can always be closed when the link is disposed of + shouldn't have any weird side-effects around program lifecycle.
Ensuring any pub FD types (like ProgramFd, SockMapFd) wrap OwnedFd, not RawFd - that should prevent the fd we manage from being closed underneath us.
Remove use of RawFd in any LinkID types. It's just a cookie we use to uniquely identify links so they could use i32s instead.

dave-tucker · 2023-08-10T10:56:11Z

aya/src/programs/links.rs

@@ -231,19 +230,19 @@ pub struct ProgAttachLinkId(RawFd, RawFd, bpf_attach_type);
 #[derive(Debug)]
 pub struct ProgAttachLink {
    prog_fd: RawFd,


This could be OwnedFd.
If we were to dup the ProgramFd it would be scoped only to this link.

My concern with duplicating here is that for most use cases what we are doing now is actually perfectly safe and correct: the program data owns both the file descriptor and the links and when the program is unloaded we will drop the links first and then the program fd. I'd be in favor of a separate PR to properly think about how to handle the lifetime of links + linkids when they need the file descriptor of a program.

aya/src/programs/cgroup_skb.rs

aya/src/programs/cgroup_sock.rs

aya/src/programs/cgroup_sock_addr.rs

aya/src/programs/cgroup_sockopt.rs

dave-tucker · 2023-08-10T11:47:35Z

aya/src/programs/lirc_mode2.rs

@@ -118,20 +120,23 @@ pub struct LircLinkId(RawFd, RawFd);
 /// An LircMode2 Link
 pub struct LircLink {
    prog_fd: RawFd,


I think this could be OwnedFd, which would solve your TODOs

Suggested change

prog_fd: RawFd,

prog_fd: OwnedFd,

I am concerned about the inconsistency that'd create since every other link does not have an owned program (it only owns the target). I think we need a separate PR to resolve the file descriptor safety in the program fd's inside links.

aya/src/programs/mod.rs

aya/src/programs/perf_attach.rs

aya/src/programs/socket_filter.rs

dave-tucker · 2023-08-10T13:05:57Z

always calling try_clone_to_owned on any ProgramFd's that get stored in a Link type. This ensures the program fd can always be closed when the link is disposed of + shouldn't have any weird side-effects around program lifecycle.

N.B: In all cases where we're calling try_clone_to_owned() we should add comments...
As an optimization later we might want to come back and replace with BorrowedFd instead - which would likely involve adding lifetimes per the PR description.

tamird · 2023-08-10T18:45:33Z

I took a shallower cut at this problem in #744 and made ProgramFd borrowed from the program.

mergify · 2023-08-10T22:58:27Z

@nrxus, this pull request is now in conflict and requires a rebase.

tamird

Reviewed 1 of 19 files at r10, 23 of 25 files at r12, 8 of 21 files at r13, all commit messages.
Reviewable status: 12 of 27 files reviewed, 22 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @nrxus)

aya/src/programs/lirc_mode2.rs line 109 at r12 (raw file):

                LircLink::new(
                    // SAFETY: The file descriptor will stay valid because we are leaking it
                    // TODO: Do not leak it?

this TODO should reference the issue

aya/src/programs/perf_attach.rs line 103 at r12 (raw file):

    event: Option<ProbeEvent>,
) -> Result<PerfLinkInner, ProgramError> {
    perf_event_ioctl(fd.as_fd(), PERF_EVENT_IOC_SET_BPF, prog_fd.as_raw_fd()).map_err(

these as_fd calls should go away now, yeah?

aya/src/programs/sock_ops.rs line 73 at r12 (raw file):

        })?;
        self.data.links.insert(SockOpsLink::new(ProgAttachLink::new(
            prog_fd.as_fd(),

revert

aya/src/programs/socket_filter.rs line 99 at r12 (raw file):

        self.data.links.insert(SocketFilterLink {
            socket,
            prog_fd: prog_fd.as_raw_fd(),

revert? as_raw_fd already above.

aya/src/programs/xdp.rs line 183 at r12 (raw file):

            XdpLinkInner::FdLink(fd_link) => {
                let link_fd = fd_link.fd;
                bpf_link_update(link_fd.as_fd(), prog_fd.as_fd(), None, 0).map_err(

I think you can revert this line.

aya/src/programs/xdp.rs line 202 at r12 (raw file):

                let replace_flags = flags | XdpFlags::REPLACE;
                unsafe {
                    netlink_set_xdp_fd(if_index, prog_fd, Some(old_prog_fd), replace_flags.bits())

i think you can revert this block. prog_fd.as_fd() is already above.

aya/src/sys/bpf.rs line 703 at r9 (raw file):

Previously, nrxus (Andres) wrote…

I am unsure what you mean by leak kernel resources. If we ever reach this it means that somehow there was a file descriptor for the currently running executable that was u32::MAX (even though I think the maximum file descriptor is i32::MAX?) and that will immediately get closed after this function ends?. I am not sure if links get detached on drop of the returned file descriptor or not though. I think there is a potential for something to go wrong here but crashing seems like a big hammer here. That being said, since it'd be extremely unexpected I am OK with it I suppose if you think that's for the best.

Ah, everything returns OwnedFd here. All good. I've extracted this to another PR; there's a bit too much going here that's not related to the core of what you're trying to do.

#757

mergify · 2023-08-15T20:45:31Z

@nrxus, this pull request is now in conflict and requires a rebase.

nrxus

Reviewable status: 4 of 27 files reviewed, 22 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @tamird)

aya/src/programs/lirc_mode2.rs line 109 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this TODO should reference the issue

It already does. I had added it in the second commit but I'll rebase to put it in the first commit.

aya/src/programs/perf_attach.rs line 103 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

these as_fd calls should go away now, yeah?

Why do you think these can go away? The fd here is an OwnedFd so we need to call as_fd() to get BorrowedFd to pass to perf_event_ioctl.

aya/src/programs/sock_ops.rs line 73 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

revert

Done.

aya/src/programs/socket_filter.rs line 99 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

revert? as_raw_fd already above.

Done.

aya/src/programs/xdp.rs line 183 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I think you can revert this line.

Done.

aya/src/programs/xdp.rs line 202 at r12 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i think you can revert this block. prog_fd.as_fd() is already above.

Done.

tamird

Reviewed 2 of 25 files at r12, 21 of 21 files at r14, 21 of 21 files at r15, all commit messages.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @nrxus)

-- commits line 2 at r14:
I think this commit message has gone stale

aya/src/programs/extension.rs line 89 at r14 (raw file):

    /// original function, see [Extension::detach].
    pub fn attach(&mut self) -> Result<ExtensionLinkId, ProgramError> {
        let prog_fd = self.data.fd.as_ref().ok_or(ProgramError::NotLoaded)?;

Revert this line?

aya/src/programs/perf_attach.rs line 74 at r14 (raw file):

pub(crate) fn perf_attach(
    prog_fd: std::os::fd::BorrowedFd<'_>,

Unnecessary qualification (you removed it in the second commit, but its should not even be in the first commit)

aya/src/programs/socket_filter.rs line 77 at r15 (raw file):

        let prog_fd = self.fd()?;
        let prog_fd = prog_fd.as_fd();
        let socket = socket.as_fd().as_raw_fd();

mind using the same pattern?

let socket = socket.as_fd();
let socket = socket.as_raw_fd();

then a future change might become pure deletion.

aya/src/programs/socket_filter.rs line 97 at r15 (raw file):

        self.data.links.insert(SocketFilterLink {
            socket,
            prog_fd: prog_fd.as_raw_fd(),

revert this diff please

aya/src/sys/bpf.rs line 193 at r14 (raw file):

        u.attach_btf_id = v;
    }

Nit: revert?

aya/src/sys/bpf.rs line 729 at r14 (raw file):

    let Ok(map_fd) = map_data.create("aya_global", None) else { return false };

Revert?

aya/src/sys/bpf.rs line 703 at r15 (raw file):

        fd.as_fd(),
        // Uses an invalid target so we get EBADF if supported.
        LinkTarget::IfIndex(u32::MAX),

reduce the diff here please

nrxus

Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @tamird)

-- commits line 2 at r14:

Previously, tamird (Tamir Duberstein) wrote…

I think this commit message has gone stale

Done.

aya/src/programs/extension.rs line 89 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Revert this line?

Done.

aya/src/programs/perf_attach.rs line 74 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Unnecessary qualification (you removed it in the second commit, but its should not even be in the first commit)

Done.

aya/src/programs/socket_filter.rs line 77 at r15 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

mind using the same pattern?

let socket = socket.as_fd();
let socket = socket.as_raw_fd();

then a future change might become pure deletion.

Done.

aya/src/programs/socket_filter.rs line 97 at r15 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

revert this diff please

Done.

aya/src/sys/bpf.rs line 729 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Revert?

Done.

aya/src/sys/bpf.rs line 703 at r15 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

reduce the diff here please

Done.

tamird

I'd like to let @dave-tucker finish his review.

Reviewed 18 of 20 files at r16, 20 of 20 files at r17, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @nrxus)

aya/src/programs/socket_filter.rs line 77 at r15 (raw file):

Previously, nrxus (Andres) wrote…

Done.

Looks not done

nrxus

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @tamird)

aya/src/programs/socket_filter.rs line 77 at r15 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Looks not done

Done.

nrxus · 2023-08-17T02:53:16Z

@alessandrod @dave-tucker any other changes I need to do before merging this? I am trying to avoid further messy conflicts, these rebases are burning me out.

tamird

Reviewed 1 of 1 files at r18, all commit messages.
Dismissed @dave-tucker from 12 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @nrxus)

aya/src/maps/sock/mod.rs line 25 at r9 (raw file):

Previously, nrxus (Andres) wrote…

I could but I think I'd rather find a way to not force file descriptor dup for the common cases. I think a follow up PR to this is warranted to address these pre-existing issues as they'd require some API change that I think would need a larger discussion and I don't want to balloon this PR further.

This might be straightforward now that #744 is landed.

aya/src/programs/socket_filter.rs line 77 at r15 (raw file):

Previously, nrxus (Andres) wrote…

Done.

Looks like it's in the wrong commit, ending with more churn rather than less.

tamird

Reviewed 26 of 26 files at r19, 20 of 20 files at r20, 2 of 2 files at r21, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alessandrod and @dave-tucker)

alessandrod

Solid work! See minor thing about generated type name, otherwise looks good to go

alessandrod · 2023-08-29T12:40:29Z

aya/src/sys/bpf.rs

-    attr.link_create.__bindgen_anon_2.target_fd = target_fd as u32;
+    attr.link_create.__bindgen_anon_1.prog_fd = prog_fd.as_raw_fd() as u32;
+    attr.link_create.__bindgen_anon_2 = match target {
+        LinkTarget::Fd(fd) => bpf_attr__bindgen_ty_14__bindgen_ty_2 {


Don't spell out the type name, bindgen types aren't stable, so this is going to
break when bindings are regenerated. Instead use MaybeUninit::zeroed and fill in place.

This commit reveals but does not address a file descriptor leak in LircLink2::query. This function returns a list of `LircLink`s where each of them have a program file descriptor that is not going to be closed. This commit does not add this leak; it merely makes it louder in the code.

This is a breaking change but adds another level of safety to ensure the file descriptor we receive is valid. Additionally, this allows aya to internally easily duplicate this file descriptor using std library methods instead of manually calling `dup` which doesn't duplicate with the CLOSE_ON_EXEC flag that is standard pratice to avoid leaking the file descriptor when exec'ing.

The signature of attach changed in aya-rs/aya#723.

mergify bot requested a review from alessandrod August 5, 2023 03:52

mergify bot added api/needs-review Makes an API change that needs review aya This is about aya (userspace) test A PR that improves test cases or CI labels Aug 5, 2023

nrxus force-pushed the map-program-owned-fd branch 2 times, most recently from 18f545f to aad5870 Compare August 5, 2023 03:57

tamird requested changes Aug 5, 2023

View reviewed changes

nrxus force-pushed the map-program-owned-fd branch from aad5870 to 0445c9c Compare August 5, 2023 23:39

nrxus commented Aug 5, 2023

View reviewed changes

nrxus requested a review from tamird August 5, 2023 23:49

tamird approved these changes Aug 7, 2023

View reviewed changes

tamird requested a review from dave-tucker August 7, 2023 23:54

nrxus force-pushed the map-program-owned-fd branch from 0445c9c to 6d27919 Compare August 8, 2023 23:28

nrxus commented Aug 8, 2023

View reviewed changes

nrxus force-pushed the map-program-owned-fd branch from 6d27919 to 86b899e Compare August 8, 2023 23:31

tamird approved these changes Aug 8, 2023

View reviewed changes

mergify bot added the needs-rebase label Aug 9, 2023

nrxus force-pushed the map-program-owned-fd branch from 86b899e to 13cea72 Compare August 10, 2023 07:25

mergify bot removed the needs-rebase label Aug 10, 2023

tamird reviewed Aug 10, 2023

View reviewed changes

dave-tucker requested changes Aug 10, 2023

View reviewed changes

mergify bot added the needs-rebase label Aug 10, 2023

nrxus force-pushed the map-program-owned-fd branch from 13cea72 to 6228f2d Compare August 11, 2023 03:49

mergify bot removed the needs-rebase label Aug 11, 2023

tamird reviewed Aug 15, 2023

View reviewed changes

mergify bot added the needs-rebase label Aug 15, 2023

nrxus force-pushed the map-program-owned-fd branch from f15d544 to 3dbfef1 Compare August 16, 2023 02:22

mergify bot removed the needs-rebase label Aug 16, 2023

nrxus force-pushed the map-program-owned-fd branch from 3dbfef1 to 97d7745 Compare August 16, 2023 02:30

nrxus commented Aug 16, 2023

View reviewed changes

nrxus requested a review from tamird August 16, 2023 02:52

tamird requested changes Aug 16, 2023

View reviewed changes

nrxus force-pushed the map-program-owned-fd branch 3 times, most recently from d2a7720 to 310a82c Compare August 17, 2023 01:21

nrxus commented Aug 17, 2023

View reviewed changes

nrxus requested a review from tamird August 17, 2023 01:27

tamird approved these changes Aug 17, 2023

View reviewed changes

nrxus force-pushed the map-program-owned-fd branch from 310a82c to a845bc2 Compare August 17, 2023 02:51

nrxus commented Aug 17, 2023

View reviewed changes

tamird requested changes Aug 17, 2023

View reviewed changes

tamird force-pushed the map-program-owned-fd branch 2 times, most recently from c4fc176 to 16db4c8 Compare August 24, 2023 15:43

tamird approved these changes Aug 24, 2023

View reviewed changes

alessandrod reviewed Aug 29, 2023

View reviewed changes

nrxus added 2 commits August 29, 2023 09:55

tamird force-pushed the map-program-owned-fd branch from 16db4c8 to 6895b1e Compare August 29, 2023 13:58

tamird merged commit c4643b3 into aya-rs:main Aug 29, 2023
21 checks passed

tamird added a commit to aya-rs/aya-template that referenced this pull request Sep 5, 2023

socket_filter: fix build

d9b1506

The signature of attach changed in aya-rs/aya#723.

tamird mentioned this pull request Sep 5, 2023

socket_filter: fix build aya-rs/aya-template#101

Merged

tamird added a commit to aya-rs/aya-template that referenced this pull request Sep 5, 2023

socket_filter: fix build

1247fde

The signature of attach changed in aya-rs/aya#723.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File descriptor safety for program fd + prog attaching #723

File descriptor safety for program fd + prog attaching #723

nrxus commented Aug 5, 2023 •

edited

Loading

netlify bot commented Aug 5, 2023 •

edited

Loading

mergify bot commented Aug 5, 2023

tamird left a comment

nrxus left a comment

tamird left a comment

nrxus left a comment

tamird left a comment

mergify bot commented Aug 9, 2023

tamird left a comment

dave-tucker left a comment

dave-tucker Aug 10, 2023

nrxus Aug 12, 2023

dave-tucker Aug 10, 2023

nrxus Aug 12, 2023

dave-tucker commented Aug 10, 2023

tamird commented Aug 10, 2023

mergify bot commented Aug 10, 2023

tamird left a comment

mergify bot commented Aug 15, 2023

nrxus left a comment

tamird left a comment

nrxus left a comment

tamird left a comment

nrxus left a comment

nrxus commented Aug 17, 2023

tamird left a comment

tamird left a comment

alessandrod left a comment

alessandrod Aug 29, 2023

tamird Aug 29, 2023

File descriptor safety for program fd + prog attaching #723

File descriptor safety for program fd + prog attaching #723

Conversation

nrxus commented Aug 5, 2023 • edited Loading

netlify bot commented Aug 5, 2023 • edited Loading

✅ Deploy Preview for aya-rs-docs ready!

mergify bot commented Aug 5, 2023

tamird left a comment

Choose a reason for hiding this comment

nrxus left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

nrxus left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 9, 2023

tamird left a comment

Choose a reason for hiding this comment

dave-tucker left a comment

Choose a reason for hiding this comment

dave-tucker Aug 10, 2023

Choose a reason for hiding this comment

nrxus Aug 12, 2023

Choose a reason for hiding this comment

dave-tucker Aug 10, 2023

Choose a reason for hiding this comment

nrxus Aug 12, 2023

Choose a reason for hiding this comment

dave-tucker commented Aug 10, 2023

tamird commented Aug 10, 2023

mergify bot commented Aug 10, 2023

tamird left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 15, 2023

nrxus left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

nrxus left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

nrxus left a comment

Choose a reason for hiding this comment

nrxus commented Aug 17, 2023

tamird left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

alessandrod left a comment

Choose a reason for hiding this comment

alessandrod Aug 29, 2023

Choose a reason for hiding this comment

tamird Aug 29, 2023

Choose a reason for hiding this comment

nrxus commented Aug 5, 2023 •

edited

Loading

netlify bot commented Aug 5, 2023 •

edited

Loading