linux-kernel - Re: [PATCH v2] perf tools: fix type mismatch

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] [day] [month] [year] [list]

Message-ID: <C2D7FE5348E1B147BCA15975FBA2307565BC6671@IN01WEMBXA.internal.synopsys.com>
Date:	Mon, 15 Dec 2014 07:14:39 +0000
From:	Vineet Gupta <Vineet.Gupta1@...opsys.com>
To:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>,
	"acme@...hat.com" <acme@...hat.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Borislav Petkov" <bp@...e.de>, Jiri Olsa <jolsa@...nel.org>,
	Cody P Schafer <dev@...yps.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v2] perf tools: fix type mismatch - long vs __statfs_word

On Tuesday 16 September 2014 08:46 PM, Alexey Brodkin wrote:
> In case of compilation with "-Wextra" following breakage happens:
> --->---
> fs.c: In function ‘fs__valid_mount’:
> fs.c:76:24: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>   else if (st_fs.f_type != magic)
>                         ^
> cc1: all warnings being treated as errors
> --->---
>
> Note that now when fs.c is in "lib/api/fs" and in "tools/lib/api/Makefile"
> CFLAGS has hard-coded "-Wextra" this is inevitable even if one builds "perf"
> with "WERROR=0".
>
> Even though "debugfs.c" gets compiled successfully because DEBUGFS_MAGIC is
> definitely positive (MSB bit is not set, so compiler doesn't care about
> explisit cast to "long") it's good to cast "st_fs.f_type" to "long" as well.
>
> And here's an explanation of observed issue.
>
> Perf tools use libc headers instead of ones from kernel source tree.
> This is because "perf" is just a user-space tool even thought its sources are
> merged in Linux kernel source tree.
> And so "statfs.h" comes from either uClibc, glibc or musl.
>
> In case of uClibc most of architectures use "legacy" version from here:
> http://git.uclibc.org/uClibc/tree/libc/sysdeps/linux/common/bits/statfs.h
>
> New UAPI compliant architectures (looks like for now it's only ARC) uses UAPI
> version of "statfs.h" from here:
> http://git.uclibc.org/uClibc/tree/libc/sysdeps/linux/common-generic/bits/statfs.h
>
> In this particular situation most interesting difference is in data type used
> for "struct statfs" member "f_type".
>
> In "legacy" version it's:
> --->---
> __SWORD_TYPE f_type;
> --->---
>
> In UAPI version it's:
> --->---
> __U32_TYPE f_type;
> --->---
>
> In its turn mentioned types are defined in
> http://git.uclibc.org/uClibc/tree/libc/sysdeps/linux/common/bits/types.h
> this way:
> --->---
> ...
> --->---
>
> And for 32-bit architectures "int" is of the same length as "long" so compiler
> is happy on data type check.
>
> But if "f_type" is "unsigned int" it's definitely not an unsigned "long".
>
> Essential fix is explicit casting of "f_type" to "long".
>
> Signed-off-by: Alexey Brodkin <abrodkin@...opsys.com>
>
> Cc: Vineet Gupta <vgupta@...opsys.com>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Cody P Schafer <dev@...yps.com>
> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> ---
>
> Changes in v2:
>
>  * Added type cast to DEBUGFS_MAGIC in "debugfs.c"
>  * Added verbose explanation of root cause
>
> Thanks a lot to Arnaldo for mention of another instance of hidden missing
> cast in "debugfs.c".
>
> ---
>  tools/lib/api/fs/debugfs.c | 2 +-
>  tools/lib/api/fs/fs.c      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/api/fs/debugfs.c b/tools/lib/api/fs/debugfs.c
> index a74fba6..93aa4cd 100644
> --- a/tools/lib/api/fs/debugfs.c
> +++ b/tools/lib/api/fs/debugfs.c
> @@ -67,7 +67,7 @@ int debugfs_valid_mountpoint(const char *debugfs)
>  
>  	if (statfs(debugfs, &st_fs) < 0)
>  		return -ENOENT;
> -	else if (st_fs.f_type != (long) DEBUGFS_MAGIC)
> +	else if ((long) st_fs.f_type != (long) DEBUGFS_MAGIC)

Hi Arnaldo,

Does this fix look good to you. We need something like this to enable upstream ARC
kernels to build perf.
OTOH do we really need the cast on both sides for this 2nd hunk ?

Thx,
-Vineet

>  		return -ENOENT;
>  
>  	return 0;
> diff --git a/tools/lib/api/fs/fs.c b/tools/lib/api/fs/fs.c
> index c1b49c3..a9fd78b 100644
> --- a/tools/lib/api/fs/fs.c
> +++ b/tools/lib/api/fs/fs.c
> @@ -75,7 +75,7 @@ static int fs__valid_mount(const char *fs, long magic)
>  
>  	if (statfs(fs, &st_fs) < 0)
>  		return -ENOENT;
> -	else if (st_fs.f_type != magic)
> +	else if ((long) st_fs.f_type != magic)
>  		return -ENOENT;
>  
>  	return 0;

--
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/