Linux CIFS filesystem development
 help / color / mirror / Atom feed
From: Sasha Levin <sashal@kernel.org>
To: patches@lists.linux.dev, stable@vger.kernel.org
Cc: ChenXiaoSong <chenxiaosong@kylinos.cn>,
	Namjae Jeon <linkinjeon@kernel.org>,
	Sasha Levin <sashal@kernel.org>,
	linux-cifs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH AUTOSEL 6.18-5.15] smb/server: cancel async requests when closing connection
Date: Mon, 19 Oct 2026 09:48:06 -0400	[thread overview]
Message-ID: <20261019135441.1420968-195-sashal@kernel.org> (raw)
In-Reply-To: <20261019135441.1420968-1-sashal@kernel.org>

From: ChenXiaoSong <chenxiaosong@kylinos.cn>

[ Upstream commit 4fd5bad647bfa45eb86bfd2f03ba1bef3fcd5851 ]

An async request may still be waiting when a connection is closed.
This can stop the connection from closing.

Cancel active async requests before waiting for them to finish.

Suggested-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

The local tree is **6.12.111** (`p-6.12-3-gc504cf0baa3d8`). The hang is
in this tree: `ksmbd_conn_handler_loop()` waits for `r_count == 0`
without cancelling blocked SMB2_LOCK async work.

===============================================================
PHASE 1: COMMIT MESSAGE FORENSICS
===============================================================

Step 1.1: PARSE THE SUBJECT LINE
Record: **[smb/server (ksmbd)] [cancel] [cancel async requests when
closing connection]**

Step 1.2: PARSE ALL COMMIT MESSAGE TAGS
Record:
- Suggested-by: Namjae Jeon `<linkinjeon@kernel.org>` (ksmbd maintainer;
  he asked for this)
- Signed-off-by: ChenXiaoSong `<chenxiaosong@kylinos.cn>` (author)
- Signed-off-by: Namjae Jeon `<linkinjeon@kernel.org>` (maintainer
  applied it)
- No Fixes:, Reported-by:, Tested-by:, Reviewed-by:, Link:, or Cc:
  stable (absence is expected)

Step 1.3: ANALYZE THE COMMIT BODY TEXT
Record:
- Bug: an async request can still be waiting when the connection closes
- Symptom: connection teardown never finishes (`wait_event` on `r_count`
  never wakes)
- Root cause: teardown waits for pending work but does not invoke the
  same `cancel_fn` path that SMB2_CANCEL uses
- Version info: none in the message; mechanism is independent of
  CHANGE_NOTIFY

Step 1.4: DETECT HIDDEN BUG FIXES
Record: Not a disguised cleanup. It is an explicit hang fix. The series
cover letter says “in preparation for change notify,” but this patch
describes a pre-existing teardown hang. In this 6.12 tree CHANGE_NOTIFY
is still `STATUS_NOT_IMPLEMENTED`; SMB2_LOCK already waits
asynchronously.

===============================================================
PHASE 2: DIFF ANALYSIS - LINE BY LINE
===============================================================

Step 2.1: INVENTORY THE CHANGES
Record:
- File: `fs/smb/server/connection.c` only (~21 lines added)
- New helper: `ksmbd_conn_cancel_async_requests()`
- Call site: `ksmbd_conn_handler_loop()` after
  `ksmbd_conn_set_releasing()`, before `wait_event(conn->r_count_q,
  ...)`
- Scope: single-file surgical fix

Step 2.2: UNDERSTAND THE CODE FLOW CHANGE
Record:
- Before: handler loop exits → set RELEASING → sleep until `r_count ==
  0` → disconnect. A worker blocked in `ksmbd_vfs_posix_lock_wait()`
  never drops `r_count`.
- After: same path, but first walk `conn->async_requests` under
  `request_lock`, mark ACTIVE work `KSMBD_WORK_CANCELLED`, and call
  `cancel_fn` so waiters wake and drain.

Step 2.3: IDENTIFY THE BUG MECHANISM
Record:
- Category: hang / stuck kthread (logic/correctness: missing
  cancellation on teardown)
- Mechanism: SMB2_LOCK deferred wait holds an `r_count` ref. Teardown
  did not call `smb2_remove_blocked_lock()` (`locks_delete_block` +
  `locks_wake_up`). Same pattern as existing `smb2_cancel()`. Non-ACTIVE
  work is skipped (avoids the known `file_lock` UAF on
  CANCELLED/CLOSED).

