[pfcwdorch]: Fix null-deref crash when non-port GLOBAL key is deleted by pinky-nexthop · Pull Request #4760 · sonic-net/sonic-swss

pinky-nexthop · 2026-07-17T05:57:48Z

Why I did it

Symptoms

An orchagent core (SIGSEGV, segfault at address 0) was observed after the following sequence of config events:

Deleted BIG_RED_SWITCH from PFC_WD|GLOBAL
Added DEVICE_NEIGHBOR config for all ports
Ran config qos reload

Based on the timestamps, the segfault happened during or immediately after config qos reload.

Root cause

PfcWdOrch::deleteEntry ignores the return value of gPortsOrch->getPort(). When a non-port key like GLOBAL is passed (e.g. when the last field is removed from PFC_WD|GLOBAL, causing Redis to emit a DEL("GLOBAL") event), port is left default-constructed and m_queue_ids is an empty vector. stopWdOnPort → unregisterFromWdDb then dereferences port.m_queue_ids[i] on a null backing pointer, causing a segfault at address 0.

Reproduced by:

redis-cli -n 4 HDEL "PFC_WD|GLOBAL" BIG_RED_SWITCH

when BIG_RED_SWITCH is the only field in that key.

How I did it

Check the return value of gPortsOrch->getPort() in deleteEntry and return early if the port is not present (as is the case for PFC_WD|GLOBAL).

How to verify it

sudo pfcwd big_red_switch enable
redis-cli -n 4 hdel 'PFC_WD|GLOBAL' 'BIG_RED_SWITCH'

orchagent no longer crashes. A unit test covering this case is added in tests/test_pfcwd.py.

mssonicbld · 2026-07-17T05:57:56Z

/azp run

azure-pipelines · 2026-07-17T05:57:56Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

azure-pipelines · 2026-07-17T05:58:06Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Signed-off-by: purush-nexthop <purush@nexthop.ai>

mssonicbld · 2026-07-17T05:59:37Z

/azp run

azure-pipelines · 2026-07-17T05:59:48Z

Azure Pipelines: Successfully started running 1 pipeline(s).

pinky-nexthop · 2026-07-17T06:02:24Z

@vmittal-msft kindly review.

mssonicbld · 2026-07-17T11:22:03Z

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}

pinky-nexthop · 2026-07-17T12:15:43Z

/azpw retry

mssonicbld · 2026-07-17T12:15:45Z

Retrying failed(or canceled) jobs...

mssonicbld · 2026-07-17T12:15:48Z

Retrying failed(or canceled) stages in build 1167507:

✅Stage TestAsan:

Job vstest: retried.

✅Stage Test:

Job vstest: retried.
Job vstest: retried.

mssonicbld · 2026-07-18T11:21:02Z

Hi @sonic-net/sonic-swss-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks!

_{---Powered by SONiC BuildBot}

pinky-nexthop · 2026-07-18T13:16:34Z

/azpw retry

mssonicbld · 2026-07-18T13:16:37Z

Retrying failed(or canceled) jobs...

mssonicbld · 2026-07-18T13:16:41Z

Retrying failed(or canceled) stages in build 1167507:

✅Stage TestAsan:

Job vstest: retried.

✅Stage Test:

Job vstest: retried.
Job vstest: retried.

mssonicbld · 2026-07-19T11:18:19Z

Hi @sonic-net/sonic-swss-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks!

_{---Powered by SONiC BuildBot}

mssonicbld · 2026-07-20T11:17:16Z

Hi @sonic-net/sonic-swss-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks!

_{---Powered by SONiC BuildBot}

pinky-nexthop · 2026-07-21T08:20:47Z

/azpw run

mssonicbld · 2026-07-21T08:20:50Z

⚠️ Notice: /azpw run only runs failed jobs now. If you want to trigger a whole pipline run, please rebase your branch or close and reopen the PR.
💡 Tip: You can also use /azpw retry to retry failed jobs directly.

Retrying failed(or canceled) jobs...

mssonicbld · 2026-07-21T08:20:53Z

Retrying failed(or canceled) stages in build 1167507:

✅Stage TestAsan:

Job vstest: retried.

✅Stage Test:

Job vstest: retried.
Job vstest: retried.

mssonicbld · 2026-07-21T10:07:20Z

/azp run

azure-pipelines · 2026-07-21T10:07:21Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

azure-pipelines · 2026-07-21T10:07:33Z

Azure Pipelines: Successfully started running 1 pipeline(s).

mssonicbld · 2026-07-22T02:56:50Z

/azp run

azure-pipelines · 2026-07-22T02:57:01Z

Azure Pipelines: Successfully started running 1 pipeline(s).

