Ship synced custom SAI headers in libsaivs-dev#1984
Conversation
|
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
yxieca-admin
left a comment
There was a problem hiding this comment.
@brcarr-nv can you fix the indentation change?
As for the unit test coverage picked up Debian folder, I think this is a mistake. @lolyu @mramezani95 can you check and fix this?
|
Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks! ---Powered by SONiC BuildBot
|
0cf436c to
c0eecf1
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
c0eecf1 to
bfe1dbb
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The tab spacing is necessary because my additions are shell commands, not make directives like the "ifneq" and similar statements above. This is all supposed to be nested under the override_dh_install recipe. |
When vendor custom SAI headers are present in SAI/custom/, parse.pl emits "#include <saicustom.h>" into the generated saimetadata.h. Install those custom headers into libsaivs-dev so the include resolves in downstream builds (e.g. sonic-swss). libsaivs-dev conflicts with mlnx-sai, so there is no /usr/include/sai/saicustom.h overwrite clash. Guarded on header existence, so it is a no-op for builds without synced custom headers. Signed-off-by: Brian Carr <brcarr@nvidia.com>
bfe1dbb to
c3031f7
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
1 similar comment
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
lolyu
left a comment
There was a problem hiding this comment.
Left an inline comment on the header-install glob (*custom*.h coverage). Otherwise the change and root-cause analysis look sound. Also worth filling in the Tested branch / test-evidence section — the 202605 backport bot is flagging it as missing.
lolyu
left a comment
There was a problem hiding this comment.
One comment on the header-install glob (*custom*.h coverage vs. the aggregated saicustom.h include chain). Root-cause analysis and overall approach look sound.
| if ls SAI/custom/*custom*.h >/dev/null 2>&1 && [ -d debian/libsaivs-dev/usr/include/sai ]; then \ | ||
| install -m 644 SAI/custom/*custom*.h debian/libsaivs-dev/usr/include/sai/; \ | ||
| fi | ||
|
|
There was a problem hiding this comment.
The *custom*.h glob is narrower than the include chain the generated metadata actually consumes, and the failure mode is silent + deferred to downstream build time.
parse.pl emits #include <saicustom.h> into the public saimetadata.h (guarded if scalar@ch), but that saicustom.h is an aggregator: parse.pl discovers custom content by scanning $CUSTOM_DIR = "../custom/" and emits per-API includes named saicustom$shortapi.h (parse.pl ~L4688) — saicustomacl.h, saicustomswitch.h, etc. So the real downstream include chain is:
saimetadata.h -> saicustom.h -> saicustomacl.h, saicustomswitch.h, ...
Every link in that chain has to be present in libsaivs-dev for the downstream saimetadata.h compile (e.g. sonic-swss) to resolve.
Two problems with *custom*.h:
-
Coverage relies on the "custom" substring being in every filename. Mellanox's split happens to name everything
saicustom*.htoday, so the glob catches them — but nothing in the SAI meta build requires that.saicustom.h(or a per-API custom header) is free to#includea vendor header inSAI/custom/whose name doesn't contain "custom" (a shared typedef/defs header, etc.). Any such header is silently dropped and the chain breaks at swss build time with a confusing "no such file" far from here. -
The guard doesn't verify coverage.
ls SAI/custom/*custom*.h >/dev/null 2>&1only checks that at least one matching file exists, not that the installed set matches what the aggregator includes. Install a partial set and the guard still passes.
Since libsaivs-dev conflicts with vendor dev packages (as the PR description establishes), there's no file-overwrite risk — which removes the only reason to be conservative about what's installed. Suggest just shipping the whole custom dir the metadata build consumes:
install -m 644 SAI/custom/*.h debian/libsaivs-dev/usr/include/sai/;That mirrors parse.pl's $CUSTOM_DIR scan exactly and guarantees the full aggregated include chain resolves.
Question: does the vendor SAI/custom/ ever contain headers — directly, or transitively #included by saicustom*.h — that don't match *custom*? If so this glob drops them. Given there's no overwrite risk, is there any reason not to install SAI/custom/*.h?
When vendor custom SAI headers are present in SAI/custom/, parse.pl emits "#include <saicustom.h>" into the generated saimetadata.h. Install those custom headers into libsaivs-dev so the include resolves in downstream builds (e.g. sonic-swss). libsaivs-dev conflicts with mlnx-sai, so there is no /usr/include/sai/saicustom.h overwrite clash. Guarded on header existence, so it is a no-op for builds without synced custom headers.
Description of PR
Type of change
Approach
What is the motivation for this PR?
ASAN-enabled
syncdaborts with a heap-buffer-overflow when creating an ACL table on Mellanox:Root cause: SONiC generates the SAI metadata (
libsaimetadata) from the OCP headers only and drops the vendorSAI/customfolder, sosai_acl_bind_point_type_tis emitted withoutSAI_ACL_BIND_POINT_TYPE_CPU_PORT(valuescount == 5). The vendor SAI (36.0.14+) defines a 6th value,CPU_PORT, and the vendorlibsaiis linked against this SONiC-generated metadata insidesyncd, so the two disagree on the enum size.libsaisizes a scratch array from the undersized metadata (calloc(enum_metadata->valuescount, ...)→ room for 5) but walks the full vendor enum space includingCPU_PORT, touching the 6th entry past the end of the allocation → heap-buffer-overflow. The fix is to build SONiC's metadata with the vendor custom headers so it matches the vendor SAI.This change is a small piece of the overall fix. It updates sonic-sairedis to handle custom SAI headers appropriately. A subsequent PR to sonic-buildimage will implement the rest of the fix for this issue.
Work item tracking
How did you do it?
Ship the headers for downstream consumers.
sonic-sairedisdebian/rulesinstalls the synced custom headers intolibsaivs-devso downstream builds of the publicsaimetadata.h(e.g.sonic-swss). The install is guarded on header presence so builds not including custom headers are unaffected.How did you verify/test it?
This issue can be verified with the following test command (sonic-mgmt):
Additionally, simply running
sudo config acl add table ASAN_REPRO L3 -p Ethernet128 -s ingressis enough to repro the issue. You can then check for ASAN results withsudo cat /var/log/asan/syncd-asan.log.*.Any platform specific information?
Documentation
Summary by CodeRabbit