Step 2.4: ASSESS THE FIX QUALITY
Record:
- Quality: obviously correct; copies the `smb2_cancel()` ACTIVE +
  `cancel_fn` pattern
- Regression risk: low. `cancel_fn` under `request_lock` is already how
  `smb2_cancel()` works. `cancel_fn` may be NULL (oplock interim work);
  the new code checks that. Patch 3 of the series closes a remaining
  TOCTOU if `setup_async_work()` runs after this walk; that is a
  separate, rarer race, not a reason to reject this patch.

===============================================================
PHASE 3: GIT HISTORY INVESTIGATION
===============================================================

Step 3.1: BLAME THE CHANGED LINES
Record:
- `wait_event(conn->r_count_q, atomic_read(&conn->r_count) == 0)` comes
  from `a14c573870a664` (“ksmbd: use wait_event instead of
  schedule_timeout()”, Jul 2022)
- `ksmbd_conn_set_releasing()` from `abcc506a9a7197` (2023)
- Async cancel via `cancel_fn` has existed since cifsd
  (`e2f34481b24db2`, 2021)
- Buggy teardown wait has been in this lineage for years, including 6.12

Step 3.2: FOLLOW THE FIXES: TAG
Record: No Fixes: tag. Not applicable.

Step 3.3: CHECK FILE HISTORY FOR RELATED CHANGES
Record:
- Related: `ddb9239828336` / `89ae9df09d2c1` — UAF if `cancel_fn` is
  fired on non-ACTIVE deferred locks (already in this tree; this patch
  uses the same ACTIVE guard)
- `setup_async_work()` / `release_async_work()` already present
- This commit is not in HEAD (`git log --grep='cancel async requests
  when closing'` on `fs/smb/server/` is empty)

Step 3.4: CHECK THE AUTHOR'S OTHER COMMITS
Record: ChenXiaoSong has other ksmbd backports in this tree (PDU size,
etc.). Namjae Jeon is the ksmbd maintainer and both suggested and
applied this.

Step 3.5: CHECK FOR DEPENDENT/PREREQUISITE COMMITS
Record: No code dependency on series patch 1 (signing). Patch 3
(`setup_async_work` refuses new async on EXITING/RELEASING) is
complementary, not required for the helper or call site. All symbols
(`KSMBD_WORK_ACTIVE`, `async_requests`, `request_lock`, `cancel_fn`,
`ksmbd_debug(CONN, ...)`) exist here. Standalone apply is viable.

===============================================================
PHASE 4: MAILING LIST AND EXTERNAL RESEARCH
===============================================================

Step 4.1: FIND THE ORIGINAL PATCH DISCUSSION
Record:
- `b4 dig` cannot match a SHA that is not in this checkout (lore search
  from `b4` also 403’d)
- Thread recovered from lore mirrors: https://ratatoskr.run/linux-
  cifs/2026/07/17307517/t
- Series: `[PATCH v3 0/3] smb/server: fix some bugs in preparation for
  change notify support`
- v2→v3: this patch switched to `list_for_each_entry_safe()`
- Namjae: “Applied 0001 and 0002 patch to #ksmbd-for-next-next.” Patch 3
  was rewritten separately.
- Later landed in the ksmbd 7.3-rc1 pull as “smb/server: cancel async
  requests when closing connection”
- No NAK. No reviewer objection to patch 2.

Step 4.2: CHECK WHO REVIEWED THE PATCH
Record: Suggested-by and applied by Namjae Jeon. Recipients were linux-
cifs / ksmbd maintainers (from the v3 thread).

Step 4.3: SEARCH FOR THE BUG REPORT
Record: No syzbot/bugzilla. Found while implementing CHANGE_NOTIFY;
author states the hang is a pre-existing bug. Trigger is concrete:
disconnect while SMB2_LOCK is deferred.

Step 4.4: CHECK FOR RELATED PATCHES AND SERIES
Record: Patch 1 is unrelated signing. Patch 3/Namjae follow-up (“avoid
registering async requests during connection close”) is a race follow-
up. Patch 2 was applied on its own.

Step 4.5: CHECK STABLE MAILING LIST HISTORY
Record: No useful stable-list hit for this subject. Not previously
rejected for stable (search did not surface a ksmbd discussion of this
patch).

===============================================================
PHASE 5: CODE SEMANTIC ANALYSIS
===============================================================

Step 5.1: IDENTIFY KEY FUNCTIONS IN THE DIFF
Record: added `ksmbd_conn_cancel_async_requests()`; one call from
`ksmbd_conn_handler_loop()`.

