DPDK-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Hemminger <stephen@networkplumber.org>
To: Anurag Mandal <anurag.mandal@intel.com>
Cc: dev@dpdk.org, ciara.loftus@intel.com, mtahhan@redhat.com,
	stable@dpdk.org
Subject: Re: [PATCH] net/af_xdp: fix default device plugin path
Date: Mon, 10 Aug 2026 08:28:07 -0700	[thread overview]
Message-ID: <20260810082807.33fc8a4a@phoenix.local> (raw)
In-Reply-To: <20260810101158.1011356-1-anurag.mandal@intel.com>

On Mon, 10 Aug 2026 10:11:58 +0000
Anurag Mandal <anurag.mandal@intel.com> wrote:

> When use_cni=1 or use_pinned_map=1 is passed without an explicit
> dp_path, the PMD builds the path to the AF_XDP device plugin
> endpoint itself.
> That default was hardcoded to /tmp/afxdp_dp/<if_name>/.
> 
> A unix domain socket and a pinned map are runtime state.
> Current Linux file hierarchy conventions place these below
> the runtime directory, not below world writable /tmp.
> DPDK already exposes such a location through
> rte_eal_get_runtime_dir().
> 
> The location cannot simply be moved as it is a contract
> duplicated in the AF_XDP Device Plugin for Kubernetes,
> which creates the socket and mounts it into the pod at
> the /tmp path the PMD expects.
> Changing only the PMD would break every existing deployment.
> 
> This patch fixes the default so that the endpoint is looked
> up below the EAL runtime directory first, and fall back to
> /tmp/afxdp_dp only when nothing is found there, a warning
> is logged whenever that fallback is taken.
> Existing plugin deployments keep working unchanged, while
> a deployment, that places the endpoint in the runtime
> directory, no longer has to pass dp_path.
> An explicit dp_path still overrides both.
> 
> The <if_name> component is kept in both layouts. It is what
> keeps the endpoints of several interfaces distinct when more
> than one UDS server is mounted in a single pod.
> 
> The runtime directory prefix makes the built path longer,
> so reject a dp_path that does not fit the sun_path field
> of struct sockaddr_un instead of letting strlcpy()
> silently truncate it.
> 
> The interface name check is moved ahead of the path
> construction, as the default path is now built from it.
> 
> Bugzilla ID: 1973
> Fixes: 9c1323736cf9 ("net/af_xdp: fix multi-interface support for k8s")
> Fixes: 8a324b1c6464 ("net/af_xdp: support AF_XDP device plugin pinned maps")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Anurag Mandal <anurag.mandal@intel.com>
> ---

Don't think changing path should go to stable.

AI review had overly wordy response. But does make good points
that need to be looked at.

Applies cleanly to main (drivers/net/af_xdp/rte_eth_af_xdp.c,
doc/guides/nics/af_xdp.rst, doc/guides/howto/af_xdp_dp.rst).

No correctness bugs found in the C changes. RTE_SIZEOF_FIELD, unistd.h,
sys/un.h and rte_eal.h are all already available in the file; kvlist is
freed unconditionally inside parse_parameters(), so the two new early
returns in probe() do not leak it; snprintf truncation is checked on both
branches; the moved if_name check lands immediately after
parse_parameters(), which is correct.

The findings below are documentation and process.

Warning: doc/guides/howto/af_xdp_dp.rst - stale note contradicts the patch

  The patch rewrites the paragraph at line 70 but leaves the third note
  block untouched. Post-patch the file reads:

     "It looks for afxdp_dp/<if_name>/afxdp.sock ... below the EAL runtime
      directory ... When no such entry exists, the PMD falls back to ..."

  and then, twenty lines later:

     "In these versions of the PMD if a user doesn't explicitly set the
      dp_path parameter when using use_cni then that path is transparently
      configured in the AF_XDP PMD to the default AF_XDP Device Plugin for
      Kubernetes mount point path."

  The second statement is now wrong. Either fold it into the new text or
  reword it to say the legacy mount point is the fallback.

Warning: doc/guides/nics/af_xdp.rst - line exceeds 100 characters

  The new use_pinned_map example is 103 characters:

    --vdev=net_af_xdp0,use_pinned_map=1,dp_path="/var/run/dpdk/rte/afxdp_dp/<<interface name>>/xsks_map"

  The line it replaces was 88. checkpatches.sh will flag this. The use_cni
  example is 98 and is fine.

