Skip to contentSkip to conversation

ksmbd: defer oplock break until m_lock is released

PATCH In review Abdifatah Suruur 3 participants 7 messages Sep 11 – Sep 16 1 message not in the archive Stable branch

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 2026090904-pecan-editor-19b7@gregkh

Abdifatah Suruur

author
#1 lore raw

PATCH 1/1 ksmbd: defer oplock break until m_lock is released

The 6.18.y backport of commit 0e753899627b5e ("ksmbd: fix use-after-free in oplock break notification", applied here as c8279ae8df68) takes ci->m_lock for read inside smb2_oplock_break_conn_get() to serialize against the opinfo->conn writers. Unlike mainline, the 6.18 tree has no deferred break
machinery: smb_send_parent_lease_break_noti(), smb_lazy_parent_lease_break_close() and smb_break_all_levII_oplock() hold ci->m_lock for read around the whole walk and call oplock_break() directly under it. Once any writer queues on the inode (a close taking the lock for write, e.g. via opinfo_del()), the inner down_read() deadlocks against it.

Port the deferred break machinery (oplock_break_add() and oplock_break_drain_none()) from the newer stable branches and make the three walks collect the breaks and drain them after releasing ci->m_lock, matching the 7.2.y and 6.12.y shape of the fix. The two remaining call sites, smb_grant_oplock() and smb_break_all_write_oplock(), take no lock around oplock_break(), so smb2_oplock_break_conn_get()'s internal lock is safe everywhere.

Fixes c8279ae8df68 ("ksmbd: fix use-after-free in oplock break notification")
Cc [email protected]
Signed-off-by Abdifatah Suruur <[email protected]>
---
 fs/smb/server/oplock.c | 63 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)
fs/smb/server/oplock.c +58 -5
index e627c1b93c8f7..935e337a95b8e 100644
@@ -1006,6 +1006,43 @@ static int oplock_break(struct oplock_info *brk_opinfo, struct ksmbd_inode *ci,
 	return err;
 }
 
+struct oplock_break_entry {
+	struct list_head	list;
+	struct oplock_info	*opinfo;
+};
+
+/*
+ * Collect an oplock for a deferred break.  oplock_break() may block for
+ * the client's break acknowledgment, and the close that wakes that wait
+ * needs ci->m_lock for write, so the walks that hold ci->m_lock defer the
+ * break until the lock is released.
+ */
+static int oplock_break_add(struct list_head *head, struct oplock_info *opinfo)
+{
+	struct oplock_break_entry *ent;
+
+	ent = kmalloc_obj(struct oplock_break_entry, KSMBD_DEFAULT_GFP);
+	if (!ent)
+		return -ENOMEM;
+
+	ent->opinfo = opinfo;
+	list_add_tail(&ent->list, head);
+	return 0;
+}
+
+static void oplock_break_drain_none(struct list_head *head,
+				    struct ksmbd_inode *ci)
+{
+	struct oplock_break_entry *ent, *tmp;
+
+	list_for_each_entry_safe(ent, tmp, head, list) {
+		oplock_break(ent->opinfo, ci, SMB2_OPLOCK_LEVEL_NONE, NULL);
+		list_del(&ent->list);
+		opinfo_put(ent->opinfo);
+		kfree(ent);
+	}
+}
+
 void destroy_lease_table(struct ksmbd_conn *conn)
 {
 	struct lease_table *lb, *lbtmp;

@@ -1149,6 +1186,7 @@ void smb_send_parent_lease_break_noti(struct ksmbd_file *fp,
 {
 	struct oplock_info *opinfo;
 	struct ksmbd_inode *p_ci = NULL;
+	LIST_HEAD(brk_list);
 
 	if (lctx->version != 2)
 		return;

@@ -1174,12 +1212,14 @@ void smb_send_parent_lease_break_noti(struct ksmbd_file *fp,
 				continue;
 			}
 
-			oplock_break(opinfo, p_ci, SMB2_OPLOCK_LEVEL_NONE, NULL);
-			opinfo_put(opinfo);
+			if (oplock_break_add(&brk_list, opinfo))
+				opinfo_put(opinfo);
 		}
 	}
 	up_read(&p_ci->m_lock);
 
+	oplock_break_drain_none(&brk_list, p_ci);
+
 	ksmbd_inode_put(p_ci);
 }
 