Step 5.2: TRACE CALLERS
Record:
- `ksmbd_conn_handler_loop()` is the per-connection kthread (TCP/RDMA
  receive loop)
- After loop exit (client drop, PDU error, `ksmbd_conn_alive()` false,
  module stop)

Step 5.3: TRACE CALLEES
Record: `spin_lock(request_lock)`, list walk,
`work->cancel_fn(work->cancel_argv)`. For lock waiters,
`smb2_remove_blocked_lock()` → `ksmbd_vfs_posix_lock_unblock()` /
`locks_wake_up()`.

Step 5.4: FOLLOW THE CALL CHAIN
Record: Userspace/SMB client can trigger it: SMB2_LOCK that returns
`FILE_LOCK_DEFERRED` → `setup_async_work(..., smb2_remove_blocked_lock,
...)` → `ksmbd_vfs_posix_lock_wait()` → client disconnect → handler
`wait_event(r_count)`. Reachable from a normal SMB session. In 6.12 the
only long-lived async waiter with `cancel_fn` is SMB2_LOCK
(`smb2_notify()` is a stub; oplock `setup_async_work(in_work, NULL,
NULL)` is immediately released).

Step 5.5: SEARCH FOR SIMILAR PATTERNS
Record: `smb2_cancel()` and `set_close_state_blocked_works()` already
cancel this way. Teardown was the missing caller.

===============================================================
PHASE 6: CROSS-REFERENCING AGAINST THE LOCAL TREE
===============================================================

Step 6.1: DOES THE BUGGY CODE EXIST IN THIS TREE?
Record: Yes. `fs/smb/server/connection.c` ~522–524:

```
out:
        ksmbd_conn_set_releasing(conn);
        /* Wait till all reference dropped to the Server object*/
        wait_event(conn->r_count_q, atomic_read(&conn->r_count) == 0);
```

`ksmbd_conn_cancel_async_requests` is absent. `r_count` is inc’d in
`queue_ksmbd_work()` and dec’d only when `handle_ksmbd_work()` finishes.
Introduced with `a14c573870a664` (2022), present throughout 6.12.

Step 6.2: CHECK FOR BACKPORT COMPLICATIONS
Record: First hunk (new helper after `ksmbd_conn_try_dequeue_request`)
matches this tree. Second hunk context includes a later
`ksmbd_debug(CONN, "Wait for all pending requests...")` line that 6.12
does not have. Expected difficulty: **minor context tweak** — insert the
one call after `ksmbd_conn_set_releasing(conn)`.

Step 6.3: CHECK IF RELATED FIXES ARE ALREADY HERE
Record: No equivalent cancel-on-close. UAF guards on `smb2_cancel()` are
already here and align with this patch’s ACTIVE check.

===============================================================
PHASE 7: SUBSYSTEM AND MAINTAINER CONTEXT
===============================================================

Step 7.1: IDENTIFY THE SUBSYSTEM AND ITS CRITICALITY
Record: `fs/smb/server` (ksmbd). **IMPORTANT** for systems that enable
`CONFIG_SMB_SERVER`; not core VFS/mm. Default is `n`.

Step 7.2: ASSESS SUBSYSTEM ACTIVITY
Record: Actively developed; many ksmbd stable backports already on this
6.12 branch. The hang itself is old, not a 7.x-only feature bug.

===============================================================
PHASE 8: IMPACT AND RISK ASSESSMENT
===============================================================

Step 8.1: DETERMINE WHO IS AFFECTED
Record: ksmbd users (`CONFIG_SMB_SERVER`) who have a client block on
SMB2_LOCK and then drop the connection. Also anyone unloading ksmbd
while such a connection exists (`stop_sessions()` keeps looping until
connections are gone).

Step 8.2: DETERMINE THE TRIGGER CONDITIONS
Record: Byte-range lock conflict + disconnect/network drop. Not every
connection; common SMB clients do use locks. Unprivileged SMB clients on
an exported share can trigger it. Not a tiny race: the waiter is already
parked on `flc_wait`.

Step 8.3: DETERMINE THE FAILURE MODE SEVERITY
Record: Connection kthread sleeps forever in `wait_event`; connection
never fully released; module teardown can stall in `stop_sessions()`.
**Severity: HIGH** (hang / stuck kthread; not a panic, not data
corruption).

Step 8.4: CALCULATE RISK-BENEFIT RATIO
Record:
- Benefit: high for ksmbd — prevents teardown/unload hangs
- Risk: low — ~21 lines, one file, established cancel path
- Ratio: favors backport

