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

Don't unwrap when removing invalid TCP state from fastpath. #621

Merged
merged 3 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 14 additions & 3 deletions lib/opte-ioctl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,8 @@ where

// It would be a shame if the command failed and we didn't have enough bytes
// to serialize the error response, so we set this to default to 16 KiB.
let mut resp_buf = Vec::with_capacity(16 * 1024);
const BASE_CAPACITY: usize = 16 * 1024;
let mut resp_buf = Vec::with_capacity(BASE_CAPACITY);
let mut rioctl = OpteCmdIoctl {
api_version: API_VERSION,
cmd,
Expand Down Expand Up @@ -313,8 +314,18 @@ where
if raw_err == libc::ENOBUFS {
assert!(rioctl.resp_len_actual != 0);
assert!(rioctl.resp_len_actual > resp_buf.capacity());
// Make room for the size the kernel claims to need
resp_buf.reserve(rioctl.resp_len_actual - resp_buf.capacity());

// Make room for at least the size the kernel claims to need.
// This can be slightly tricky: since every retry reruns the
// command, the size of the next resp could change from under
// us (increase or decrease). Keep some headroom to account
// for this.
let wanted_capacity =
BASE_CAPACITY / 4 + rioctl.resp_len_actual;

// XDE could write into `resp_buf` (but does not).
// .len() *should* be zero -- but don't bank on it.
resp_buf.reserve(wanted_capacity - resp_buf.len());
rioctl.resp_bytes = resp_buf.as_mut_ptr();
rioctl.resp_len = resp_buf.capacity();
rioctl.resp_len_actual = 0;
Expand Down
14 changes: 12 additions & 2 deletions lib/opte/src/engine/port.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1395,9 +1395,19 @@ impl<N: NetworkImpl> Port<N> {
let ufid_out = &flow_lock.outbound_ufid;

let ufid_in = flow_lock.inbound_ufid.as_ref();
self.uft_tcp_closed(&mut local_lock, ufid_out, ufid_in);

let _ = local_lock.tcp_flows.remove(ufid_out).unwrap();
// Because we've dropped the port lock, another packet could have also
// invalidated this flow and removed the entry. It could even install
// new UFT/TCP entries, depending on lock/process ordering.
//
// Verify that the state we want to remove still exists, and is
// `Arc`-identical.
if let Some(found_entry) = local_lock.tcp_flows.get(ufid_out) {
if Arc::ptr_eq(found_entry, &entry) {
self.uft_tcp_closed(&mut local_lock, ufid_out, ufid_in);
_ = local_lock.tcp_flows.remove(ufid_out);
}
}
Comment on lines +1405 to +1410
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's ok to swap the order here, you could make use of the Entry API to lookup and remove the tcp flow to save on searching through it twice:

Suggested change
if let Some(found_entry) = local_lock.tcp_flows.get(ufid_out) {
if Arc::ptr_eq(found_entry, &entry) {
self.uft_tcp_closed(&mut local_lock, ufid_out, ufid_in);
_ = local_lock.tcp_flows.remove(ufid_out);
}
}
if let Entry::Occupied(found_entry) = local_lock.tcp_flows.entry(*ufid_out) {
if Arc::ptr_eq(found_entry.get(), &entry) {
_ = found_entry.remove_entry();
self.uft_tcp_closed(&mut local_lock, ufid_out, ufid_in);
}
}

Copy link
Collaborator Author

@FelixMcFelix FelixMcFelix Dec 3, 2024

Choose a reason for hiding this comment

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

Thanks a lot for the review Luqman.

I think this would be a good idea (the swap is valid as I read it), except local_lock.tcp_flows here is a FlowTable and not a BTreeMap. It'd be nice to forward the Entry API while respecting the capacity constraints in FlowTable::add etc. to enable that. I'll open a ticket.

EDIT: #627.


if reprocess {
lock = Some(local_lock);
Expand Down