Re: [PATCH v4 3/9] drivers: add sxe2 basic structures

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri,  1 May 2026 09:59:18 +0800
liujie5@linkdatatechnology.com wrote:

> From: Jie Liu <liujie5@linkdatatechnology.com>
> 
> This patch adds the base infrastructure for the sxe2 common
> library. It includes the mandatory OS abstraction layer (OSAL),
> common structure definitions, error codes, and the logging
> system implementation.
> 
> Specifically, this commit:
>  - Implements the logging stream management using RTE_LOG_LINE.
>  - Defines device-specific error codes and status registers.
>  - Adds the initial meson build configuration for the common library.
> 
> Signed-off-by: Jie Liu <liujie5@linkdatatechnology.com>
> ---
NAK.


NAK.

DPDK drivers must not reinvent their own logging, and they absolutely
must not hijack the log stream that is shared with the application and
the rest of DPDK. This patch does both.

Concrete problems in what you posted:

1. PMD_LOG_NOTICE/WARN/ERR/CRIT/ALERT/EMERG call
   rte_openlog_stream(g_sxe2_common_log_fp) before logging and
   rte_openlog_stream(NULL) after. rte_openlog_stream() sets the
   *global* DPDK log stream for the entire application. A driver has
   no business deciding where the application's logs go. As written,
   this will silently redirect every other PMD's, every library's,
   and the application's own output any time sxe2 emits a message.

2. Those same macros emit the message twice -- once before the stream
   is swapped and once after -- so every NOTICE/WARN/ERR comes out
   duplicated.

3. Drivers must not open files. fopen("/var/log/sxe2pmd.log.<ts>",
   "w+") from inside a PMD is a security problem on every level:

   - It runs with whatever privileges the application has, which for
     DPDK is typically root or CAP_*-loaded. Creating files in
     /var/log under those privileges is a classic symlink / TOCTOU
     attack surface.
   - The path is attacker-influenceable in the timestamp component
     and is not created with O_CREAT|O_EXCL, no mode argument, no
     directory fd, none of the hardening you would expect.
   - Log content is written without any escaping; anything an
     attacker can get into a log message ends up in a file the
     operator will later cat or grep.
   - It bypasses whatever logging policy the operator, distro,
     systemd unit, container runtime, SELinux/AppArmor profile, or
     application has configured. A driver silently writing to
     /var/log is exactly the kind of thing those policies exist to
     prevent.
   - It doesn't work for non-root users, in unprivileged containers,
     on read-only rootfs systems, or on Windows.

   Drivers log through rte_log. The application decides where those
   logs go. Full stop.

4. RTE_LOG_REGISTER_SUFFIX(..., com, DEBUG) defaults the log type to
   DEBUG. The default level is the application's choice, not the
   driver's.

5. SXE2_DPDK_DEBUG is unconditionally defined in
   drivers/common/sxe2/meson.build, so the "debug" path with the file
   hijacking is always on. There is no off switch.

6. SXE2_PMD_LOG adds a thread id, basename, line number, and function
   name to every line. If those are useful, they are useful for every
   driver -- not just yours.

General principles, which apply to every driver:

- Drivers do not reinvent logging. Use RTE_LOG / RTE_LOG_LINE and a
  log type registered for the driver. That is it.
- Drivers do not open files, and do not change logging behavior that
  is shared with the application or with other drivers. Both have
  security implications well beyond this driver.
- If something is genuinely missing from the common logging
  infrastructure -- per-driver log files, richer prefixes, thread
  ids, structured fields, whatever -- propose it as a change to
  lib/log so every driver and every application benefits, and so it
  gets reviewed for security by people who do that for a living. Do
  not bury it inside one driver.
- "It works for our embedded use case" is not an argument. DPDK runs
  on Linux, FreeBSD, and Windows; on x86, Arm, POWER, and RISC-V; in
  bare metal, VMs, and containers; as root and as unprivileged users.
  A driver has to behave reasonably in all of those.
- Once we make an exception for one driver, every subsystem will
  expect one. That is not happening.

Please drop sxe2_common_log.c and sxe2_common_log.h entirely. Use the
existing RTE_LOG_* macros directly against a log type registered for
your driver, and respect the level and stream that the application
has configured.

While you're at it, the same "this driver is a special snowflake"
pattern shows up elsewhere in this series and gets the same answer:

- sxe2_type.h redefines u8/u16/u32/u64/s8/.../__le16/__be32 instead
  of using uintN_t and rte_beN_t.
- sxe2_errno.h reinvents errno values that already exist.
- sxe2_osal.h re-implements bitmap operations, allocation wrappers,
  and an OS abstraction layer that DPDK already provides.

This is the cost of being part of a shared community project across
many platforms and architectures. Please rework on that basis before
reposting.

Stephen

PS: AI revised this text, my initial version was probably
not safe for public consumption.