===============================================================
PHASE 9: FINAL SYNTHESIS
===============================================================

Step 9.1: COMPILE THE EVIDENCE
FOR:
- Real hang in this 6.12 tree when SMB2_LOCK is deferred across
  disconnect
- Small, obviously correct, maintainer-suggested and applied
- Same cancel protocol as `smb2_cancel()`, with the ACTIVE guard already
  required here
- No missing types/helpers in this tree

AGAINST:
- Series was framed around CHANGE_NOTIFY (not in 6.12 as a real waiter)
- Follow-up patch 3 not included (rarer TOCTOU)
- ksmbd is config-optional
- No public crash report / syzbot

UNRESOLVED:
- Could not `b4 dig` a local SHA (commit not in this checkout)
- lore.kernel.org fetches returned 403; used ratatoskr/openwall mirrors
  instead
- No stable-list discussion found

Step 9.2: APPLY THE STABLE RULES CHECKLIST
1. Obviously correct and tested? **PASS** — mirrors `smb2_cancel()`;
   maintainer applied v3
2. Fixes a real bug that affects users? **PASS** — teardown hang on
   deferred lock + disconnect
3. Important issue? **PASS** — hang / stuck connection kthread (HIGH)
4. Small and contained? **PASS** — one file, ~21 lines
5. No new features or APIs? **PASS**
6. Can apply to the local tree? **PASS** — bug present; one-line context
   adjustment for missing debug print

Step 9.3: CHECK FOR EXCEPTION CATEGORIES
Record: None (not an ID/quirk/DT/build/docs exception). It is a normal
hang fix.

Step 9.4: MAKE YOUR DECISION
This is a hang fix that belongs on 6.12. The CHANGE_NOTIFY framing is
not a reason to skip it: the waiter that deadlocks teardown is already
SMB2_LOCK. Patch 3 is a nice follow-up, not a prerequisite for this
change to be correct for already-queued async work.

Verification:
- [Phase 1] Parsed tags: Suggested-by/SOB Namjae Jeon; author
  ChenXiaoSong; no Fixes/Cc/Reported-by
- [Phase 2] Diff: +21 lines in `connection.c`; helper + one call before
  `wait_event`
- [Phase 3] `git describe HEAD` / `make kernelversion`: 6.12.111,
  `p-6.12-3-gc504cf0baa3d8`
- [Phase 3] `git blame` on wait path: `a14c573870a664` (2022) introduced
  `wait_event` without async cancel
- [Phase 3] `git log --grep='cancel async requests when closing' --
  fs/smb/server/`: empty — not in this tree
- [Phase 3] `git merge-base --is-ancestor ddb9239828336 HEAD`: yes —
  ACTIVE-state UAF guard already present
- [Phase 3] Confirmed `ksmbd_conn_try_dequeue_request` /
  `ksmbd_conn_lock` match insertion context
- [Phase 4] b4 dig on a local SHA: lore 403; candidate commit not in
  tree
- [Phase 4] v3 thread: Namjae applied 0001 and 0002; patch 3 rewritten
- [Phase 4] 7.3-rc1 ksmbd pull lists this subject
- [Phase 5] `setup_async_work` callers: `smb2_lock` (real waiter) and
  oplock (NULL cancel_fn, immediate release)
- [Phase 5] `smb2_notify()` in this tree returns
  `STATUS_NOT_IMPLEMENTED`
- [Phase 6] Local lines 521–524: set_releasing then wait_event, no
  cancel
- [Phase 6] Backport: helper applies cleanly; call site needs dropping
  unavailable debug context
- [Phase 8] Failure mode verified: `queue_ksmbd_work` incs `r_count`;
  lock wait never returns so `ksmbd_conn_r_count_dec` never runs
- UNVERIFIED: original lore.kernel.org HTML (403); no stable@ thread
  found

**YES**

 fs/smb/server/connection.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/fs/smb/server/connection.c b/fs/smb/server/connection.c
index 16fcedf45739f..8f1b70e39362e 100644
--- a/fs/smb/server/connection.c
+++ b/fs/smb/server/connection.c
@@ -232,6 +232,26 @@ void ksmbd_conn_try_dequeue_request(struct ksmbd_work *work)
 	wake_up_all(&conn->req_running_q);
 }
 
