linux-kernel - Re: [PATCH 02/34] maple_tree: Clean up mas_parent

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]

Message-ID: <20230426210708.r2gpnzocxfulz5y5@revolver>
Date:   Wed, 26 Apr 2023 17:07:08 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Peng Zhang <zhangpeng.00@...edance.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        maple-tree@...ts.infradead.org,
        Wei Yang <richard.weiyang@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 02/34] maple_tree: Clean up mas_parent_enum()

* Peng Zhang <zhangpeng.00@...edance.com> [230426 00:15]:
> 
> 在 2023/4/25 22:09, Liam R. Howlett 写道:
> > From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
> > 
> > mas_parent_enum() is a simple wrapper for mte_parent_enum() which is
> > only called from that wrapper.  Remove the wrapper and inline
> > mte_parent_enum() into mas_parent_enum().
> > 
> > At the same time, clean up the bit masking of the root pointer since it
> > cannot be set by the time the bit masking occurs.  Change the check on
> > the root bit to a WARN_ON(), and fix the verification code to not
> > trigger the WARN_ON() before checking if the node is root.
> > 
> > Reported-by: Wei Yang <richard.weiyang@...il.com>
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@...cle.com>
> > ---
> >   lib/maple_tree.c | 28 +++++++++++-----------------
> >   1 file changed, 11 insertions(+), 17 deletions(-)
> > 
> > diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> > index 9cf4fca42310c..ac0245dd88dad 100644
> > --- a/lib/maple_tree.c
> > +++ b/lib/maple_tree.c
> > @@ -428,25 +428,23 @@ static inline unsigned long mte_parent_slot_mask(unsigned long parent)
> >    * mas_parent_enum() - Return the maple_type of the parent from the stored
> >    * parent type.
> >    * @mas: The maple state
> > - * @node: The maple_enode to extract the parent's enum
> > + * @enode: The maple_enode to extract the parent's enum
> >    * Return: The node->parent maple_type
> >    */
> >   static inline
> > -enum maple_type mte_parent_enum(struct maple_enode *p_enode,
> > -				struct maple_tree *mt)
> > +enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode)
> 
> Do you think it's better to rename this function to mas_parent_type()?
> The meaning of enum is not obvious and there is already a similar
> function mte_node_type().

Yes, thanks. I'll make that change in v2.

> 
> >   {
> >   	unsigned long p_type;
> > -	p_type = (unsigned long)p_enode;
> > -	if (p_type & MAPLE_PARENT_ROOT)
> > -		return 0; /* Validated in the caller. */
> > +	p_type = (unsigned long)mte_to_node(enode)->parent;
> > +	if (WARN_ON(p_type & MAPLE_PARENT_ROOT))
> > +		return 0;
> >   	p_type &= MAPLE_NODE_MASK;
> > -	p_type = p_type & ~(MAPLE_PARENT_ROOT | mte_parent_slot_mask(p_type));
> > -
> > +	p_type &= ~mte_parent_slot_mask(p_type);
> >   	switch (p_type) {
> >   	case MAPLE_PARENT_RANGE64: /* or MAPLE_PARENT_ARANGE64 */
> > -		if (mt_is_alloc(mt))
> > +		if (mt_is_alloc(mas->tree))
> >   			return maple_arange_64;
> >   		return maple_range_64;
> >   	}
> > @@ -454,12 +452,6 @@ enum maple_type mte_parent_enum(struct maple_enode *p_enode,
> >   	return 0;
> >   }
> > -static inline
> > -enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode)
> > -{
> > -	return mte_parent_enum(ma_enode_ptr(mte_to_node(enode)->parent), mas->tree);
> > -}
> > -
> >   /*
> >    * mte_set_parent() - Set the parent node and encode the slot
> >    * @enode: The encoded maple node.
> > @@ -7008,14 +7000,16 @@ static void mas_validate_parent_slot(struct ma_state *mas)
> >   {
> >   	struct maple_node *parent;
> >   	struct maple_enode *node;
> > -	enum maple_type p_type = mas_parent_enum(mas, mas->node);
> > -	unsigned char p_slot = mte_parent_slot(mas->node);
> > +	enum maple_type p_type;
> > +	unsigned char p_slot;
> >   	void __rcu **slots;
> >   	int i;
> >   	if (mte_is_root(mas->node))
> >   		return;
> > +	p_slot = mte_parent_slot(mas->node);
> > +	p_type = mas_parent_enum(mas, mas->node);
> >   	parent = mte_parent(mas->node);
> >   	slots = ma_slots(parent, p_type);
> >   	MT_BUG_ON(mas->tree, mas_mn(mas) == parent);