linux-kernel - Re: [PATCH] hashtable: remove a redundant check in hash_for_each

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:   Thu, 07 Oct 2021 08:16:11 +1100
From:   "NeilBrown" <neilb@...e.de>
To:     "Wei Yang" <richard.weiyang@...il.com>
Cc:     kuba@...nel.org, gregkh@...uxfoundation.org, mojha@...eaurora.org,
        jkosina@...e.cz, linux-kernel@...r.kernel.org,
        "Wei Yang" <richard.weiyang@...il.com>
Subject: Re: [PATCH] hashtable: remove a redundant check in hash_for_each_xxx()

On Thu, 07 Oct 2021, Wei Yang wrote:
> The three hash_for_each_xxx() helper iterate the hash table with help
> of hlist_for_each_entry_xxx(), which breaks the loop only when obj is
> NULL.
> 
> This means the check during each iteration is redundant. This patch
> removes it.
> 
> Signed-off-by: Wei Yang <richard.weiyang@...il.com>
> ---
>  include/linux/hashtable.h | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/hashtable.h b/include/linux/hashtable.h
> index f6c666730b8c..a15719ed303f 100644
> --- a/include/linux/hashtable.h
> +++ b/include/linux/hashtable.h
> @@ -124,8 +124,7 @@ static inline void hash_del_rcu(struct hlist_node *node)
>   * @member: the name of the hlist_node within the struct
>   */
>  #define hash_for_each(name, bkt, obj, member)				\
> -	for ((bkt) = 0, obj = NULL; obj == NULL && (bkt) < HASH_SIZE(name);\
> -			(bkt)++)\
> +	for ((bkt) = 0, obj = NULL; (bkt) < HASH_SIZE(name); (bkt)++)	\
>  		hlist_for_each_entry(obj, &name[bkt], member)

I think you are missing an important property of this code.
What we have here is a new loop command (hash_for_each()) that is
constructed from 2 nested loops.  This sort of construct is in general
difficult to use because in C it is common to use "break" to exit a loop
early.  'break' cannot exit two levels of loop though.  So if you aren't
careful, doing something like

  hash_for_each() {
     do something
     if (some test)
        break;
  }

might not do what you expect.  The 'break' will exit the inner loop, but
not the outer loop.  That could easily lead to buggy code.

But this macro *is* careful.  If the loop body *does* use break, then
the inner loop will abort but 'obj' will still be non-NULL.  The test
for NULL in the outer loop causes the outer loop to abort too - as the
programmer probably expected.

So by removing the 'obj == NULL' test, you would cause any usage which
breaks out of the loop to now be incorrect.

I recommend that instead of this patch, you provide a patch which
improves the documentation to make this clear. e.g.

  Note: it is safe to 'break' out of this loop even though it is a two
  nested loops.  The 'obj == NULL' test ensures that when the inner loop
  is broken, the outer loop will break too.

Thanks,
NeilBrown

>  
>  /**
> @@ -136,8 +135,7 @@ static inline void hash_del_rcu(struct hlist_node *node)
>   * @member: the name of the hlist_node within the struct
>   */
>  #define hash_for_each_rcu(name, bkt, obj, member)			\
> -	for ((bkt) = 0, obj = NULL; obj == NULL && (bkt) < HASH_SIZE(name);\
> -			(bkt)++)\
> +	for ((bkt) = 0, obj = NULL; (bkt) < HASH_SIZE(name); (bkt)++)	\
>  		hlist_for_each_entry_rcu(obj, &name[bkt], member)
>  
>  /**
> @@ -150,8 +148,7 @@ static inline void hash_del_rcu(struct hlist_node *node)
>   * @member: the name of the hlist_node within the struct
>   */
>  #define hash_for_each_safe(name, bkt, tmp, obj, member)			\
> -	for ((bkt) = 0, obj = NULL; obj == NULL && (bkt) < HASH_SIZE(name);\
> -			(bkt)++)\
> +	for ((bkt) = 0, obj = NULL; (bkt) < HASH_SIZE(name); (bkt)++)	\
>  		hlist_for_each_entry_safe(obj, tmp, &name[bkt], member)
>  
>  /**
> -- 
> 2.23.0
> 
>