@@ -1187,6 +1227,7 @@ void smb_lazy_parent_lease_break_close(struct ksmbd_file *fp)
 {
 	struct oplock_info *opinfo;
 	struct ksmbd_inode *p_ci = NULL;
+	LIST_HEAD(brk_list);
 
 	rcu_read_lock();
 	opinfo = rcu_dereference(fp->f_opinfo);

@@ -1215,12 +1256,14 @@ void smb_lazy_parent_lease_break_close(struct ksmbd_file *fp)
 				continue;
 			}
 
-			oplock_break(opinfo, p_ci, SMB2_OPLOCK_LEVEL_NONE, NULL);
-			opinfo_put(opinfo);
+			if (oplock_break_add(&brk_list, opinfo))
+				opinfo_put(opinfo);
 		}
 	}
 	up_read(&p_ci->m_lock);
 
+	oplock_break_drain_none(&brk_list, p_ci);
+
 	ksmbd_inode_put(p_ci);
 }
 

@@ -1420,6 +1463,7 @@ void smb_break_all_levII_oplock(struct ksmbd_work *work, struct ksmbd_file *fp,
 	struct oplock_info *op, *brk_op;
 	struct ksmbd_inode *ci;
 	struct ksmbd_conn *conn = work->conn;
+	LIST_HEAD(brk_list);
 
 	if (!test_share_config_flag(work->tcon->share_conf,
 				    KSMBD_SHARE_FLAG_OPLOCKS))

@@ -1467,12 +1511,21 @@ void smb_break_all_levII_oplock(struct ksmbd_work *work, struct ksmbd_file *fp,
 			    SMB2_LEASE_KEY_SIZE))
 			goto next;
 		brk_op->open_trunc = is_trunc;
-		oplock_break(brk_op, ci, SMB2_OPLOCK_LEVEL_NONE, NULL);
+
+		/*
+		 * Defer the break until ci->m_lock is released: oplock_break()
+		 * may block waiting for the break acknowledgment, and the
+		 * close that wakes that wait needs ci->m_lock for write.
+		 */
+		if (!oplock_break_add(&brk_list, brk_op))
+			continue;
 next:
 		opinfo_put(brk_op);
 	}
 	up_read(&ci->m_lock);
 
+	oplock_break_drain_none(&brk_list, ci);
+
 	if (op)
 		opinfo_put(op);
 }
Signature · 1 line
2.53.0
maintainer
#2 lore raw
Quoted 4 lines

Port the deferred break machinery (oplock_break_add() and oplock_break_drain_none()) from the newer stable branches and make the three walks collect the breaks and drain them after releasing ci->m_lock, matching the 7.2.y and 6.12.y shape of the fix.

Holding this one for a Reviewed-by/Ack from a maintainer before queuing. It's porting the deferred-break mechanism onto 6.18 rather than a straight cherry-pick, and it's fixing a self-deadlock that's already shipped in v6.18.51.

Signature · 2 lines
Thanks,
Sasha
author
#3 lore raw
In reply to Abdifatah Suruur

Hi Namjae,

Sasha is holding this 6.18.y follow-up pending a maintainer review:

https://lore.kernel.org/stable/[email protected]/

The 6.18.y backport of the oplock UAF fix (c8279ae8df68, shipped in 6.18.51) calls oplock_break() directly while holding ci->m_lock, and the backported smb2_oplock_break_conn_get() takes down_read(ci->m_lock) internally -- a self-nested read lock that deadlocks as soon as a writer queues behind it. The patch ports the oplock_break_add()/ oplock_break_drain() machinery you have on newer branches so the three walks collect the breaks and drain them after releasing m_lock, matching the 7.2.y and 6.12.y shape of the fix.

Could you give it a Reviewed-by/Ack when you get a chance?

Thanks,
Abdifatah

Acked-by
#4 lore raw
Quoted 16 lines · Abdifatah Suruur

Hi Namjae,

Sasha is holding this 6.18.y follow-up pending a maintainer review:

https://lore.kernel.org/stable/[email protected]/

The 6.18.y backport of the oplock UAF fix (c8279ae8df68, shipped in 6.18.51) calls oplock_break() directly while holding ci->m_lock, and the backported smb2_oplock_break_conn_get() takes down_read(ci->m_lock) internally -- a self-nested read lock that deadlocks as soon as a writer queues behind it. The patch ports the oplock_break_add()/ oplock_break_drain() machinery you have on newer branches so the three walks collect the breaks and drain them after releasing m_lock, matching the 7.2.y and 6.12.y shape of the fix.

Quoted 1 line

