[PATCH] alpha: binfmt_aout fix

[PATCH] alpha: binfmt_aout fix

[Ivan Kokshaysky]

This fixes the problem introduced by commit 3bfacef412 (get rid of
special-casing the /sbin/loader on alpha): osf/1 ecoff binary
segfaults when binfmt_aout built as module. That happens because
aout binary handler gets on the top of the binfmt list due to late
registration, and kernel attempts to execute the binary without
preparatory work that must be done by binfmt_loader.

Fixed by changing the registration order of the default
binfmt handlers using list_add_tail() and introducing insert_binfmt()
function which places new handler on the top of the binfmt list.
This might be generally useful for installing arch-specific frontends
for default handlers or just for overriding them.

Signed-off-by: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
---
 arch/alpha/kernel/Makefile        |    6 +++++-
 arch/alpha/kernel/binfmt_loader.c |    2 +-
 fs/exec.c                         |    7 ++++---
 include/linux/binfmts.h           |   14 +++++++++++++-
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/arch/alpha/kernel/Makefile b/arch/alpha/kernel/Makefile
index a427538..7739a62 100644
--- a/arch/alpha/kernel/Makefile
+++ b/arch/alpha/kernel/Makefile
@@ -8,7 +8,7 @@ EXTRA_CFLAGS	:= -Werror -Wno-sign-compare
 
 obj-y    := entry.o traps.o process.o init_task.o osf_sys.o irq.o \
 	    irq_alpha.o signal.o setup.o ptrace.o time.o \
-	    alpha_ksyms.o systbls.o err_common.o io.o binfmt_loader.o
+	    alpha_ksyms.o systbls.o err_common.o io.o
 
 obj-$(CONFIG_VGA_HOSE)	+= console.o
 obj-$(CONFIG_SMP)	+= smp.o
@@ -43,6 +43,10 @@ else
 # Misc support
 obj-$(CONFIG_ALPHA_SRM)		+= srmcons.o
 
+ifdef CONFIG_BINFMT_AOUT
+obj-y	+= binfmt_loader.o
+endif
+
 # Core logic support
 obj-$(CONFIG_ALPHA_APECS)	+= core_apecs.o
 obj-$(CONFIG_ALPHA_CIA)		+= core_cia.o
