Re: compat-signal-noarch-2004-01-29.patch

Re: compat-signal-noarch-2004-01-29.patch

[Carlos O'Donell]

On Thu, Feb 26, 2004 at 08:50:58PM +0100, Andi Kleen wrote:
> > I do not believe these changes will cause kernel maintainers 
> > to loose their good sense of design. Sprinkling is_compat_task
> > in every nook and cranny is not the goal of these changes. 
> 
> It may not be the goal, but it's opening the floodgates for it.
>
> Is it really that hard to keep the 32/64bit check in architecture
> specific code? It's only a few lines of code anyways.

The same argument can be used in favour of the changes. Your assumption
of causation between these changes and future changes wrt ABI design 
issues is IMO without merit. The patch fixes present problems, with
minimal impact, a clean interface, and allows this code to be leveraged 
by a number of arches.

Do you have a draft for an alternate x86_64 ABI that I can read?
Are you already working on an alternate ABI implementation?

Lets move forward. What are x86_64's needs wrt compat?

c.

Re: compat-signal-noarch-2004-01-29.patch

[Carlos O'Donell]

> If you really don't want to do that please provide a way that I can
> define away the compat task macro for x86-64 at least (although that
> would be a bad idea, just not having it would be better)

Already done, it's part of the change.

In include/linux/compat.h we include include/asm/compat.h first.
Thus, define HAVE_ARCH_IS_COMPAT_TASK and your own version of
is_compat_task.
 
Example untested patch for x86_64 provided at the end.

> No, it doesn't exist yet. I just don't want to close the door for such
> future enhancements. There is a patch for IA64 that does this though.
> I really don't want this generic macro thing for x86-64. 

That's your decision to make Andi, and I've infact taken that into
account by allowing arches to override the default behaviour of
is_compat_task() *and* all the associated compat functions called by
that check (e.g. compat_copy_siginfo_to_user and HAVE_ARCH_*).

I agree wholeheartedly. I don't want to remove choice, but I do want to
make maintenance easier, and reduce duplication of effort.

> They are pretty standard, except for the odd long long is different
> from long long in 32bit requirement (which many people who try to
> design compat compatible interfaces get wrong unfortunately because
> it's a non issue on the RISCs) 

AFAIK 'long long' has to be a _minimum_ of 64-bits, but may infact be
larger. Implementation details always have to be worked out by hand :o)

The patch below will effectively remove is_compat_task() for x86_64.
Apply it if you wish. You can do a number of nasty things with the
definition of is_compat_task() in order to catch improper uses.

c.

--- a/include/asm-x86_64/compat.h	28 Feb 2004 01:51:30 -0000	1.2
+++ b/include/asm-x86_64/compat.h	28 Feb 2004 19:02:06 -0000
@@ -9,6 +9,9 @@
 
 #define COMPAT_USER_HZ	100
 
+#define HAVE_ARCH_IS_COMPAT_TASK
+#define is_compat_task(x) 0
+
 typedef u32		compat_size_t;
 typedef s32		compat_ssize_t;
 typedef s32		compat_time_t;

Re: compat-signal-noarch-2004-01-29.patch

[Andi Kleen]

On Sat, 28 Feb 2004 14:15:37 -0500
Carlos O'Donell <carlos@baldric.uwo.ca> wrote:


> I agree wholeheartedly. I don't want to remove choice, but I do want to
> make maintenance easier, and reduce duplication of effort.

With adding is_compat_task() you're making long term mainteance harder.
 
> > They are pretty standard, except for the odd long long is different
> > from long long in 32bit requirement (which many people who try to
> > design compat compatible interfaces get wrong unfortunately because
> > it's a non issue on the RISCs) 
> 
> AFAIK 'long long' has to be a _minimum_ of 64-bits, but may infact be
> larger. Implementation details always have to be worked out by hand :o)

You misundertood, but never mind.
 
> The patch below will effectively remove is_compat_task() for x86_64.
> Apply it if you wish. You can do a number of nasty things with the
> definition of is_compat_task() in order to catch improper uses.

I must admit I am still not very happy about this.

It would be better to not have it in the first place, because it's a bogus
interface. In the current form people will start to use it for all kinds
of broken uses and it will be eventually impossible to get rid of it.

Is it really that hard to do the switch in architecture specific code?
I really fail to see where the problem is for you with this.

-Andi

Re: compat-signal-noarch-2004-01-29.patch

[Carlos O'Donell]

> Is it really that hard to do the switch in architecture specific code?

Yes, because I loathe the code duplication I'm seeing in all the arch
directories. Please don't tell me that it's more maintainable that way.
IMO, and I am not granted a crystal ball, the situation could be made
cleaner through:

a. Creation of arch/generic, assigning a maintainer to that arch.
b. Move kernel/signal.c(copy_siginfo_to_user) to arch/generic/signal.c
c. Have all interested arches build with arch/generic/signal.o
d. Add compat code into arch/generic for people with simple ABI's 
   to include.

The fundamental issue is that kernel signal code wants to write to the
stack, and that's a no-no. A generic copy_siginfo_to_user function was
written, and it was good enough for almost all the arches. Now we need
compat for that generic function. Who's responsible? Me?

Please comment. 

Andi, I appreciate your input, I'm trying to wrap my head around your
worries, but my inexperience limits my understanding. Perhaps your
glasses are more jaded than mine?

c.

Re: compat-signal-noarch-2004-01-29.patch

[Andi Kleen]

On Sat, 28 Feb 2004 15:28:33 -0500
Carlos O'Donell <carlos@baldric.uwo.ca> wrote:

> > Is it really that hard to do the switch in architecture specific code?
> 
> Yes, because I loathe the code duplication I'm seeing in all the arch
> directories. Please don't tell me that it's more maintainable that way.

It's about 5 lines of code. Not really a big issue. The bulk will be 
shared.

> IMO, and I am not granted a crystal ball, the situation could be made
> cleaner through:

[...]

Not sure what this has to do with the issue at hand of adding an is_compat_task()
or not. I have no great opinion on fs/compat et.al vs arch/generic, filenames
really don't concern me.

-Andi

Re: compat-signal-noarch-2004-01-29.patch

[Andi Kleen]

On Fri, 27 Feb 2004 18:40:37 -0500
Carlos O'Donell <carlos@baldric.uwo.ca> wrote:

> On Thu, Feb 26, 2004 at 08:50:58PM +0100, Andi Kleen wrote:
> > > I do not believe these changes will cause kernel maintainers 
> > > to loose their good sense of design. Sprinkling is_compat_task
> > > in every nook and cranny is not the goal of these changes. 
> > 
> > It may not be the goal, but it's opening the floodgates for it.
> >
> > Is it really that hard to keep the 32/64bit check in architecture
> > specific code? It's only a few lines of code anyways.
> 
> The same argument can be used in favour of the changes. Your assumption
> of causation between these changes and future changes wrt ABI design 
> issues is IMO without merit. The patch fixes present problems, with
> minimal impact, a clean interface, and allows this code to be leveraged 
> by a number of arches.

Most of the code is fine, all I request is that you keep the 
part that switches between 32bit and 64bit signals in architecture
specific code.

These are a few lines of your code only.

If you really don't want to do that please provide a way that I can
define away the compat task macro for x86-64 at least (although that would
be a bad idea, just not having it would be better)

> Do you have a draft for an alternate x86_64 ABI that I can read?
> Are you already working on an alternate ABI implementation?

No, it doesn't exist yet. I just don't want to close the door
for such future enhancements. There is a patch for IA64 that does this 
though.

I really don't want this generic macro thing for x86-64. 

> Lets move forward. What are x86_64's needs wrt compat?

They are pretty standard, except for the odd long long is different from long long 
in 32bit requirement (which many people who try to design compat compatible
interfaces get wrong unfortunately because it's a non issue on the RISCs) 

-Andi