|
| 1 | +From a42896bebfcc287ed1e61d820a888e33b1eb80ce Mon Sep 17 00:00:00 2001 |
| 2 | +From: DaeMyung Kang <charsyam@gmail.com> |
| 3 | +Date: Tue, 28 Apr 2026 23:08:55 +0900 |
| 4 | +Subject: [PATCH] ksmbd: harden file lifetime during session teardown |
| 5 | + |
| 6 | +__close_file_table_ids() is the per-session teardown that closes every |
| 7 | +fp belonging to a session (or to one tree connect on that session) by |
| 8 | +walking the session's volatile-id idr. The current loop has three |
| 9 | +related problems on busy or racing workloads: |
| 10 | + |
| 11 | + * Sleeping under ft->lock. The session-teardown skip callback, |
| 12 | + session_fd_check(), already sleeps in ksmbd_vfs_copy_durable_owner() |
| 13 | + -> kstrdup(GFP_KERNEL) and down_write(&fp->f_ci->m_lock) (a |
| 14 | + rw_semaphore). Running the callback inside write_lock(&ft->lock) |
| 15 | + trips CONFIG_DEBUG_ATOMIC_SLEEP / CONFIG_PROVE_LOCKING on a |
| 16 | + durable-fd workload. |
| 17 | + |
| 18 | + * Refcount accounting blind to f_state. The unconditional |
| 19 | + atomic_dec_and_test(&fp->refcount) does not distinguish |
| 20 | + FP_INITED (idr-owned reference still intact) from FP_CLOSED (an |
| 21 | + earlier ksmbd_close_fd() already consumed the idr-owned reference |
| 22 | + while leaving fp in the idr because a holder kept refcount |
| 23 | + non-zero). When the latter races with teardown the same path |
| 24 | + over-decrements into a holder reference and ksmbd_fd_put() later |
| 25 | + UAFs that holder. |
| 26 | + |
| 27 | + * FP_NEW window. Between __open_id() publishing fp into the |
| 28 | + session idr and ksmbd_update_fstate(..., FP_INITED) committing the |
| 29 | + transition at the end of smb2_open(), an fp is in FP_NEW and an |
| 30 | + intervening teardown that takes a transient reference and |
| 31 | + unpublishes the volatile id leaves the original idr-owned |
| 32 | + reference orphaned -- the opener is unaware that fp has been |
| 33 | + unpublished, returns success to the client, and the fp leaks at |
| 34 | + refcount = 1. |
| 35 | + |
| 36 | +Refactor __close_file_table_ids() to take a transient reference on fp |
| 37 | +and unpublish fp from the session idr *under ft->lock* before calling |
| 38 | +skip() outside the lock. A transient ref protects lifetime but not |
| 39 | +concurrent field mutation, so the idr_remove() is what keeps |
| 40 | +__ksmbd_lookup_fd() through this session's idr from granting a new |
| 41 | +ksmbd_fp_get() reference to an fp whose fp->conn / fp->tcon / |
| 42 | +fp->volatile_id / op->conn / lock_list links are about to be rewritten |
| 43 | +by session_fd_check(). Durable reconnect is unaffected because it |
| 44 | +reaches fp through the global durable table (ksmbd_lookup_durable_fd |
| 45 | +-> global_ft). |
| 46 | + |
| 47 | +Decide n_to_drop together with any FP_INITED -> FP_CLOSED transition |
| 48 | +under ft->lock so teardown and ksmbd_close_fd() never both consume the |
| 49 | +idr-owned reference. See ksmbd_mark_fp_closed() for the per-state |
| 50 | +accounting. For the FP_NEW path to be safe, the opener has to learn |
| 51 | +that fp was unpublished: ksmbd_update_fstate() now returns -ENOENT |
| 52 | +when an FP_NEW -> FP_INITED transition finds f_state already advanced |
| 53 | +or the volatile id cleared (both committed by teardown under |
| 54 | +ft->lock); smb2_open() propagates that as STATUS_OBJECT_NAME_INVALID |
| 55 | +and drops the original reference via ksmbd_fd_put(). |
| 56 | + |
| 57 | +The list removal cannot be left for a deferred final putter because |
| 58 | +fp->volatile_id has already been cleared and __ksmbd_remove_fd() will |
| 59 | +intentionally skip both idr_remove() and list_del_init(). Move the |
| 60 | +m_fp_list unlink in __ksmbd_remove_fd() above the volatile-id check so |
| 61 | +that an FP_NEW fp that happened to be added to m_fp_list (smb2_open() |
| 62 | +adds fp->node before ksmbd_update_fstate() runs) is still cleaned up |
| 63 | +on the deferred putter path; list_del_init() on an empty node is a |
| 64 | +no-op and remains safe for fps that were never added. |
| 65 | + |
| 66 | +Add a defensive guard in session_fd_check() that refuses non-FP_INITED |
| 67 | +fps so that even if a teardown reaches an FP_NEW fp it falls into the |
| 68 | +close branch (where the n_to_drop = 1 accounting keeps the opener's |
| 69 | +reference alive) instead of the durable-preserve branch (which mutates |
| 70 | +fp->conn / fp->tcon). |
| 71 | + |
| 72 | +Validation on a debug kernel additionally built with CONFIG_DEBUG_LIST |
| 73 | +and CONFIG_DEBUG_OBJECTS_WORK used a same-session two-tcon workload |
| 74 | +(open/write storm on one tcon, 50 tree disconnects on the other) and |
| 75 | +reported no list-corruption, work_struct ODEBUG, sleep-in-atomic, |
| 76 | +lockdep or kmemleak reports. Reverting only the |
| 77 | +__close_file_table_ids() hunk while keeping a forced-is_reconnectable() |
| 78 | +harness produced the expected sleep-in-atomic at vfs_cache.c:1095, |
| 79 | +confirming the ft->lock-out-of-sleepable-skip discipline. |
| 80 | + |
| 81 | +KASAN-enabled direct SMB2 coverage with durable handles enabled |
| 82 | +exercised ksmbd_close_tree_conn_fds(), ksmbd_close_session_fds(), |
| 83 | +the FP_NEW failure path, tree_conn_fd_check(), and a non-zero |
| 84 | +session_fd_check() durable-preserve return. This produced no KASAN, |
| 85 | +DEBUG_LIST, ODEBUG, or WARNING reports. |
| 86 | + |
| 87 | +Fixes: f44158485826 ("cifsd: add file operations") |
| 88 | +Signed-off-by: DaeMyung Kang <charsyam@gmail.com> |
| 89 | +Acked-by: Namjae Jeon <linkinjeon@kernel.org> |
| 90 | +Signed-off-by: Steve French <stfrench@microsoft.com> |
| 91 | +--- |
| 92 | + fs/smb/server/smb2pdu.c | 6 +- |
| 93 | + fs/smb/server/vfs_cache.c | 179 +++++++++++++++++++++++++++++++++----- |
| 94 | + fs/smb/server/vfs_cache.h | 4 +- |
| 95 | + 3 files changed, 164 insertions(+), 25 deletions(-) |
| 96 | + |
| 97 | +--- a/fs/smb/server/smb2pdu.c |
| 98 | ++++ b/fs/smb/server/smb2pdu.c |
| 99 | +@@ -3777,8 +3777,10 @@ err_out1: |
| 100 | + |
| 101 | + err_out2: |
| 102 | + if (!rc) { |
| 103 | +- ksmbd_update_fstate(&work->sess->file_table, fp, FP_INITED); |
| 104 | +- rc = ksmbd_iov_pin_rsp(work, (void *)rsp, iov_len); |
| 105 | ++ rc = ksmbd_update_fstate(&work->sess->file_table, fp, |
| 106 | ++ FP_INITED); |
| 107 | ++ if (!rc) |
| 108 | ++ rc = ksmbd_iov_pin_rsp(work, (void *)rsp, iov_len); |
| 109 | + } |
| 110 | + if (rc) { |
| 111 | + if (rc == -EINVAL) |
| 112 | +--- a/fs/smb/server/vfs_cache.c |
| 113 | ++++ b/fs/smb/server/vfs_cache.c |
| 114 | +@@ -358,13 +358,13 @@ static void ksmbd_remove_durable_fd(stru |
| 115 | + |
| 116 | + static void __ksmbd_remove_fd(struct ksmbd_file_table *ft, struct ksmbd_file *fp) |
| 117 | + { |
| 118 | +- if (!has_file_id(fp->volatile_id)) |
| 119 | +- return; |
| 120 | +- |
| 121 | + down_write(&fp->f_ci->m_lock); |
| 122 | + list_del_init(&fp->node); |
| 123 | + up_write(&fp->f_ci->m_lock); |
| 124 | + |
| 125 | ++ if (!has_file_id(fp->volatile_id)) |
| 126 | ++ return; |
| 127 | ++ |
| 128 | + write_lock(&ft->lock); |
| 129 | + idr_remove(ft->idr, fp->volatile_id); |
| 130 | + write_unlock(&ft->lock); |
| 131 | +@@ -748,15 +748,58 @@ err_out: |
| 132 | + return ERR_PTR(ret); |
| 133 | + } |
| 134 | + |
| 135 | +-void ksmbd_update_fstate(struct ksmbd_file_table *ft, struct ksmbd_file *fp, |
| 136 | +- unsigned int state) |
| 137 | ++/** |
| 138 | ++ * ksmbd_update_fstate() - update an fp state under the file-table lock |
| 139 | ++ * @ft: file table that publishes @fp's volatile id |
| 140 | ++ * @fp: file pointer to update |
| 141 | ++ * @state: new state |
| 142 | ++ * |
| 143 | ++ * Return: 0 on success. The FP_NEW -> FP_INITED transition is special: |
| 144 | ++ * -ENOENT if teardown already unpublished @fp by advancing the state or |
| 145 | ++ * clearing the volatile id. Other state updates preserve the historical |
| 146 | ++ * fire-and-forget behavior. |
| 147 | ++ */ |
| 148 | ++int ksmbd_update_fstate(struct ksmbd_file_table *ft, struct ksmbd_file *fp, |
| 149 | ++ unsigned int state) |
| 150 | + { |
| 151 | ++ int ret; |
| 152 | ++ |
| 153 | + if (!fp) |
| 154 | +- return; |
| 155 | ++ return -ENOENT; |
| 156 | + |
| 157 | + write_lock(&ft->lock); |
| 158 | +- fp->f_state = state; |
| 159 | ++ if (state == FP_INITED && |
| 160 | ++ (fp->f_state != FP_NEW || !has_file_id(fp->volatile_id))) { |
| 161 | ++ ret = -ENOENT; |
| 162 | ++ } else { |
| 163 | ++ fp->f_state = state; |
| 164 | ++ ret = 0; |
| 165 | ++ } |
| 166 | + write_unlock(&ft->lock); |
| 167 | ++ |
| 168 | ++ return ret; |
| 169 | ++} |
| 170 | ++ |
| 171 | ++/* |
| 172 | ++ * ksmbd_mark_fp_closed() - mark fp closed under ft->lock and return how many |
| 173 | ++ * refs the teardown path owns. |
| 174 | ++ * |
| 175 | ++ * FP_INITED has a normal idr-owned reference, so teardown owns both that |
| 176 | ++ * reference and the transient lookup reference. FP_NEW is still owned by the |
| 177 | ++ * in-flight opener/reopener, which will drop the original reference after |
| 178 | ++ * ksmbd_update_fstate(..., FP_INITED) observes the cleared volatile id. |
| 179 | ++ * FP_CLOSED on entry means an earlier ksmbd_close_fd() already consumed the |
| 180 | ++ * idr-owned ref. |
| 181 | ++ */ |
| 182 | ++static int ksmbd_mark_fp_closed(struct ksmbd_file *fp) |
| 183 | ++{ |
| 184 | ++ if (fp->f_state == FP_INITED) { |
| 185 | ++ set_close_state_blocked_works(fp); |
| 186 | ++ fp->f_state = FP_CLOSED; |
| 187 | ++ return 2; |
| 188 | ++ } |
| 189 | ++ |
| 190 | ++ return 1; |
| 191 | + } |
| 192 | + |
| 193 | + static int |
| 194 | +@@ -764,7 +807,8 @@ __close_file_table_ids(struct ksmbd_sess |
| 195 | + struct ksmbd_tree_connect *tcon, |
| 196 | + bool (*skip)(struct ksmbd_tree_connect *tcon, |
| 197 | + struct ksmbd_file *fp, |
| 198 | +- struct ksmbd_user *user)) |
| 199 | ++ struct ksmbd_user *user), |
| 200 | ++ bool skip_preserves_fp) |
| 201 | + { |
| 202 | + struct ksmbd_file_table *ft = &sess->file_table; |
| 203 | + struct ksmbd_file *fp; |
| 204 | +@@ -772,32 +816,120 @@ __close_file_table_ids(struct ksmbd_sess |
| 205 | + int num = 0; |
| 206 | + |
| 207 | + while (1) { |
| 208 | ++ int n_to_drop; |
| 209 | ++ |
| 210 | + write_lock(&ft->lock); |
| 211 | + fp = idr_get_next(ft->idr, &id); |
| 212 | + if (!fp) { |
| 213 | + write_unlock(&ft->lock); |
| 214 | + break; |
| 215 | + } |
| 216 | +- |
| 217 | +- if (skip(tcon, fp, sess->user) || |
| 218 | +- !atomic_dec_and_test(&fp->refcount)) { |
| 219 | ++ if (!atomic_inc_not_zero(&fp->refcount)) { |
| 220 | + id++; |
| 221 | + write_unlock(&ft->lock); |
| 222 | + continue; |
| 223 | + } |
| 224 | + |
| 225 | +- set_close_state_blocked_works(fp); |
| 226 | +- idr_remove(ft->idr, fp->volatile_id); |
| 227 | +- fp->volatile_id = KSMBD_NO_FID; |
| 228 | +- write_unlock(&ft->lock); |
| 229 | ++ if (skip_preserves_fp) { |
| 230 | ++ /* |
| 231 | ++ * Session teardown: skip() is session_fd_check(), |
| 232 | ++ * which may sleep and mutates fp->conn / fp->tcon / |
| 233 | ++ * fp->volatile_id when it chooses to preserve fp |
| 234 | ++ * for durable reconnect. Unpublish fp from the |
| 235 | ++ * session idr here, under ft->lock, so that |
| 236 | ++ * __ksmbd_lookup_fd() through this session cannot |
| 237 | ++ * grant a new ksmbd_fp_get() reference to an fp |
| 238 | ++ * whose fields are about to be rewritten outside |
| 239 | ++ * the lock. Durable reconnect still reaches fp via |
| 240 | ++ * global_ft. |
| 241 | ++ */ |
| 242 | ++ idr_remove(ft->idr, id); |
| 243 | ++ fp->volatile_id = KSMBD_NO_FID; |
| 244 | ++ write_unlock(&ft->lock); |
| 245 | + |
| 246 | ++ if (skip(tcon, fp, sess->user)) { |
| 247 | ++ /* |
| 248 | ++ * session_fd_check() has converted fp to |
| 249 | ++ * durable-preserve state and cleared its |
| 250 | ++ * per-conn fields. fp is already unpublished |
| 251 | ++ * above; the original idr-owned ref keeps it |
| 252 | ++ * alive for the durable scavenger. Drop only |
| 253 | ++ * the transient ref. atomic_dec() is safe -- |
| 254 | ++ * atomic_inc_not_zero() succeeded on a |
| 255 | ++ * positive value and we added one more, so |
| 256 | ++ * refcount cannot be zero here. |
| 257 | ++ */ |
| 258 | ++ atomic_dec(&fp->refcount); |
| 259 | ++ id++; |
| 260 | ++ continue; |
| 261 | ++ } |
| 262 | ++ |
| 263 | ++ /* |
| 264 | ++ * Keep the close-state decision under the same lock |
| 265 | ++ * observed by ksmbd_update_fstate(), which is how an |
| 266 | ++ * in-flight FP_NEW opener learns that teardown has |
| 267 | ++ * cleared its volatile id. |
| 268 | ++ */ |
| 269 | ++ write_lock(&ft->lock); |
| 270 | ++ n_to_drop = ksmbd_mark_fp_closed(fp); |
| 271 | ++ write_unlock(&ft->lock); |
| 272 | ++ } else { |
| 273 | ++ /* |
| 274 | ++ * Tree teardown: skip() is tree_conn_fd_check(), a |
| 275 | ++ * cheap pointer compare that doesn't sleep and has |
| 276 | ++ * no side effects, so keep the skip decision plus |
| 277 | ++ * the unpublish-and-mark-closed sequence atomic |
| 278 | ++ * under ft->lock. fps belonging to other tree |
| 279 | ++ * connects (skip() == true) stay fully published in |
| 280 | ++ * the session idr with no lock window. |
| 281 | ++ */ |
| 282 | ++ if (skip(tcon, fp, sess->user)) { |
| 283 | ++ atomic_dec(&fp->refcount); |
| 284 | ++ write_unlock(&ft->lock); |
| 285 | ++ id++; |
| 286 | ++ continue; |
| 287 | ++ } |
| 288 | ++ idr_remove(ft->idr, id); |
| 289 | ++ fp->volatile_id = KSMBD_NO_FID; |
| 290 | ++ n_to_drop = ksmbd_mark_fp_closed(fp); |
| 291 | ++ write_unlock(&ft->lock); |
| 292 | ++ } |
| 293 | ++ |
| 294 | ++ /* |
| 295 | ++ * fp->volatile_id is already cleared to prevent stale idr |
| 296 | ++ * removal from a deferred final close. Remove fp from |
| 297 | ++ * m_fp_list here because __ksmbd_remove_fd() will skip the |
| 298 | ++ * list unlink when volatile_id is KSMBD_NO_FID. |
| 299 | ++ */ |
| 300 | + down_write(&fp->f_ci->m_lock); |
| 301 | + list_del_init(&fp->node); |
| 302 | + up_write(&fp->f_ci->m_lock); |
| 303 | + |
| 304 | +- __ksmbd_close_fd(ft, fp); |
| 305 | +- |
| 306 | +- num++; |
| 307 | ++ /* |
| 308 | ++ * Drop the references this iteration owns: |
| 309 | ++ * |
| 310 | ++ * n_to_drop == 2: we observed FP_INITED and committed |
| 311 | ++ * the FP_CLOSED transition ourselves, so we own the |
| 312 | ++ * transient (+1) and the still-intact idr-owned ref. |
| 313 | ++ * |
| 314 | ++ * n_to_drop == 1: either a prior ksmbd_close_fd() |
| 315 | ++ * already consumed the idr-owned ref, or fp was still |
| 316 | ++ * FP_NEW and the in-flight opener/reopener must keep |
| 317 | ++ * the original reference until ksmbd_update_fstate() |
| 318 | ++ * observes the cleared volatile id. |
| 319 | ++ * |
| 320 | ++ * If we end up as the final putter, finalize fp and |
| 321 | ++ * account the open_files_count decrement via the caller's |
| 322 | ++ * atomic_sub(num, ...). Otherwise the remaining user's |
| 323 | ++ * ksmbd_fd_put() reaches __put_fd_final(), which does its |
| 324 | ++ * own atomic_dec(&open_files_count), so we must not count |
| 325 | ++ * this fp here -- doing so would double-decrement the |
| 326 | ++ * connection-wide counter. |
| 327 | ++ */ |
| 328 | ++ if (atomic_sub_and_test(n_to_drop, &fp->refcount)) { |
| 329 | ++ __ksmbd_close_fd(NULL, fp); |
| 330 | ++ num++; |
| 331 | ++ } |
| 332 | + id++; |
| 333 | + } |
| 334 | + |
| 335 | +@@ -1071,6 +1203,9 @@ static bool session_fd_check(struct ksmb |
| 336 | + if (!is_reconnectable(fp)) |
| 337 | + return false; |
| 338 | + |
| 339 | ++ if (fp->f_state != FP_INITED) |
| 340 | ++ return false; |
| 341 | ++ |
| 342 | + if (WARN_ON_ONCE(!fp->conn)) |
| 343 | + return false; |
| 344 | + |
| 345 | +@@ -1122,7 +1257,8 @@ void ksmbd_close_tree_conn_fds(struct ks |
| 346 | + { |
| 347 | + int num = __close_file_table_ids(work->sess, |
| 348 | + work->tcon, |
| 349 | +- tree_conn_fd_check); |
| 350 | ++ tree_conn_fd_check, |
| 351 | ++ false); |
| 352 | + |
| 353 | + atomic_sub(num, &work->conn->stats.open_files_count); |
| 354 | + } |
| 355 | +@@ -1131,7 +1267,8 @@ void ksmbd_close_session_fds(struct ksmb |
| 356 | + { |
| 357 | + int num = __close_file_table_ids(work->sess, |
| 358 | + work->tcon, |
| 359 | +- session_fd_check); |
| 360 | ++ session_fd_check, |
| 361 | ++ true); |
| 362 | + |
| 363 | + atomic_sub(num, &work->conn->stats.open_files_count); |
| 364 | + } |
| 365 | +@@ -1271,7 +1408,7 @@ void ksmbd_destroy_file_table(struct ksm |
| 366 | + if (!ft->idr) |
| 367 | + return; |
| 368 | + |
| 369 | +- __close_file_table_ids(sess, NULL, session_fd_check); |
| 370 | ++ __close_file_table_ids(sess, NULL, session_fd_check, true); |
| 371 | + idr_destroy(ft->idr); |
| 372 | + kfree(ft->idr); |
| 373 | + ft->idr = NULL; |
| 374 | +--- a/fs/smb/server/vfs_cache.h |
| 375 | ++++ b/fs/smb/server/vfs_cache.h |
| 376 | +@@ -172,8 +172,8 @@ int ksmbd_close_inode_fds(struct ksmbd_w |
| 377 | + int ksmbd_init_global_file_table(void); |
| 378 | + void ksmbd_free_global_file_table(void); |
| 379 | + void ksmbd_set_fd_limit(unsigned long limit); |
| 380 | +-void ksmbd_update_fstate(struct ksmbd_file_table *ft, struct ksmbd_file *fp, |
| 381 | +- unsigned int state); |
| 382 | ++int ksmbd_update_fstate(struct ksmbd_file_table *ft, struct ksmbd_file *fp, |
| 383 | ++ unsigned int state); |
| 384 | + bool ksmbd_vfs_compare_durable_owner(struct ksmbd_file *fp, |
| 385 | + struct ksmbd_user *user); |
| 386 | + |
0 commit comments