Hi Linus,
On 2023/10/31 06:18, Linus Torvalds wrote:
On Mon, 30 Oct 2023 at 11:53, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
After fixing a couple of brainos, it seems to work.
This all makes me unnaturally nervous, probably because it;s overly
subtle, and I have lost the context for some of the rules.
I like the patch, because honestly, our current logic for dput_fast()
is nasty, andI agree with you that the existence of d_op->d_delete()
shouldn't change the locking logic.
At the same time, I just worry. That whole lockref_put_return() thing
has horrific semantics, and this is the only case that uses it, and I
wish we didn't need it.
[ Looks around. Oh. Except we have lockref_put_return() in fs/erofs/
too, and that looks completely bogus, since it doesn't check the
return value! ]
74 struct erofs_workgroup *erofs_insert_workgroup(struct super_block *sb,
75 struct erofs_workgroup *grp)
76 {
77 struct erofs_sb_info *const sbi = EROFS_SB(sb);
78 struct erofs_workgroup *pre;
79
80 /*
81 * Bump up before making this visible to others for the XArray in order
82 * to avoid potential UAF without serialized by xa_lock.
83 */
84 lockref_get(&grp->lockref);
85
86 repeat:
87 xa_lock(&sbi->managed_pslots);
88 pre = __xa_cmpxchg(&sbi->managed_pslots, grp->index,
89 NULL, grp, GFP_NOFS);
90 if (pre) {
91 if (xa_is_err(pre)) {
92 pre = ERR_PTR(xa_err(pre));
93 } else if (!erofs_workgroup_get(pre)) {
94 /* try to legitimize the current in-tree one */
95 xa_unlock(&sbi->managed_pslots);
96 cond_resched();
97 goto repeat;
98 }
99 lockref_put_return(&grp->lockref);
This line it just decreases the reference count just bumpped up at the
line 84 (and it will always succeed).
Since it finds a previous one at line 88, so the old one will be used
(and be returned) instead of the new one and the new allocated one
will be freed in the caller.
Hopefully it explains the use case here.
100 grp = pre;
101 }
102 xa_unlock(&sbi->managed_pslots);
103 return grp;
104 }
Thanks,
Gao Xiang
