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.

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

sys: bpf_prog_get_fd_by_id returns OwnedFd #688

Merged
merged 1 commit into from
Jul 26, 2023
Merged

sys: bpf_prog_get_fd_by_id returns OwnedFd #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions aya/src/programs/lirc_mode2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//! Lirc programs.
use std::os::fd::{AsRawFd, RawFd};
use std::os::fd::{AsRawFd, IntoRawFd as _, RawFd};

use crate::{
generated::{bpf_attach_type::BPF_LIRC_MODE2, bpf_prog_type::BPF_PROG_TYPE_LIRC_MODE2},
Expand Down Expand Up @@ -101,12 +101,12 @@ impl LircMode2 {
io_error,
})?;

prog_fds.push(fd as RawFd);
prog_fds.push(fd);
}

Ok(prog_fds
.into_iter()
.map(|prog_fd| LircLink::new(prog_fd, target_fd.as_raw_fd()))
.map(|prog_fd| LircLink::new(prog_fd.into_raw_fd(), target_fd.as_raw_fd()))
Copy link
Contributor

Choose a reason for hiding this comment

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

@tamird

I know this is already merged but the OwnedFd is dropped here since there are no other usages of OwnedFd and thus the file descriptor is closed and yet we are creating a LircLink with this closed file descriptor. LircLink has a method info that uses this file descriptor to get its program information but that file descriptor is now closed. I don't really use lirc probes so I am not how how to test/prove that but I figured it's worth calling out that this PR most likely broke getting the program information from a queried list of lirc programs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Derp nvm, I thought we were calling for as_raw_fd. Ignore

.collect())
}
}
Expand Down
10 changes: 4 additions & 6 deletions aya/src/programs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ use libc::ENOSPC;
use std::{
ffi::CString,
io,
os::unix::io::{AsRawFd, RawFd},
os::fd::{AsRawFd, IntoRawFd as _, RawFd},
path::{Path, PathBuf},
};
use thiserror::Error;
Expand Down Expand Up @@ -955,7 +955,7 @@ impl ProgramInfo {
call: "bpf_prog_get_fd_by_id",
io_error,
})?;
Ok(fd as RawFd)
Ok(fd.into_raw_fd())
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is another place that this PR probably broke things. The fd returned here is already closed because the OwnedFd is dropped so anyone calling for this fd is getting a bad file descriptor. Additionally, the comment of this method no longer makes sense:

/// The returned fd must be closed when no longer needed.

I think this would be a quick fix though, we just gotta make this function return an OwnedFd instead and remove the comment since OwnedFds are closed on drop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Derp nvm, I thought we were calling for as_raw_fd. Ignore

}

/// Loads a program from a pinned path in bpffs.
Expand Down Expand Up @@ -1003,14 +1003,12 @@ impl Iterator for ProgramsIter {
io_error,
})
.and_then(|fd| {
let info = bpf_prog_get_info_by_fd(fd)
bpf_prog_get_info_by_fd(fd.as_raw_fd())
.map_err(|io_error| ProgramError::SyscallError {
call: "bpf_prog_get_info_by_fd",
io_error,
})
.map(ProgramInfo);
unsafe { libc::close(fd) };
info
.map(ProgramInfo)
}),
)
}
Expand Down
16 changes: 13 additions & 3 deletions aya/src/sys/bpf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{
ffi::{CStr, CString},
io,
mem::{self, MaybeUninit},
os::unix::io::RawFd,
os::fd::{FromRawFd as _, OwnedFd, RawFd},
slice,
};

Expand Down Expand Up @@ -460,13 +460,23 @@ pub(crate) fn bpf_prog_query(
ret
}

pub(crate) fn bpf_prog_get_fd_by_id(prog_id: u32) -> Result<RawFd, io::Error> {
pub(crate) fn bpf_prog_get_fd_by_id(prog_id: u32) -> Result<OwnedFd, io::Error> {
let mut attr = unsafe { mem::zeroed::<bpf_attr>() };

attr.__bindgen_anon_6.__bindgen_anon_1.prog_id = prog_id;

match sys_bpf(bpf_cmd::BPF_PROG_GET_FD_BY_ID, &attr) {
Ok(v) => Ok(v as RawFd),
Ok(v) => {
let v = v.try_into().map_err(|_err| {
// _err is std::num::TryFromIntError or std::convert::Infallible depending on
// target, so we can't ascribe.
io::Error::new(
io::ErrorKind::InvalidData,
format!("bpf_prog_get_fd_by_id: invalid fd returned: {}", v),
)
})?;
Ok(unsafe { OwnedFd::from_raw_fd(v) })
}
Err((_, err)) => Err(err),
}
}
Expand Down