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

DemiBuffer use of data_len/pkt_len is inconsistent w.r.t. DPDK creates UB #1169

Open
kyleholohan opened this issue Mar 6, 2024 · 0 comments
Labels
enhancement Enhancement Request on an Existing Feature

Comments

@kyleholohan
Copy link
Contributor

Context

When creating a new, empty DemiBuffer via DemiBuffer::new, the constructor initializes data_len and pkt_len to buf_len (all fields of MetaData). This area has a few related issues:

  • The Deref implementation creates a slice containing all DemiBuffer bytes in the range [0, data_len). Dereferencing a new DemiBuffer is undefined behavior, as it violates the safety requirements of slice::from_raw_parts, namely (emphasis added):

    data must point to len consecutive properly initialized values of type T.

    and from the GlobalAlloc::alloc documentation:

    The allocated block of memory may or may not be initialized.

  • This behavior is inconsistent with DPDK. The closest equivalent function is probably rte_pktmbuf_alloc, which initializes buf_len depending on the memory pool but data_len and pkt_len are initially zero. The expected workflow here is to allocate an mbuf from the pool, then use method such as rte_pktmbuf_append as bytes in the packet are initialized.

  • Network transports used by LibOSes such as Catnap accept a DemiBuffer in their pop calls (e.g., in src\rust\catnap\win\transport.rs, SharedCatnapTransport::pop). These methods depend on DemiBuffer::len() method to determine how much data they may fill, however, as noted above, this relies on data_len, which should indicate the number of initialized/used bytes. Currently, these LibOSes rely on the UB described above.

Proposed Solution

Make the following changes:

  • Initialize the data_len and pkt_len fields of MetaData to zero for new (i.e., not cloned/copied) DemiBuffers.
  • Add a buffer_len() or equivalent method to DemiBuffer, exposing the buf_len MetaData field.
  • Add append and prepend methods for manipulating the DemiBuffer length and content. There are many considerations here:
    • Ideally, these would be safe methods. The naive way to accomplish this would use copies (e.g., append(content: &[u8]) -> copy content into the buffer); however, this is not a great solution for performance-sensitive applications.
    • The unstable extension core_io_borrowed_buf offers an interesting idiom here. Borrowing a slice of [MaybeUninit] as a BorrowedBuf could offer a safe idiom as follows (not taking into account chaining, lifetimes elided):
      pub fn append<F: FnOnce(BorrowedBuf) -> BorrowedBuf>(&mut self, fn: F) {
          let buf = fn(BorrowedBuf::from(unsafe { slice::from_raw_parts_mut::<MaybeUninit<u8>>(self.data_ptr().cast().offset(self.len()), self.buffer_len()) }));
          let metadata: &mut MetaData = self.as_metadata();
          metadata.data_len += buf.init_len();
          metadata.pkt_len += buf.init_len();
      }
    • Expose a series of unsafe functions which allow direct manipulation of the buffer and/or length. Require the user to initialize or indicate initialization correctly. This is likely the most performant/flexible approach.
    • Combination of the above.

Alternative Solutions

Alternatively, DemiBuffer might just initialize all bytes upfront. This still breaks consistency with DPDK, but provides a very simple solution which doesn't break anything else. Conceptually, DemiBuffer still lacks counterpart methods for trim and adjust, so this approach likely just postpones a real solution here.

@kyleholohan kyleholohan added the enhancement Enhancement Request on an Existing Feature label Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement Request on an Existing Feature
Projects
None yet
Development

No branches or pull requests

1 participant