test_pfcwd_global_hdel_last_field_no_crash exercised PFC_WD|GLOBAL delete/hdel paths but its post-conditions were contaminated by the BIG_RED_SWITCH field it used to build a single-field GLOBAL entry: - BIG_RED_SWITCH:enable puts pfcwdorch into big-red-switch mode, which force-drops monitored queues and bypasses normal storm detection. Deleting the field does not transition BRS off, so the queue stayed in force-drop mode and verify_pfcwd_state saw PFC_WD_STATUS=operational instead of stormed. Explicitly disable BIG_RED_SWITCH and reset GLOBAL to a clean POLL_INTERVAL before verifying storm detection. - The BRS enable/disable cycle also bumps the cumulative DEADLOCK_DETECTED/RESTORED counters, so verify_pfcwd_counters saw 2/2 instead of the expected 1/0. Reset the counters before the storm so the single storm yields detected=1, restored=0. Signed-off-by: Pinky Agrawal <pinky@nexthop.ai>

mssonicbld · 2026-07-22T06:48:02Z

/azp run

azure-pipelines · 2026-07-22T06:48:13Z

Azure Pipelines: Successfully started running 1 pipeline(s).

pinky-nexthop · 2026-07-22T11:32:17Z

/azpw retry

mssonicbld · 2026-07-22T11:32:20Z

Retrying failed(or canceled) jobs...

mssonicbld · 2026-07-22T11:32:23Z

Retrying failed(or canceled) stages in build 1171644:

✅Stage BuildAsan:

Job amd64: retried.

vmittal-msft

Reviewed the fix and tests. Root cause is confirmed and the fix is correct, minimal, and safe — recommending approval, with two minor/optional suggestions inline.

Root cause (verified): deleteEntry() ignored getPort()'s return, leaving a default-constructed Port whose m_queue_ids is an empty vector. stopWdOnPort → unregisterFromWdDb then indexes port.m_queue_ids[i] for i in 0..PFC_WD_TC_MAX(8), dereferencing a null vector backing pointer → SIGSEGV at address 0. Worth calling out the asymmetry that made this reachable: createEntry is overridden in PfcWdSwOrch to special-case GLOBAL, but deleteEntry is not — so DEL("GLOBAL") falls through to the port-only path.

Fix: Correct. task_invalid_entry is handled by doTask (entry erased, no retry loop), and it mirrors the guard/message already used in PfcWdSwOrch::doTask's APPL_DB path.

Tests: Both are well-constructed, clearly commented, and vendor-agnostic. test_pfcwd_global_key_del_no_crash covers the direct deleteEntry("GLOBAL") path; test_pfcwd_global_hdel_last_field_no_crash reproduces the real-world last-field hdel → Redis auto-del trigger (valid, since the CONFIG_DB consumer is a keyspace-driven SubscriberStateTable and DVS runs with notify-keyspace-events AKE). Test vstest and TestAsan vstest both pass, so the null-deref would be caught under ASan.

Address review: GLOBAL is a valid config key, not a port, so deleting PFC_WD|GLOBAL should not be logged as an "Invalid port interface" error. Short-circuit the GLOBAL key to task_success before the port lookup, mirroring the existing createEntry override. Genuinely invalid port keys still return task_invalid_entry with the error. Update the test docstring to match. Signed-off-by: Pinky Agrawal <pinky@nexthop.ai>

mssonicbld · 2026-07-23T04:17:50Z

/azp run

azure-pipelines · 2026-07-23T04:18:01Z

Azure Pipelines: Successfully started running 1 pipeline(s).

pinky-nexthop · 2026-07-23T06:40:03Z

/azpw retry

mssonicbld · 2026-07-23T06:40:05Z

Retrying failed(or canceled) jobs...

mssonicbld · 2026-07-23T06:40:08Z

Retrying failed(or canceled) stages in build 1172589:

✅Stage BuildAsan:

Job amd64: retried.

Add test_pfcwd_del_unknown_port_no_crash: deleting a PFC_WD entry for a non-existent port drives the getPort() failure path (task_invalid_entry), which the GLOBAL short-circuit no longer reaches. Restores diff coverage on deleteEntry to 100%. Signed-off-by: Pinky Agrawal <pinky@nexthop.ai>

mssonicbld · 2026-07-23T11:13:42Z

/azp run

azure-pipelines · 2026-07-23T11:13:54Z

Azure Pipelines: Successfully started running 1 pipeline(s).

vmittal-msft

Re-reviewed after the latest changes (aba9491). Both of my earlier comments are addressed — approving.

1. Misleading "Invalid port interface" ERROR on routine GLOBAL removal (my minor/optional suggestion): Resolved exactly as suggested. deleteEntry now short-circuits GLOBAL before the port lookup, mirroring the existing key == PFC_WD_GLOBAL special-case in createEntry:

if (name == PFC_WD_GLOBAL)
{
    return task_process_status::task_success;
}

The SWSS_LOG_ERROR("Invalid port interface ...") now only fires for genuinely invalid ports, and the added comment clearly explains the rationale.

2. GLOBAL-delete no-op behavioral note: Confirmed intended, with the state-teardown behavior tracked as follow-up in #4785.