+static void ksmbd_conn_cancel_async_requests(struct ksmbd_conn *conn)
+{
+	struct ksmbd_work *work, *tmp;
+
+	ksmbd_debug(CONN, "Cancel pending async requests on releasing connection\n");
+	spin_lock(&conn->request_lock);
+	list_for_each_entry_safe(work, tmp, &conn->async_requests,
+				 async_request_entry) {
+		if (work->state != KSMBD_WORK_ACTIVE)
+			continue;
+
+		ksmbd_debug(CONN, "Cancel async request id %d\n",
+			    work->async_id);
+		work->state = KSMBD_WORK_CANCELLED;
+		if (work->cancel_fn)
+			work->cancel_fn(work->cancel_argv);
+	}
+	spin_unlock(&conn->request_lock);
+}
+
 void ksmbd_conn_lock(struct ksmbd_conn *conn)
 {
 	mutex_lock(&conn->srv_mutex);
@@ -543,6 +563,7 @@ int ksmbd_conn_handler_loop(void *p)
 
 out:
 	ksmbd_conn_set_releasing(conn);
+	ksmbd_conn_cancel_async_requests(conn);
 	/* Wait till all reference dropped to the Server object*/
 	ksmbd_debug(CONN, "Wait for all pending requests(%d)\n", atomic_read(&conn->r_count));
 	wait_event(conn->r_count_q, atomic_read(&conn->r_count) == 0);
-- 
2.53.0


  parent reply	other threads:[~2026-10-19 13:59 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20261019135441.1420968-1-sashal@kernel.org>
2026-10-19 13:45 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: skip fallocate for SMB2_CREATE_ALLOCATION_SIZE on a stream handle Sasha Levin
2026-10-19 13:45 ` [PATCH AUTOSEL 6.18-6.12] ksmbd: fix off-by-one rejecting minimal COPYCHUNK query-limits request Sasha Levin
2026-10-19 13:46 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: handle empty QUERY_ALLOCATED_RANGES output Sasha Levin
2026-10-19 13:46 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: notify parent directory leases on child create Sasha Levin
2026-10-19 13:46 ` [PATCH AUTOSEL 6.18-6.1] ksmbd: handle allocated range queries on dense files Sasha Levin
2026-10-19 13:47 ` [PATCH AUTOSEL 6.18-6.6] ksmbd: report actual xattr value length in stream enumeration Sasha Levin
2026-10-19 13:48 ` Sasha Levin [this message]
2026-10-19 13:48 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: allow FSCTL_SET_SPARSE without input buffer Sasha Levin
2026-10-19 13:48 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: protect private extended attributes Sasha Levin
2026-10-19 13:48 ` [PATCH AUTOSEL 6.18-6.1] ksmbd: allow I/O on directory named streams Sasha Levin
2026-10-19 13:49 ` [PATCH AUTOSEL 6.18-5.15] smb: server: Free session data in user_session.c with kfree_sensitive() Sasha Levin
2026-10-19 13:50 ` [PATCH AUTOSEL 6.18-6.6] ksmbd: return buffer too small for short security queries Sasha Levin
2026-10-19 13:50 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: honor client signing-required in all modes Sasha Levin
2026-10-19 13:50 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: preserve access denied status for copychunk Sasha Levin
2026-10-19 13:51 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: fix heap out-of-bounds write in krb5_authenticate() Sasha Levin
2026-10-19 13:51 ` [PATCH AUTOSEL 6.18-6.6] smb: server: Make sure that passkey is not leaked on the heap in user_config.c Sasha Levin
2026-10-19 13:51 ` [PATCH AUTOSEL 6.18-6.1] ksmbd: honor byte-range locks for zero data Sasha Levin
2026-10-19 13:52 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: reject blocking compound lock requests Sasha Levin
2026-10-19 13:52 ` [PATCH AUTOSEL 6.18-6.12] ksmbd: reject SMB3.1.1 binding with mismatched cipher Sasha Levin
2026-10-19 13:52 ` [PATCH AUTOSEL 6.18-6.1] ksmbd: fix AsyncId zeroed before use in smb2_lock() cancel response Sasha Levin
2026-10-19 13:52 ` [PATCH AUTOSEL 6.18-6.6] ksmbd: fix durable handle v2 default timeout units (60 -> 60000) Sasha Levin
2026-10-19 13:53 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: enable TCP keepalive for accepted connections Sasha Levin
2026-10-19 13:53 ` [PATCH AUTOSEL 6.18-5.15] ksmbd: fix SMB2 byte-range lock end offset Sasha Levin

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=20261019135441.1420968-195-sashal@kernel.org \
    --to=sashal@kernel.org \
    --cc=chenxiaosong@kylinos.cn \
    --cc=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=patches@lists.linux.dev \
    --cc=stable@vger.kernel.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