diff mbox

Handle SForced in storage_modifiers

Message ID 1479150736-28392-1-git-send-email-jlayton@redhat.com (mailing list archive)
State Rejected, archived
Headers show

Commit Message

Jeff Layton Nov. 14, 2016, 7:12 p.m. UTC
We have been seeing errors like this for a while now in the sparse
Fedora package, when doing kernel builds:

    ./include/linux/err.h:53:25: warning: dereference of noderef expression
    ./include/linux/err.h:35:16: warning: dereference of noderef expression

This spews all over the build because this comes from IS_ERR(), which
is called everywhere. Even odder, it turns out that if we build the
package with -fpic turned off, then it works fine.

With some brute-force debugging, I think I've finally found the cause.
This array is missing the SForced element. When this is added then the
problem goes away.

As to why this goes away when -fpic is removed, I can only assume that
we get lucky with the memory layout and have a zeroed out region just
beyond the end of the array.

Fixes: 3829c4d8b097776e6b3472290a9fae08a705ab7a
Cc: Al Viro <viro@ftp.linux.org.uk>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 parse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Luc Van Oostenryck Nov. 14, 2016, 8:04 p.m. UTC | #1
On Mon, Nov 14, 2016 at 02:12:16PM -0500, Jeff Layton wrote:
> We have been seeing errors like this for a while now in the sparse
> Fedora package, when doing kernel builds:
> 
>     ./include/linux/err.h:53:25: warning: dereference of noderef expression
>     ./include/linux/err.h:35:16: warning: dereference of noderef expression
> 
> This spews all over the build because this comes from IS_ERR(), which
> is called everywhere. Even odder, it turns out that if we build the
> package with -fpic turned off, then it works fine.
> 
> With some brute-force debugging, I think I've finally found the cause.
> This array is missing the SForced element. When this is added then the
> problem goes away.
> 
> As to why this goes away when -fpic is removed, I can only assume that
> we get lucky with the memory layout and have a zeroed out region just
> beyond the end of the array.
> 
> Fixes: 3829c4d8b097776e6b3472290a9fae08a705ab7a
> Cc: Al Viro <viro@ftp.linux.org.uk>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  parse.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/parse.c b/parse.c
> index 205e12644a6c..b290ff2636f2 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -1286,7 +1286,8 @@ static unsigned long storage_modifiers(struct decl_state *ctx)
>  		[SAuto] = MOD_AUTO,
>  		[SExtern] = MOD_EXTERN,
>  		[SStatic] = MOD_STATIC,
> -		[SRegister] = MOD_REGISTER
> +		[SRegister] = MOD_REGISTER,
> +		[SForced] = 0
>  	};
>  	return mod[ctx->storage_class] | (ctx->is_inline ? MOD_INLINE : 0)
>  		| (ctx->is_tls ? MOD_TLS : 0);
> -- 

Humm,

The array is statically initialized and never modified,
your patch shouldn't change anything, and this regardless of
the memory layout or compiler options.


Luc Van Oostenryck
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds Nov. 14, 2016, 8:17 p.m. UTC | #2
On Mon, Nov 14, 2016 at 12:04 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> The array is statically initialized and never modified,
> your patch shouldn't change anything, and this regardless of
> the memory layout or compiler options.

The problem is the _size_ of the array. Without that initializer for
SForced case, it is one entry too small, and you get a random access
past the end of the array.

The patch is definitely correct.

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luc Van Oostenryck Nov. 14, 2016, 8:21 p.m. UTC | #3
On Mon, Nov 14, 2016 at 12:17:47PM -0800, Linus Torvalds wrote:
> On Mon, Nov 14, 2016 at 12:04 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > The array is statically initialized and never modified,
> > your patch shouldn't change anything, and this regardless of
> > the memory layout or compiler options.
> 
> The problem is the _size_ of the array. Without that initializer for
> SForced case, it is one entry too small, and you get a random access
> past the end of the array.
> 
> The patch is definitely correct.
> 
>               Linus
> --

Ah yes, indeed.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/parse.c b/parse.c
index 205e12644a6c..b290ff2636f2 100644
--- a/parse.c
+++ b/parse.c
@@ -1286,7 +1286,8 @@  static unsigned long storage_modifiers(struct decl_state *ctx)
 		[SAuto] = MOD_AUTO,
 		[SExtern] = MOD_EXTERN,
 		[SStatic] = MOD_STATIC,
-		[SRegister] = MOD_REGISTER
+		[SRegister] = MOD_REGISTER,
+		[SForced] = 0
 	};
 	return mod[ctx->storage_class] | (ctx->is_inline ? MOD_INLINE : 0)
 		| (ctx->is_tls ? MOD_TLS : 0);