linux-kernel - Re: [PATCH] include/log2.h: Fix rounddown_pow_of

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, 17 Nov 2011 15:05:49 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Andrei Warkentin <andreiw@...are.com>
Cc:	linux-kernel@...r.kernel.org,
	Rolf Eike Beer <eike-kernel@...tec.de>,
	opensuse-kernel@...nsuse.org,
	Sergiu Iordache <sergiu@...omium.org>,
	Marco Stornelli <marco.stornelli@...il.com>,
	Eddie Wai <eddie.wai@...adcom.com>,
	Jayamohan Kallickal <jayamohan.kallickal@...lex.com>,
	Guennadi Liakhovetski <g.liakhovetski@....de>
Subject: Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

On Wed, 16 Nov 2011 14:56:06 -0500
Andrei Warkentin <andreiw@...are.com> wrote:

> 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1. It does
> in case the argument is a variable but in case it's a constant it behaves
> wrong and returns 0. Probably nobody ever did it so this was never noticed,
> however net/drivers/vmxnet3 with latest GCC does and breaks on unicpu systems.
> 
> This is similar to Rolf's patch to roundup_pow_of_two(1).
> 
> Cc: Rolf Eike Beer <eike-kernel@...tec.de>
> Cc: opensuse-kernel@...nsuse.org
> Reviewed-by: Jesper Juhl <jj@...osbits.net>
> Signed-off-by: Andrei Warkentin <andreiw@...are.com>
> ---
>  include/linux/log2.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index 25b8086..ccda848 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h
> @@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>  #define rounddown_pow_of_two(n)			\
>  (						\
>  	__builtin_constant_p(n) ? (		\
> -		(n == 1) ? 0 :			\
> +		(n == 1) ? 1 :			\
>  		(1UL << ilog2(n))) :		\
>  	__rounddown_pow_of_two(n)		\
>   )

I assume that nobody has gone off and checked whether all current
callers will survive this change.  If they had, they'd have looked in
drivers/char/ramoops.c and seen:

	rounddown_pow_of_two(pdata->mem_size);
	rounddown_pow_of_two(pdata->record_size);

These operations are no-ops.  It should be

	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
	pdata->record_size = rounddown_pow_of_two(pdata->record_size);

Marco or Sergio: please fix, test and send it over sometime?


drivers/scsi/bnx2i/bnx2i_hwi.c does

                if (!is_power_of_2(hba->max_sqes))
                        hba->max_sqes = rounddown_pow_of_two(hba->max_sqes);

                if (!is_power_of_2(hba->max_rqes))
                        hba->max_rqes = rounddown_pow_of_two(hba->max_rqes);


Both the "if" statements can and should be removed.  I would blame upon
inadequate documentation of rounddown_pow_of_two().


drivers/scsi/be2iscsi/be_main.c does

	if (curr_alloc_size - rounddown_pow_of_two(curr_alloc_size))
		curr_alloc_size = rounddown_pow_of_two(curr_alloc_size);

which is a strange way of doing

	if (!is_power_of_2(curr_alloc_size))
		curr_alloc_size = rounddown_pow_of_two(curr_alloc_size);

which is equivalent to doing

	curr_alloc_size = rounddown_pow_of_two(curr_alloc_size);

but there's an `else' clause to that `if' which I am presently finding
incomprehensible.


drivers/mmc/host/sh_mmcif.c unnecessarily uses __rounddown_pow_of_two()
then feeds the result into ilog2() in an apparent attempt to
reimplement fls().



That we have this many warts using these interfaces is an indication
that the interfaces aren't very good.  Poorly documented, at least.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/