From: Andrew Morton <akpm@linux-foundation.org>
To: Cesar Eduardo Barros <cesarb@cesarb.net>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Russell King <linux@arm.linux.org.uk>,
Ralf Baechle <ralf@linux-mips.org>,
David Howells <dhowells@redhat.com>,
Koichi Yasutake <yasutake.koichi@jp.panasonic.com>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
"David S. Miller" <davem@davemloft.net>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
x86@kernel.org, Arnd Bergmann <arnd@arndb.de>,
Rusty Russell <rusty@rustcorp.com.au>
Subject: Re: [PATCH] Make kunmap_atomic() harder to misuse
Date: Sat, 29 May 2010 20:42:56 -0700 [thread overview]
Message-ID: <20100529204256.b92b1ff6.akpm@linux-foundation.org> (raw)
In-Reply-To: <1275043993-26557-1-git-send-email-cesarb@cesarb.net>
On Fri, 28 May 2010 07:53:13 -0300 Cesar Eduardo Barros <cesarb@cesarb.net> wrote:
> kunmap_atomic() is currently at level -4 on Rusty's "Hard To Misuse"
> list[1] ("Follow common convention and you'll get it wrong"), except in
> some architectures when CONFIG_DEBUG_HIGHMEM is set[2][3].
>
> kunmap() takes a pointer to a struct page; kunmap_atomic(), however,
> takes takes a pointer to within the page itself. This seems to once in a
> while trip people up (the convention they are following is the one from
> kunmap()).
>
> Make it much harder to misuse, by moving it to level 9 on Rusty's
> list[4] ("The compiler/linker won't let you get it wrong"). This is done
> by refusing to build if the pointer passed to it is convertible to a
> struct page * but it is not a void * (verified by trying to convert it
> to a pointer to a dummy struct).
>
> The real kunmap_atomic() is renamed to kunmap_atomic_notypecheck()
> (which is what you would call in case for some strange reason calling it
> with a pointer to a struct page is not incorrect in your code).
>
Fair enough, that's a 99% fix. A long time ago I made kmap_atomic()
return a char * (iirc) and kunmap_atomic() is passed a char*. It
worked, but I ended up throwing it away. I don't precisely remember
why - I think it was intrusiveness and general hassle rather than
anything fundamental.
>
> ...
>
> +/* Prevent people trying to call kunmap_atomic() as if it were kunmap() */
> +struct __kunmap_atomic_dummy {};
> +#define kunmap_atomic(addr, idx) do { \
> + BUILD_BUG_ON( \
> + __builtin_types_compatible_p(typeof(addr), struct page *) && \
> + !__builtin_types_compatible_p(typeof(addr), struct __kunmap_atomic_dummy *)); \
> + kunmap_atomic_notypecheck((addr), (idx)); \
> + } while (0)
<looks around>
OK, it seems that __builtin_types_compatible_p() is supported on all
approved gcc versions.
We have a little __same_type() helper for this. __must_be_array()
should be using it, too.
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Morton <akpm@linux-foundation.org>
To: Cesar Eduardo Barros <cesarb@cesarb.net>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Russell King <linux@arm.linux.org.uk>,
Ralf Baechle <ralf@linux-mips.org>,
David Howells <dhowells@redhat.com>,
Koichi Yasutake <yasutake.koichi@jp.panasonic.com>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
"David S. Miller" <davem@davemloft.net>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
x86@kernel.org, Arnd Bergmann <arnd@arndb.de>,
Rusty Russell <rusty@rustcorp.com.au>
Subject: Re: [PATCH] Make kunmap_atomic() harder to misuse
Date: Sat, 29 May 2010 20:42:56 -0700 [thread overview]
Message-ID: <20100529204256.b92b1ff6.akpm@linux-foundation.org> (raw)
In-Reply-To: <1275043993-26557-1-git-send-email-cesarb@cesarb.net>
On Fri, 28 May 2010 07:53:13 -0300 Cesar Eduardo Barros <cesarb@cesarb.net> wrote:
> kunmap_atomic() is currently at level -4 on Rusty's "Hard To Misuse"
> list[1] ("Follow common convention and you'll get it wrong"), except in
> some architectures when CONFIG_DEBUG_HIGHMEM is set[2][3].
>
> kunmap() takes a pointer to a struct page; kunmap_atomic(), however,
> takes takes a pointer to within the page itself. This seems to once in a
> while trip people up (the convention they are following is the one from
> kunmap()).
>
> Make it much harder to misuse, by moving it to level 9 on Rusty's
> list[4] ("The compiler/linker won't let you get it wrong"). This is done
> by refusing to build if the pointer passed to it is convertible to a
> struct page * but it is not a void * (verified by trying to convert it
> to a pointer to a dummy struct).
>
> The real kunmap_atomic() is renamed to kunmap_atomic_notypecheck()
> (which is what you would call in case for some strange reason calling it
> with a pointer to a struct page is not incorrect in your code).
>
Fair enough, that's a 99% fix. A long time ago I made kmap_atomic()
return a char * (iirc) and kunmap_atomic() is passed a char*. It
worked, but I ended up throwing it away. I don't precisely remember
why - I think it was intrusiveness and general hassle rather than
anything fundamental.
>
> ...
>
> +/* Prevent people trying to call kunmap_atomic() as if it were kunmap() */
> +struct __kunmap_atomic_dummy {};
> +#define kunmap_atomic(addr, idx) do { \
> + BUILD_BUG_ON( \
> + __builtin_types_compatible_p(typeof(addr), struct page *) && \
> + !__builtin_types_compatible_p(typeof(addr), struct __kunmap_atomic_dummy *)); \
> + kunmap_atomic_notypecheck((addr), (idx)); \
> + } while (0)
<looks around>
OK, it seems that __builtin_types_compatible_p() is supported on all
approved gcc versions.
We have a little __same_type() helper for this. __must_be_array()
should be using it, too.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2010-05-30 3:44 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-05-28 10:53 [PATCH] Make kunmap_atomic() harder to misuse Cesar Eduardo Barros
2010-05-28 10:53 ` Cesar Eduardo Barros
2010-05-30 3:42 ` Andrew Morton [this message]
2010-05-30 3:42 ` Andrew Morton
2010-05-30 17:42 ` Cesar Eduardo Barros
2010-05-30 17:42 ` Cesar Eduardo Barros
2010-05-31 10:15 ` Rusty Russell
2010-05-31 10:15 ` Rusty Russell
2010-05-31 10:45 ` Cesar Eduardo Barros
2010-05-31 10:45 ` Cesar Eduardo Barros
2010-05-31 23:45 ` [PATCH v2] " Cesar Eduardo Barros
2010-05-31 23:45 ` Cesar Eduardo Barros
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100529204256.b92b1ff6.akpm@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=arnd@arndb.de \
--cc=benh@kernel.crashing.org \
--cc=cesarb@cesarb.net \
--cc=davem@davemloft.net \
--cc=dhowells@redhat.com \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux@arm.linux.org.uk \
--cc=mingo@redhat.com \
--cc=paulus@samba.org \
--cc=ralf@linux-mips.org \
--cc=rusty@rustcorp.com.au \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
--cc=yasutake.koichi@jp.panasonic.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.