On 2023/03/27 9:04, Al Viro wrote:
> However, I don't think this is the right fix. Note that the problem is
> with get_branch() done under the rwlock; all other places are safe. But
> in get_branch() we only need the lock (and only shared, at that) around
> the verify+add pair. See how it's done in fs/minix/itree_common.c...
> Something like this:
I can see fs/minix/itree_common.c is doing similar things. But since I'm not
familiar with filesystems, I can't be convinced that minix's assumption is safe.
> static Indirect *find_shared(struct inode *inode,
> int depth,
> int offsets[],
> Indirect chain[],
> sysv_zone_t *top)
> {
> Indirect *partial, *p;
> int k, err;
>
> *top = 0;
> for (k = depth; k > 1 && !offsets[k-1]; k--)
> ;
>
> - write_lock(&pointers_lock);
> partial = get_branch(inode, k, offsets, chain, &err);
Does the result of verify_chain() from get_branch() remain valid even after
sleep by preemption at this line made get_branch() to execute "*err = -EAGAIN;"
line rather than "return NULL;" line?
> if (!partial)
> partial = chain + k-1;
Can sleep by preemption at this line or
> +
> + write_lock(&pointers_lock);
> /*
> * If the branch acquired continuation since we've looked at it -
> * fine, it should all survive and (new) top doesn't belong to us.
> */
> if (!partial->key && *partial->p) {
> write_unlock(&pointers_lock);
at this line change the result of "!partial->key && *partial->p" test above?
> goto no_top;
> }
> for (p=partial; p>chain && all_zeroes((sysv_zone_t*)p->bh->b_data,p->p); p--)
> ;
> /*
> * OK, we've found the last block that must survive. The rest of our
> * branch should be detached before unlocking. However, if that rest
> * of branch is all ours and does not grow immediately from the inode
> * it's easier to cheat and just decrement partial->p.
> */
> if (p == chain + k - 1 && p > chain) {
> p->p--;
> } else {
> *top = *p->p;
> *p->p = 0;
> }
> write_unlock(&pointers_lock);
>
> while (partial > p) {
> brelse(partial->bh);
> partial--;
> }
> no_top:
> return partial;
> }
I feel worried about
/*
* Indirect block might be removed by truncate while we were
* reading it. Handling of that case (forget what we've got and
* reread) is taken out of the main path.
*/
if (err == -EAGAIN)
goto changed;
in get_block()...