Warning: doc/guides/nics/af_xdp.rst - /var/run/dpdk/rte is not the runtime
directory in general

  Both examples now hardcode /var/run/dpdk/rte. eal_create_runtime_dir()
  builds <base>/dpdk/<file-prefix>, where <base> is RUNTIME_DIRECTORY if
  set, /var/run for uid 0, and otherwise XDG_RUNTIME_DIR or /tmp. So for a
  non-root run the path is /tmp/dpdk/rte, and any --file-prefix changes the
  last component. A user copying the example verbatim gets a path that does
  not exist, and the "not below world writable /tmp" rationale in the commit
  message does not hold for the non-root case either.

  Suggest keeping <<interface name>> placeholder style for the prefix too,
  e.g. dp_path="<runtime dir>/afxdp_dp/<<interface name>>/afxdp.sock", with
  one sentence saying the runtime directory is what
  rte_eal_get_runtime_dir() returns and depends on uid and --file-prefix.
  That also brings the pinned-map line back under 100 characters.

Warning: doc - "place the pinned map below the EAL runtime directory" is not
actionable as written

  Both new notes say:

     "New deployments should place the socket or pinned map below the EAL
      runtime directory"

  A pinned eBPF map can only be created by bpf_obj_pin() onto a bpffs mount.
  The EAL runtime directory is an ordinary directory created with mkdir(),
  so nothing can be pinned into it directly - it only works if the deployment
  bind-mounts a bpffs entry there, which is what the device plugin does today
  for /tmp/afxdp_dp. Worth saying "mount the endpoint below" rather than
  "place ... below", so readers do not conclude bpf_obj_pin() will work
  against the runtime directory.

Warning: Cc: stable@dpdk.org on a behavior change

  The patch mixes two things. The strlcpy() truncation fix in init_uds_sock()
  and probe(), plus the moved if_name check, are straightforward fixes and
  are good stable material. Preferring the runtime directory over
  /tmp/afxdp_dp is a new default-resolution policy: it introduces a new
  lookup, a new WARNING on every probe of an existing deployment, and a new
  hard -ENAMETOOLONG failure for dp_path values that previously worked
  (truncated). Stable branches are not supposed to take behavior changes.

  Suggest splitting: patch 1 = truncation rejection + if_name reorder, with
  the two Fixes: tags and Cc: stable; patch 2 = runtime directory preference,
  main only, with a release note entry since it changes the documented
  meaning of an existing vdev argument.

Info: get_dflt_dp_path() could apply the sun_path limit so the fallback is
still reachable

  The helper bounds both snprintf() results against PATH_MAX. For the UDS
  case the real limit is sizeof(sun_path) (108), and that limit is only
  applied later in probe(). runtime_dir itself is bounded by UNIX_PATH_MAX,
  so with a long --file-prefix the runtime-directory candidate can exceed
  107 bytes. If that path happens to exist, probe() now fails with
  -ENAMETOOLONG rather than falling back to the legacy path, which would
  have fit. Passing the applicable limit into the helper (or checking
  sun_path on the first candidate before access()) would keep the fallback
  reachable in that case.

Info: the length check in init_uds_sock() is unreachable

  internals->dp_path is only ever populated from the probe()-validated
  dp_path, and probe() already rejects anything >= sizeof(sun_path) when
  use_cni is set. The check inside init_uds_sock() cannot fire. It is
  harmless defense in depth, but if you want only one, init_uds_sock() is
  the better place - it is the actual point of use and covers send_msg(),
  which does the same strlcpy() into dst.sun_path.

Info: WARNING is a strong level for the currently supported configuration

  get_dflt_dp_path() logs at WARNING every time the runtime directory has no
  entry, which is every probe of every existing AF_XDP Device Plugin
  deployment - i.e. the configuration the documentation still describes as
  supported. NOTICE would convey "you may want to migrate" without making
  correctly configured pods look broken in logs.

  reply	other threads:[~2026-08-10 15:28 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-10 10:11 [PATCH] net/af_xdp: fix default device plugin path Anurag Mandal
2026-08-10 15:28 ` Stephen Hemminger [this message]
2026-08-11  8:27 ` [PATCH v2] " Anurag Mandal
2026-08-11 17:59   ` Stephen Hemminger
2026-08-12  1:47 ` [PATCH v3] " Anurag Mandal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260810082807.33fc8a4a@phoenix.local \
    --to=stephen@networkplumber.org \
    --cc=anurag.mandal@intel.com \
    --cc=ciara.loftus@intel.com \
    --cc=dev@dpdk.org \
    --cc=mtahhan@redhat.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox