[RFC] is_compat_task helper

[RFC] is_compat_task helper

[Kyle McMartin]

As many people have noticed, there's a growing need for some
generic code to know if a task is being run under 32 on 64 or
other such situations requiring compat conversions. Adding
the is_compat_task() macro provides a nice way to abstract this,
which is currently a mess of #ifdef (CONFIG_ARCH1) #else (CONFIG_ARCH2)
(see drivers/input/evdev.c for example). Arch maintainers are free
to override the default __is_compat_task() macro in their asm/compat.h
headers to be correct for their arch. This provides generic code with a
way to get what it wants, without needing to know ugly per-arch
details.

[Not only does this nicely clean up generic code, it's also useful
 to clean up some ugliness in the per-arch signal, ptrace, etc., code.]

See http://lkml.org/lkml/2006/1/8/170 for an example of generic code
that can be nicely cleaned up this way.

Based on prior work by Carlos O'Donell <carlos@parisc-linux.org>

Signed-off-by: Carlos O'Donell <carlos@parisc-linux.org>
Signed-off-by: Kyle McMartin <kyle@parisc-linux.org>

diff --git a/include/linux/compat.h b/include/linux/compat.h
index f9ca534..1645200 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -6,10 +6,16 @@
  */
 #include <linux/config.h>
 
-#ifdef CONFIG_COMPAT
+#ifndef CONFIG_COMPAT
+
+/* No tasks needing compat if !CONFIG_COMPAT */
+#define __is_compat_task(x)	0
+
+#else
 
 #include <linux/stat.h>
 #include <linux/param.h>	/* for HZ */
+#include <linux/personality.h>	/* Conditional process compat */
 #include <linux/sem.h>
 
 #include <asm/compat.h>
@@ -18,6 +24,11 @@
 #define compat_jiffies_to_clock_t(x)	\
 		(((unsigned long)(x) * COMPAT_USER_HZ) / HZ)
 
+/* Arches may override __is_compat_task from asm/compat.h */
+#ifndef __is_compat_task
+#define __is_compat_task(x)	(personality(x->personality) == PER_LINUX32)
+#endif /* __is_compat_task */
+
 typedef __compat_uid32_t	compat_uid_t;
 typedef __compat_gid32_t	compat_gid_t;
 
@@ -162,4 +173,10 @@ int get_compat_sigevent(struct sigevent 
 		const struct compat_sigevent __user *u_event);
 
 #endif /* CONFIG_COMPAT */
+
+static inline int
+is_compat_task(struct task_struct *task) {
+	return __is_compat_task(task);
+}
+
 #endif /* _LINUX_COMPAT_H */

Re: [RFC] is_compat_task helper

[Andi Kleen]

On Tuesday 24 January 2006 01:07, Kyle McMartin wrote:

> +/* Arches may override __is_compat_task from asm/compat.h */
> +#ifndef __is_compat_task
> +#define __is_compat_task(x)	(personality(x->personality) == PER_LINUX32)

I don't think this particular patch is a good idea. PER_LINUX32 means
something completely different than you think on many architectures.
You can't do a default for it.

-Andi

Re: [RFC] is_compat_task helper

[Kyle McMartin]

On Tue, Jan 24, 2006 at 01:13:39AM +0100, Andi Kleen wrote:
> I don't think this particular patch is a good idea. PER_LINUX32 means
> something completely different than you think on many architectures.
> You can't do a default for it.
>

Hmm, I like the way x86-64 handles this. Perhaps it would be better
to not do anything by default at all. I thought about using thread_info
flags, but those are not nearly consistent.

Cheers,
	Kyle

Re: [RFC] is_compat_task helper

[David S. Miller]

From: Andi Kleen <ak@suse.de>
Date: Tue, 24 Jan 2006 01:13:39 +0100

> On Tuesday 24 January 2006 01:07, Kyle McMartin wrote:
> 
> > +/* Arches may override __is_compat_task from asm/compat.h */
> > +#ifndef __is_compat_task
> > +#define __is_compat_task(x)	(personality(x->personality) == PER_LINUX32)
> 
> I don't think this particular patch is a good idea. PER_LINUX32 means
> something completely different than you think on many architectures.
> You can't do a default for it.

Indeed, it is definitely preferable to just flat out break
the build than give a bogus default.

If the build breaks, you see the failure and the platform
maintainer makes sure the correct implementation is made.
If it silently just builds, you end up with potential silent
failures which just sucks :)

Re: [RFC] is_compat_task helper

[Kyle McMartin]

On Mon, Jan 23, 2006 at 09:07:55PM -0800, David S. Miller wrote:
> From: Andi Kleen <ak@suse.de>
> Date: Tue, 24 Jan 2006 01:13:39 +0100
> > On Tuesday 24 January 2006 01:07, Kyle McMartin wrote:
> > > +/* Arches may override __is_compat_task from asm/compat.h */
> > I don't think this particular patch is a good idea. PER_LINUX32 means
> > something completely different than you think on many architectures.
> > You can't do a default for it.
> 
> Indeed, it is definitely preferable to just flat out break
> the build than give a bogus default.
>

Sounds fine to me.
 
> If the build breaks, you see the failure and the platform
> maintainer makes sure the correct implementation is made.
> If it silently just builds, you end up with potential silent
> failures which just sucks :)

Alrighty, would there by any objection to ripping out the 
`generic' __is_compat_task and letting this sit in -mm for arch
maintainers to fix their builds?

Thanks,
	Kyle