Bonus: The new test_pfcwd_del_unknown_port_no_crash nicely covers the invalid-port guard — verified it's meaningful, since base createEntry also rejects Ethernet9999 with task_invalid_entry and deleteEntry's guard handles the delete, so orchagent survives both paths.

CI: All checks green, including Test vstest and TestAsan vstest — the ASan run would catch this null-deref, so the fix is verified clean.

The fix remains correct, minimal, consistent with createEntry, and vendor-agnostic. LGTM. 👍

kperumalbfn · 2026-07-23T20:51:51Z

@dgsudharsan Could you please merge this.

pinky-nexthop requested review from kperumalbfn and prsunny as code owners July 17, 2026 05:57

Fix null-deref crash when GLOBAL key is deleted

48d10a6

Signed-off-by: purush-nexthop <purush@nexthop.ai>

pinky-nexthop force-pushed the fix-null-deref-crash-when-global-key-is branch from 6791ee8 to 48d10a6 Compare July 17, 2026 05:58

kperumalbfn previously approved these changes Jul 17, 2026

View reviewed changes

peterbailey-arista mentioned this pull request Jul 18, 2026

[CI] Azure.sonic-swss (Test vstest) Install dependencies: pip3 not found #4763

Closed

pinky-nexthop closed this Jul 21, 2026

pinky-nexthop reopened this Jul 21, 2026

pinky-nexthop dismissed kperumalbfn’s stale review via 0709583 July 22, 2026 02:56

pinky-nexthop force-pushed the fix-null-deref-crash-when-global-key-is branch from 5fd99b6 to 0709583 Compare July 22, 2026 02:56

pinky-nexthop force-pushed the fix-null-deref-crash-when-global-key-is branch from 0709583 to 49efa30 Compare July 22, 2026 06:47

pinky-nexthop requested a review from kperumalbfn July 22, 2026 14:50

vmittal-msft self-requested a review July 22, 2026 16:58

vmittal-msft reviewed Jul 22, 2026

View reviewed changes

Comment thread orchagent/pfcwdorch.cpp

Comment thread orchagent/pfcwdorch.cpp

pinky-nexthop mentioned this pull request Jul 23, 2026

PFC_WD|GLOBAL deletion does not reset applied state (BIG_RED_SWITCH stays enabled) #4785

Open

pinky-nexthop requested a review from vmittal-msft July 23, 2026 17:06

vmittal-msft approved these changes Jul 23, 2026

View reviewed changes

kperumalbfn approved these changes Jul 23, 2026

View reviewed changes

dgsudharsan approved these changes Jul 23, 2026

View reviewed changes

dgsudharsan merged commit a709754 into sonic-net:master Jul 23, 2026
19 checks passed

Uh oh!

Conversation

pinky-nexthop commented Jul 17, 2026

Why I did it

How I did it

How to verify it

Uh oh!

mssonicbld commented Jul 17, 2026

Uh oh!

azure-pipelines Bot commented Jul 17, 2026

Uh oh!

azure-pipelines Bot commented Jul 17, 2026

Uh oh!

mssonicbld commented Jul 17, 2026

Uh oh!

azure-pipelines Bot commented Jul 17, 2026

Uh oh!

pinky-nexthop commented Jul 17, 2026

Uh oh!

mssonicbld commented Jul 17, 2026

Uh oh!

pinky-nexthop commented Jul 17, 2026

Uh oh!

mssonicbld commented Jul 17, 2026

Uh oh!

mssonicbld commented Jul 17, 2026

Uh oh!

mssonicbld commented Jul 18, 2026

Uh oh!

pinky-nexthop commented Jul 18, 2026

Uh oh!

mssonicbld commented Jul 18, 2026

Uh oh!

mssonicbld commented Jul 18, 2026

Uh oh!

mssonicbld commented Jul 19, 2026

Uh oh!

mssonicbld commented Jul 20, 2026

Uh oh!

pinky-nexthop commented Jul 21, 2026

Uh oh!

mssonicbld commented Jul 21, 2026

Uh oh!

mssonicbld commented Jul 21, 2026

Uh oh!

mssonicbld commented Jul 21, 2026

Uh oh!

azure-pipelines Bot commented Jul 21, 2026

Uh oh!

azure-pipelines Bot commented Jul 21, 2026

Uh oh!

mssonicbld commented Jul 22, 2026

Uh oh!

azure-pipelines Bot commented Jul 22, 2026

Uh oh!

mssonicbld commented Jul 22, 2026

Uh oh!

azure-pipelines Bot commented Jul 22, 2026

Uh oh!

pinky-nexthop commented Jul 22, 2026

Uh oh!

mssonicbld commented Jul 22, 2026

Uh oh!

mssonicbld commented Jul 22, 2026

Uh oh!

vmittal-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Jul 23, 2026

Uh oh!

azure-pipelines Bot commented Jul 23, 2026

Uh oh!

pinky-nexthop commented Jul 23, 2026

Uh oh!

mssonicbld commented Jul 23, 2026

Uh oh!

mssonicbld commented Jul 23, 2026

Uh oh!