Skip to content

Conversation

@nawaz1991
Copy link

When using unified ring_fd polling, VMA auto-accepts connections. If a client sends data immediately after connect, the packet can arrive before m_p_rx_ring is initialized, causing a NULL pointer dereference in rx_input_cb() / fill_completion().

Fix by deriving the ring from the packet's descriptor owner when m_p_rx_ring is NULL. The descriptor owner is always valid as it points to the ring that received the packet.

Fixes: #1153

Description

What

Fix SocketXtreme NULL pointer dereference in rx_input_cb() (TCP) and fill_completion() (UDP) when a packet arrives for an auto-accepted socket whose m_p_rx_ring has not been initialized yet.

Why

With unified ring_fd polling, VMA auto-accepts connections. If the client sends data immediately after connect, CQ processing can invoke rx_input_cb() before m_p_rx_ring is set:

m_socketxtreme.completion = m_p_rx_ring->get_comp(); // m_p_rx_ring == NULL -> SIGSEGV

How

Derive the ring from the packet's descriptor owner on the rare uninitialized path:

if (unlikely(!m_p_rx_ring)) {
    m_p_rx_ring = p_rx_pkt_mem_buf_desc_info->p_desc_owner; // TCP
    // or p_desc->p_desc_owner for UDP
}
m_socketxtreme.completion = m_p_rx_ring->get_comp();

The descriptor owner always points to the ring that received the packet, providing a valid fallback. The unlikely() hint ensures no hot-path performance impact.

Change Type

  • Bugfix
  • Feature
  • Code style update
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

Checklist

  • Code follows the style guidelines of this project
  • Comments have been inserted in hard to understand places
  • Documentation has been updated (if necessary)
  • Test has been added (if possible)

Files Changed

File Change
src/vma/sock/sockinfo_tcp.cpp Add NULL check for m_p_rx_ring in rx_input_cb()
src/vma/sock/sockinfo_udp.cpp Add NULL check for m_p_rx_ring in fill_completion()

Testing

Environment:

  • Rocky Linux 9.6 (kernel 5.14.0-570.49.1.el9_6.x86_64)
  • MLNX_OFED_LINUX-24.10-3.2.5.0
  • ConnectX-6 Lx (25GbE)
  • VMA_SOCKETXTREME=1

Results:

Test Before (system libvma) After (patched)
Standalone repro: server + multi-threaded client SIGSEGV in rx_input_cb() within seconds No crash, runs indefinitely
Stress test: 4 client threads, rapid connect+send Crashes on first burst Packets delivered correctly

Reproduction

Standalone reproduction code available with issue reported as #1153

# Server (will crash with unpatched VMA)
sudo LD_PRELOAD=/usr/lib64/libvma.so \
    VMA_SOCKETXTREME=1 VMA_RX_POLL=-1 VMA_TRACELEVEL=-1 \
    taskset -c <CPU> ./server <IP> <PORT>

# Client (kernel TCP - no VMA needed, ~900 conn/s)
taskset -c <CPU> ./client <IP> <PORT>

Key conditions:

  • Server uses unified ring_fd from listener (get_socket_rings_fds)
  • Server does NOT initialize accepted sockets before next poll
  • Client sends data immediately after connect completes
  • Kernel TCP client recommended for higher throughput (~9x faster than VMA client)

When using unified ring_fd polling, VMA auto-accepts connections.
If a client sends data immediately after connect, the packet can
arrive before m_p_rx_ring is initialized, causing a NULL pointer
dereference in rx_input_cb() / fill_completion().

Fix by deriving the ring from the packet's descriptor owner when
m_p_rx_ring is NULL. The descriptor owner is always valid as it
points to the ring that received the packet.

Fixes: Mellanox#1153
Signed-off-by: Muhammad Nawaz <m.nawaz2003@gmail.com>
@svc-nbu-swx-media
Copy link

Can one of the admins verify this patch?

@greptile-apps
Copy link

greptile-apps bot commented Dec 5, 2025

Greptile Overview

Greptile Summary

Fixes NULL pointer dereference crash in SocketXtreme mode when packets arrive for auto-accepted connections before m_p_rx_ring is initialized. The fix derives the ring pointer from the packet descriptor's p_desc_owner field, which is always valid and points to the ring that received the packet.

Key Changes:

  • sockinfo_tcp.cpp:1878 - Adds unlikely(!m_p_rx_ring) guard in rx_input_cb() before calling get_comp()
  • sockinfo_udp.cpp:1771 - Adds identical guard in fill_completion() for UDP consistency
  • Uses unlikely() branch hint to avoid performance impact on hot path
  • Protected by existing locks (lock_tcp_con() for TCP, called from locked context for UDP)

Technical Correctness:

  • p_desc_owner is always set during buffer allocation (src/vma/dev/buffer_pool.cpp:158) and points to the ring_slave that owns the descriptor
  • The assignment m_p_rx_ring = p_desc_owner is safe because it occurs within existing lock protection
  • Once set, m_p_rx_ring remains valid for the socket's lifetime (only reassigned during ring migration, also under lock)
  • The fix matches VMA's existing pattern for m_p_rx_ring initialization (see src/vma/sock/sockinfo.cpp:939,1149)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk - fixes critical crash with correct fallback logic
  • Score reflects well-understood race condition with proven fix: adds defensive NULL check using guaranteed-valid fallback (descriptor owner always points to receiving ring), includes comprehensive reproduction case and testing, uses appropriate performance hint (unlikely), follows existing codebase patterns for ring initialization, and has proper thread safety (existing lock protection)
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/vma/sock/sockinfo_tcp.cpp 5/5 Adds NULL check for m_p_rx_ring in rx_input_cb() (line 1878), deriving ring from p_desc_owner when uninitialized - fixes crash in auto-accepted connections
src/vma/sock/sockinfo_udp.cpp 5/5 Adds NULL check for m_p_rx_ring in fill_completion() (line 1771), deriving ring from p_desc->p_desc_owner when uninitialized - mirrors TCP fix for consistency

Sequence Diagram

sequenceDiagram
    participant Client
    participant Listener as Listener Socket
    participant Ring as ring_fd (unified)
    participant NewSock as New TCP Socket
    participant CQ as Completion Queue

    Note over Client,CQ: Race Condition Timeline

    Client->>Listener: SYN packet
    Listener->>Client: SYN-ACK
    Client->>Listener: ACK (connection established)
    
    Note over Ring: socketxtreme_poll() on ring_fd
    Ring->>Listener: VMA_SOCKETXTREME_NEW_CONNECTION_ACCEPTED
    Listener->>NewSock: Create sockinfo_tcp (m_p_rx_ring = NULL)
    
    Note over Client: Client sends data immediately
    Client->>CQ: Data packet arrives
    CQ->>Ring: Packet queued
    
    Note over Ring: socketxtreme_poll() continues processing
    Ring->>NewSock: rx_input_cb(desc) called
    
    alt Without Fix (CRASH)
        NewSock->>NewSock: m_p_rx_ring->get_comp()
        Note over NewSock: SIGSEGV! m_p_rx_ring is NULL
    end
    
    alt With Fix (SAFE)
        NewSock->>NewSock: if (!m_p_rx_ring)
        NewSock->>NewSock: m_p_rx_ring = desc->p_desc_owner
        NewSock->>NewSock: m_p_rx_ring->get_comp() ✓
        Note over NewSock: No crash, ring derived from descriptor
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@nawaz1991
Copy link
Author

@galnoam, please assign reviewer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SocketXtreme: SIGSEGV in rx_input_cb() due to NULL m_p_rx_ring on auto-accepted connections

2 participants