Could you give it a Reviewed-by/Ack when you get a chance?

BTW, Could we keep the original upstream subject and description?

ksmbd: don't hold ci->m_lock while waiting for a lease break ack

This is a backport of 27a9bc968d4a, and we can add a brief description explaining why this backport is needed for 6.18.y.

Acked-by Namjae Jeon <[email protected]>

Thanks!

maintainer
#5 lore raw
In reply to Abdifatah Suruur
Quoted 4 lines

Port the deferred break machinery (oplock_break_add() and oplock_break_drain_none()) from the newer stable branches and make the three walks collect the breaks and drain them after releasing ci->m_lock, matching the 7.2.y and 6.12.y shape of the fix.

The code is fine, but Namjae's Ack came with a condition on how this is
presented: keep the upstream subject and describe it as a backport of 27a9bc968d4a ("ksmbd: don't hold ci->m_lock while waiting for a lease break ack").

Signature · 2 lines
Thanks,
Sasha
author
#6 lore raw

PATCH 1/1 ksmbd: don't hold ci->m_lock while waiting for a lease break ack

commit 27a9bc968d4ae809a3f5ca2f2b136856ee613cb4 upstream.

When a cifs.ko client caches a read-handle (RH) lease via deferred close and a conflicting open arrives, ksmbd breaks the lease and waits for the acknowledgment in wait_for_break_ack() for up to OPLOCK_WAIT_TIME (35s). __smb_break_all_levII_oplock() runs that wait while holding ci->m_lock for read.

cifs.ko reacts to a handle-lease break by closing the deferred handle rather than sending a lease break acknowledgment. That close path (close_id_del_oplock() -> opinfo_del()) takes ci->m_lock for write and is exactly what would wake the waiter, but it blocks on the read lock held by the waiting thread. The break is then resolved only by the 35s timeout, so xfstests generic/001 takes ~78s with leases enabled versus ~4s with oplocks only.

Collect the target opinfos (each pinned with a reference) while holding ci->m_lock, then break them after releasing it, matching how smb_grant_oplock() already breaks a conflicting lease using only a reference. The reference keeps the opinfo (and its conn and lease) alive across the unlocked window, and a close racing the break is handled by the existing OPLOCK_CLOSING state check. Apply the same fix to the parent lease break paths.

Signed-off-by Namjae Jeon <[email protected]>
Signed-off-by Steve French <[email protected]>

[ This tree predates the deferred break machinery. The 6.18.y backport of the oplock break UAF fix (applied here as c8279ae8df68) calls oplock_break() directly while holding ci->m_lock, and its smb2_oplock_break_conn_get() takes down_read(ci->m_lock) internally, so the three walks self-nest the read lock and deadlock for up to OPLOCK_WAIT_TIME (35s) once a writer queues on the inode. Port the machinery above so the walks collect the breaks and drain them after releasing ci->m_lock, matching the 7.2.y shape of the fix. ]

Acked-by Namjae Jeon <[email protected]>
Signed-off-by Abdifatah Suruur <[email protected]>

---
v2:
- reframe as a backport of upstream commit 27a9bc968d4a ("ksmbd:
don't hold ci->m_lock while waiting for a lease break ack"), keeping the upstream subject and description, per Namjae; carry his Acked-by

---
 fs/smb/server/oplock.c | 63 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)
fs/smb/server/oplock.c +58 -5
index e627c1b93c8f7..935e337a95b8e 100644
@@ -1006,6 +1006,43 @@ static int oplock_break(struct oplock_info *brk_opinfo, struct ksmbd_inode *ci,
 	return err;
 }
 
+struct oplock_break_entry {
+	struct list_head	list;
+	struct oplock_info	*opinfo;
+};
+
+/*
+ * Collect an oplock for a deferred break.  oplock_break() may block for
+ * the client's break acknowledgment, and the close that wakes that wait
+ * needs ci->m_lock for write, so the walks that hold ci->m_lock defer the
+ * break until the lock is released.
+ */
+static int oplock_break_add(struct list_head *head, struct oplock_info *opinfo)
+{
+	struct oplock_break_entry *ent;
+
+	ent = kmalloc_obj(struct oplock_break_entry, KSMBD_DEFAULT_GFP);
+	if (!ent)
+		return -ENOMEM;
+
+	ent->opinfo = opinfo;
+	list_add_tail(&ent->list, head);
+	return 0;
+}
+
+static void oplock_break_drain_none(struct list_head *head,
+				    struct ksmbd_inode *ci)
+{
+	struct oplock_break_entry *ent, *tmp;
+
+	list_for_each_entry_safe(ent, tmp, head, list) {
+		oplock_break(ent->opinfo, ci, SMB2_OPLOCK_LEVEL_NONE, NULL);
+		list_del(&ent->list);
+		opinfo_put(ent->opinfo);
+		kfree(ent);
+	}
+}
+
 void destroy_lease_table(struct ksmbd_conn *conn)
 {
 	struct lease_table *lb, *lbtmp;

@@ -1149,6 +1186,7 @@ void smb_send_parent_lease_break_noti(struct ksmbd_file *fp,
 {
 	struct oplock_info *opinfo;
 	struct ksmbd_inode *p_ci = NULL;
+	LIST_HEAD(brk_list);
 
 	if (lctx->version != 2)
 		return;

@@ -1174,12 +1212,14 @@ void smb_send_parent_lease_break_noti(struct ksmbd_file *fp,
 				continue;
 			}
 
-			oplock_break(opinfo, p_ci, SMB2_OPLOCK_LEVEL_NONE, NULL);
-			opinfo_put(opinfo);
+			if (oplock_break_add(&brk_list, opinfo))
+				opinfo_put(opinfo);
 		}
 	}
 	up_read(&p_ci->m_lock);
 
+	oplock_break_drain_none(&brk_list, p_ci);
+
 	ksmbd_inode_put(p_ci);
 }
 

@@ -1187,6 +1227,7 @@ void smb_lazy_parent_lease_break_close(struct ksmbd_file *fp)
 {
 	struct oplock_info *opinfo;
 	struct ksmbd_inode *p_ci = NULL;
+	LIST_HEAD(brk_list);
 
 	rcu_read_lock();
 	opinfo = rcu_dereference(fp->f_opinfo);

@@ -1215,12 +1256,14 @@ void smb_lazy_parent_lease_break_close(struct ksmbd_file *fp)
 				continue;
 			}
 
-			oplock_break(opinfo, p_ci, SMB2_OPLOCK_LEVEL_NONE, NULL);
-			opinfo_put(opinfo);
+			if (oplock_break_add(&brk_list, opinfo))
+				opinfo_put(opinfo);
 		}
 	}
 	up_read(&p_ci->m_lock);
 
+	oplock_break_drain_none(&brk_list, p_ci);
+
 	ksmbd_inode_put(p_ci);
 }
 

@@ -1420,6 +1463,7 @@ void smb_break_all_levII_oplock(struct ksmbd_work *work, struct ksmbd_file *fp,
 	struct oplock_info *op, *brk_op;
 	struct ksmbd_inode *ci;
 	struct ksmbd_conn *conn = work->conn;
+	LIST_HEAD(brk_list);
 
 	if (!test_share_config_flag(work->tcon->share_conf,
 				    KSMBD_SHARE_FLAG_OPLOCKS))

@@ -1467,12 +1511,21 @@ void smb_break_all_levII_oplock(struct ksmbd_work *work, struct ksmbd_file *fp,
 			    SMB2_LEASE_KEY_SIZE))
 			goto next;
 		brk_op->open_trunc = is_trunc;
-		oplock_break(brk_op, ci, SMB2_OPLOCK_LEVEL_NONE, NULL);
+
+		/*
+		 * Defer the break until ci->m_lock is released: oplock_break()
+		 * may block waiting for the break acknowledgment, and the
+		 * close that wakes that wait needs ci->m_lock for write.
+		 */
+		if (!oplock_break_add(&brk_list, brk_op))
+			continue;
 next:
 		opinfo_put(brk_op);
 	}
 	up_read(&ci->m_lock);
 
+	oplock_break_drain_none(&brk_list, ci);
+
 	if (op)
 		opinfo_put(op);
 }
Signature · 1 line
2.53.0
maintainer Applied
#7 lore raw

Re: [PATCH 6.18.y] ksmbd: don't hold ci->m_lock while waiting for a lease break ack

Quoted 5 lines

commit 27a9bc968d4ae809a3f5ca2f2b136856ee613cb4 upstream.

When a cifs.ko client caches a read-handle (RH) lease via deferred close and a conflicting open arrives, ksmbd breaks the lease and waits for the acknowledgment in wait_for_break_ack() for up to OPLOCK_WAIT_TIME (35s).

Queued for 6.18, thanks.

Signature · 2 lines
Thanks,
Sasha