diff --git a/arch/alpha/kernel/binfmt_loader.c b/arch/alpha/kernel/binfmt_loader.c
index 4a0af90..3fcfad4 100644
--- a/arch/alpha/kernel/binfmt_loader.c
+++ b/arch/alpha/kernel/binfmt_loader.c
@@ -46,6 +46,6 @@ static struct linux_binfmt loader_format = {
 
 static int __init init_loader_binfmt(void)
 {
-	return register_binfmt(&loader_format);
+	return insert_binfmt(&loader_format);
 }
 arch_initcall(init_loader_binfmt);
diff --git a/fs/exec.c b/fs/exec.c
index 052a961..974098d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -69,17 +69,18 @@ int suid_dumpable = 0;
 static LIST_HEAD(formats);
 static DEFINE_RWLOCK(binfmt_lock);
 
-int register_binfmt(struct linux_binfmt * fmt)
+int __register_binfmt(struct linux_binfmt * fmt, int insert)
 {
 	if (!fmt)
 		return -EINVAL;
 	write_lock(&binfmt_lock);
-	list_add(&fmt->lh, &formats);
+	insert ? list_add(&fmt->lh, &formats) :
+		 list_add_tail(&fmt->lh, &formats);
 	write_unlock(&binfmt_lock);
 	return 0;	
 }
 
-EXPORT_SYMBOL(register_binfmt);
+EXPORT_SYMBOL(__register_binfmt);
 
 void unregister_binfmt(struct linux_binfmt * fmt)
 {
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 6638b81..61ee18c 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -82,7 +82,19 @@ struct linux_binfmt {
 	int hasvdso;
 };
 
-extern int register_binfmt(struct linux_binfmt *);
+extern int __register_binfmt(struct linux_binfmt *fmt, int insert);
+
+/* Registration of default binfmt handlers */
+static inline int register_binfmt(struct linux_binfmt *fmt)
+{
+	return __register_binfmt(fmt, 0);
+}
+/* Same as above, but adds a new binfmt at the top of the list */
+static inline int insert_binfmt(struct linux_binfmt *fmt)
+{
+	return __register_binfmt(fmt, 1);
+}
+
 extern void unregister_binfmt(struct linux_binfmt *);
 
 extern int prepare_binprm(struct linux_binprm *);

Re: [PATCH] alpha: binfmt_aout fix

[Al Viro]

On Fri, Apr 24, 2009 at 06:47:38PM +0400, Ivan Kokshaysky wrote:
> This fixes the problem introduced by commit 3bfacef412 (get rid of
> special-casing the /sbin/loader on alpha): osf/1 ecoff binary
> segfaults when binfmt_aout built as module. That happens because
> aout binary handler gets on the top of the binfmt list due to late
> registration, and kernel attempts to execute the binary without
> preparatory work that must be done by binfmt_loader.
> 
> Fixed by changing the registration order of the default
> binfmt handlers using list_add_tail() and introducing insert_binfmt()
> function which places new handler on the top of the binfmt list.
> This might be generally useful for installing arch-specific frontends
> for default handlers or just for overriding them.

Hmm...  How about always doing list_add_tail() instead?

Re: [PATCH] alpha: binfmt_aout fix

[Ivan Kokshaysky]

On Fri, Apr 24, 2009 at 04:05:49PM +0100, Al Viro wrote:
> On Fri, Apr 24, 2009 at 06:47:38PM +0400, Ivan Kokshaysky wrote:
> > Fixed by changing the registration order of the default
> > binfmt handlers using list_add_tail() and introducing insert_binfmt()
> > function which places new handler on the top of the binfmt list.
> > This might be generally useful for installing arch-specific frontends
> > for default handlers or just for overriding them.
> 
> Hmm...  How about always doing list_add_tail() instead?

That would fix the module case, but gets broken when binfmt_aout
in built-in, because the generic handlers are registered pretty
early (core_initcall) and alpha binfmt_loader is an arch_initcall.

Ivan.

Re: [PATCH] alpha: binfmt_aout fix

[Al Viro]

On Fri, Apr 24, 2009 at 07:46:02PM +0400, Ivan Kokshaysky wrote:
> On Fri, Apr 24, 2009 at 04:05:49PM +0100, Al Viro wrote:
> > On Fri, Apr 24, 2009 at 06:47:38PM +0400, Ivan Kokshaysky wrote:
> > > Fixed by changing the registration order of the default
> > > binfmt handlers using list_add_tail() and introducing insert_binfmt()
> > > function which places new handler on the top of the binfmt list.
> > > This might be generally useful for installing arch-specific frontends
> > > for default handlers or just for overriding them.
> > 
> > Hmm...  How about always doing list_add_tail() instead?
> 
> That would fix the module case, but gets broken when binfmt_aout
> in built-in, because the generic handlers are registered pretty
> early (core_initcall) and alpha binfmt_loader is an arch_initcall.

OK, probably the easiest way to deal with that.

ACK

[PATCH] alpha: binfmt_aout fix

[Ivan Kokshaysky]

This fixes the problem introduced by commit 3bfacef412 (get rid of
special-casing the /sbin/loader on alpha): osf/1 ecoff binary
segfaults when binfmt_aout built as module. That happens because
aout binary handler gets on the top of the binfmt list due to late
registration, and kernel attempts to execute the binary without
preparation work that must be done by binfmt_loader.

So I had to add a check for bprm->loader in load_aout_binary()
which makes a search order irrelevant.

Also, do not build the binfmt_loader when CONFIG_BINFMT_AOUT
is not set.

Signed-off-by: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
---
 arch/alpha/kernel/Makefile |    6 +++++-
 fs/binfmt_aout.c           |    6 ++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/arch/alpha/kernel/Makefile b/arch/alpha/kernel/Makefile
index b469775..e203713 100644
--- a/arch/alpha/kernel/Makefile
+++ b/arch/alpha/kernel/Makefile
@@ -8,7 +8,7 @@ EXTRA_CFLAGS	:= -Werror -Wno-sign-compare
 
 obj-y    := entry.o traps.o process.o init_task.o osf_sys.o irq.o \
 	    irq_alpha.o signal.o setup.o ptrace.o time.o \
-	    alpha_ksyms.o systbls.o err_common.o io.o binfmt_loader.o
+	    alpha_ksyms.o systbls.o err_common.o io.o
 
 obj-$(CONFIG_VGA_HOSE)	+= console.o
 obj-$(CONFIG_SMP)	+= smp.o
@@ -43,6 +43,10 @@ else
 # Misc support
 obj-$(CONFIG_ALPHA_SRM)		+= srmcons.o
 
+ifdef CONFIG_BINFMT_AOUT
+obj-y	+= binfmt_loader.o
+endif
+
 # Core logic support
 obj-$(CONFIG_ALPHA_APECS)	+= core_apecs.o
 obj-$(CONFIG_ALPHA_CIA)		+= core_cia.o
diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index b639dcf..c890e5f 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -226,6 +226,12 @@ static int load_aout_binary(struct linux_binprm * bprm, struct pt_regs * regs)
 	int retval;
 
 	ex = *((struct exec *) bprm->buf);		/* exec-header */
+#ifdef __alpha__
+	/* Check whether the OSF/1 loader is required and present. */
+	if (ex.fh.f_magic == 0x183 && (ex.fh.f_flags & 0x3000) == 0x3000 &&
+	    !bprm->loader)
+		return -ENOEXEC;
+#endif
 	if ((N_MAGIC(ex) != ZMAGIC && N_MAGIC(ex) != OMAGIC &&
 	     N_MAGIC(ex) != QMAGIC && N_MAGIC(ex) != NMAGIC) ||
 	    N_TRSIZE(ex) || N_DRSIZE(ex) ||

Re: [PATCH] alpha: binfmt_aout fix

[Christoph Hellwig]

On Tue, Feb 17, 2009 at 01:49:56PM +0300, Ivan Kokshaysky wrote:
> This fixes the problem introduced by commit 3bfacef412 (get rid of
> special-casing the /sbin/loader on alpha): osf/1 ecoff binary
> segfaults when binfmt_aout built as module. That happens because
> aout binary handler gets on the top of the binfmt list due to late
> registration, and kernel attempts to execute the binary without
> preparation work that must be done by binfmt_loader.

I think the proper fix for this is to make sure newly registered
binfmts are added to the tail of the list, not the front.

Re: [PATCH] alpha: binfmt_aout fix

[Ivan Kokshaysky]

On Tue, Feb 17, 2009 at 06:08:42AM -0500, Christoph Hellwig wrote:
> I think the proper fix for this is to make sure newly registered
> binfmts are added to the tail of the list, not the front.

Yes, I was thinking about that. But then I need to make sure
that alpha binfmt_loader is registered first, which is not
possible at the moment as binfmts get registered at core_initcall
level.
If it's acceptable to change binfmts initcall level to, say,
fs_initcall, then changing list_add() to list_add_tail() would
certainly work fine here.

Ivan.

Re: [PATCH] alpha: binfmt_aout fix

[Andrew Morton]

On Tue, 17 Feb 2009 14:40:56 +0300
Ivan Kokshaysky <ink@jurassic.park.msu.ru> wrote:

> On Tue, Feb 17, 2009 at 06:08:42AM -0500, Christoph Hellwig wrote:
> > I think the proper fix for this is to make sure newly registered
> > binfmts are added to the tail of the list, not the front.
> 
> Yes, I was thinking about that. But then I need to make sure
> that alpha binfmt_loader is registered first, which is not
> possible at the moment as binfmts get registered at core_initcall
> level.
> If it's acceptable to change binfmts initcall level to, say,
> fs_initcall, then changing list_add() to list_add_tail() would
> certainly work fine here.
> 

It would make sense to register the core, default handlers first and
to permit arch-specific handlers to then override (or to front-end) the
core handlers.

But that sounds a bit risky for 2.6.29.  Is this fix needed in 2.6.29? 
If so, perhaps we should merge version 1 as a temporary thing.  But if
so, please let's not do the temporary-thing-which-hangs-around-forever thing?

Re: [PATCH] alpha: binfmt_aout fix

[Ivan Kokshaysky]

On Wed, Feb 18, 2009 at 03:52:31PM -0800, Andrew Morton wrote:
> It would make sense to register the core, default handlers first and
> to permit arch-specific handlers to then override (or to front-end) the
> core handlers.

Yes, sounds good. I'll make a patch for 2.6.30.

> But that sounds a bit risky for 2.6.29.  Is this fix needed in 2.6.29? 
> If so, perhaps we should merge version 1 as a temporary thing.  But if
> so, please let's not do the temporary-thing-which-hangs-around-forever thing?

Just drop it. The bug is not so terribly critical (I don't think that a lot
of people are using osf binaries these days).

Ivan.