linux-kernel - Re: [PATCH v3 2/2] regmap: Add maple tree based register cache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 Apr 2023 19:05:52 +0100
From:   Mark Brown <broonie@...nel.org>
To:     "Liam R. Howlett" <Liam.Howlett@...cle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] regmap: Add maple tree based register cache

On Mon, Apr 03, 2023 at 11:45:08AM -0400, Liam R. Howlett wrote:
> * Mark Brown <broonie@...nel.org> [230329 20:10]:

> > +	/* Any adjacent entries to extend/merge? */
> > +	mas_set_range(&mas, reg - 1, reg + 1);
> > +	index = reg;
> > +	last = reg;
> > +
> > +	lower = mas_find(&mas, reg - 1);

> If you just want to check the previous, you can use:
> mas_prev(&mas, reg - 1);
> This will try the previous entry without rewalking from the top of the
> tree and you don't need to mas_set_range() call.

I did give this a spin but it doesn't seem to be doing what I'd expect
for indexes 0 and 1, in a register map with index 0 if we attempt to
insert index 1 the mas_prev() doesn't find the existing entry for index
0 so we don't attempt to combine them.  It seems to do the right thing
for non-zero indexes, I'll have a poke at some point.

> > +	/*
> > +	 * This is safe because the regmap lock means the Maple lock
> > +	 * is redundant, but we need to take it due to lockdep asserts
> > +	 * in the maple tree code.
> > +	 */
> > +	mas_lock(&mas);

> > +	mas_set_range(&mas, index, last);
> > +	ret = mas_store_gfp(&mas, entry, GFP_KERNEL);

> You can avoid this walk as well by changing the order of the code
> before:

> mas_walk(&mas, reg);
> if entry... return
> mas_next(&mas, reg + 1);
> ...
> mas_prev(&mas, reg - 1);
> ...

> This should now be pointing at the location mas_store_gfp() expects:
> mas.last = last;
> ret = mas_store_gfp()

This appears to be triggering data corruption for me in the cache drop
test, again I'll have a poke at some point.  We seem to be getting
rubbish for the upper data block, though only in the drop test which
means it's possibly an interaction with how the tree is affected by
dropping the middle of a block.  Might well be something stupid I'm
doing either here or in the drop function.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)