linux-kernel - Re: [PATCH] Staging: pi433: fix some warnings detected using sparse

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: <20170728141707.sdp7yy236247olad@mwanda>
Date:   Fri, 28 Jul 2017 17:17:07 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Elia Geretto <elia.f.geretto@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: pi433: fix some warnings detected using sparse

On Fri, Jul 28, 2017 at 02:56:26PM +0200, Elia Geretto wrote:
> This patch corrects some visibility issues regarding some functions and
> solves a warning related to a non-matching union. After this patch,
> sparse produces only one other warning regarding a bitwise operator;
> however, this behaviour seems to be intended.

I can't understand this changelog at all....  :/  What are we fixing
exactly?  It seems like we're fixing something about bitwise
operators...  I guess let me check the Sparse warnings...  Here they are
from the latest linux-next:

drivers/staging/pi433/pi433_if.c:211:9: warning: mixing different enum types
drivers/staging/pi433/pi433_if.c:211:9:     int enum optionOnOff  versus
drivers/staging/pi433/pi433_if.c:211:9:     int enum packetFormat 
drivers/staging/pi433/pi433_if.c:211:9: warning: mixing different enum types
drivers/staging/pi433/pi433_if.c:211:9:     int enum optionOnOff  versus
drivers/staging/pi433/pi433_if.c:211:9:     int enum packetFormat 
drivers/staging/pi433/pi433_if.c:268:9: warning: mixing different enum types
drivers/staging/pi433/pi433_if.c:268:9:     int enum optionOnOff  versus
drivers/staging/pi433/pi433_if.c:268:9:     int enum packetFormat 
drivers/staging/pi433/pi433_if.c:268:9: warning: mixing different enum types
drivers/staging/pi433/pi433_if.c:268:9:     int enum optionOnOff  versus
drivers/staging/pi433/pi433_if.c:268:9:     int enum packetFormat 
drivers/staging/pi433/pi433_if.c:317:1: warning: symbol 'pi433_receive' was not declared. Should it be static?
drivers/staging/pi433/pi433_if.c:467:1: warning: symbol 'pi433_tx_thread' was not declared. Should it be static?
drivers/staging/pi433/pi433_if.c:1155:36: error: incompatible types for operation (<)
drivers/staging/pi433/pi433_if.c:1155:36:    left side has type struct task_struct *tx_task_struct
drivers/staging/pi433/pi433_if.c:1155:36:    right side has type int
drivers/staging/pi433/rf69.c:206:17: warning: dubious: x & !y
drivers/staging/pi433/rf69.c:436:5: warning: symbol 'rf69_set_bandwidth_intern' was not declared. Should it be static?

Each type of fix should be sent as a separate fix with a better
changelog.  People have already done the "static" fixes and IS_ERR()
fixes, so don't worry about those.  But I don't think anyway has fixed
the enum issues so resend that.  Also the bitwise thing is a real bug,
but there is already a fix for that, it just hasn't been merged yet.

> 
> Signed-off-by: Elia Geretto <elia.f.geretto@...il.com>
> ---
>  drivers/staging/pi433/pi433_if.c | 17 +++++++++++------
>  drivers/staging/pi433/rf69.c     |  4 +++-
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index d9328ce5ec1d..f8219a53ce60 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -208,7 +208,10 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
>  	{
>  		SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
>  	}
> -	SET_CHECKED(rf69_set_packet_format  (dev->spi, rx_cfg->enable_length_byte));
> +	if (rx_cfg->enable_length_byte == optionOn)
> +		SET_CHECKED(rf69_set_packet_format(dev->spi, packetLengthVar));
> +	else
> +		SET_CHECKED(rf69_set_packet_format(dev->spi, packetLengthFix));

The SET_CHECKED() macro is total garbage.  It has a hidden return and
it calls the rf69_set_packet_format() twice on error it expands to:

	if (rf69_set_packet_format(dev->spi, rx_cfg->enable_length_byte)) < 0)
		return rf69_set_packet_format(dev->spi, rx_cfg->enable_length_byte);

Mega turbo barf!  Kill it with fire!

regards,
dan carpenter