2007-08-07 20:16:01

by Olaf Hering

[permalink] [raw]
Subject: [PATCH] remove binfmts.h from header exports


remove linux/binfmts.h from make headers_install

A recent patch added PAGE_SIZE to the part outside of __KERNEL__.
qemu and ia32el have their own define of MAX_ARG_PAGES.
No package uses linux/binfmts.h, so it is safe to not provide it.

Signed-off-by: Olaf Hering <[email protected]>

---
include/linux/Kbuild | 1 -
include/linux/binfmts.h | 2 --
2 files changed, 3 deletions(-)

--- a/include/linux/Kbuild
+++ b/include/linux/Kbuild
@@ -173,7 +173,6 @@ unifdef-y += atm.h
unifdef-y += atm_tcp.h
unifdef-y += audit.h
unifdef-y += auto_fs.h
-unifdef-y += binfmts.h
unifdef-y += capability.h
unifdef-y += capi.h
unifdef-y += cciss_ioctl.h
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -17,7 +17,6 @@ struct pt_regs;
/* sizeof(linux_binprm->buf) */
#define BINPRM_BUF_SIZE 128

-#ifdef __KERNEL__

#define CORENAME_MAX_SIZE 128

@@ -99,5 +98,4 @@ extern void compute_creds(struct linux_b
extern int do_coredump(long signr, int exit_code, struct pt_regs * regs);
extern int set_binfmt(struct linux_binfmt *new);

-#endif /* __KERNEL__ */
#endif /* _LINUX_BINFMTS_H */


2007-08-07 20:37:27

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: [PATCH] remove binfmts.h from header exports

On Tue, Aug 07, 2007 at 10:16:03PM +0200, Olaf Hering wrote:
> remove linux/binfmts.h from make headers_install
>
> A recent patch added PAGE_SIZE to the part outside of __KERNEL__.
> qemu and ia32el have their own define of MAX_ARG_PAGES.

Should they use kernel header instead?

> No package uses linux/binfmts.h, so it is safe to not provide it.

And? Does it contain stuff which is userspace visible?
binfmts.h has at least CORENAME_MAX_SIZE and SUID_DUMP_*

"userspace doesn't use header" is something headers_install has never
been about.

> --- a/include/linux/Kbuild
> +++ b/include/linux/Kbuild
> @@ -173,7 +173,6 @@ unifdef-y += atm.h
> unifdef-y += atm_tcp.h
> unifdef-y += audit.h
> unifdef-y += auto_fs.h
> -unifdef-y += binfmts.h
> unifdef-y += capability.h
> unifdef-y += capi.h
> unifdef-y += cciss_ioctl.h
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -17,7 +17,6 @@ struct pt_regs;
> /* sizeof(linux_binprm->buf) */
> #define BINPRM_BUF_SIZE 128
>
> -#ifdef __KERNEL__
>
> #define CORENAME_MAX_SIZE 128
>
> @@ -99,5 +98,4 @@ extern void compute_creds(struct linux_b
> extern int do_coredump(long signr, int exit_code, struct pt_regs * regs);
> extern int set_binfmt(struct linux_binfmt *new);
>
> -#endif /* __KERNEL__ */
> #endif /* _LINUX_BINFMTS_H */

2007-08-07 23:38:39

by Rene Herman

[permalink] [raw]
Subject: Re: [PATCH] remove binfmts.h from header exports

On 08/07/2007 10:16 PM, Olaf Hering wrote:

> remove linux/binfmts.h from make headers_install
>
> A recent patch added PAGE_SIZE to the part outside of __KERNEL__.
> qemu and ia32el have their own define of MAX_ARG_PAGES.
> No package uses linux/binfmts.h, so it is safe to not provide it.

How is "all packages" defined?

Rene.

2007-08-08 06:10:30

by Olaf Hering

[permalink] [raw]
Subject: Re: [PATCH] remove binfmts.h from header exports

On Wed, Aug 08, Rene Herman wrote:

> >No package uses linux/binfmts.h, so it is safe to not provide it.
>
> How is "all packages" defined?

3052 openSuSE 10.3 packages.

2007-08-08 06:14:32

by Olaf Hering

[permalink] [raw]
Subject: Re: [PATCH] remove binfmts.h from header exports

On Wed, Aug 08, Alexey Dobriyan wrote:

> On Tue, Aug 07, 2007 at 10:16:03PM +0200, Olaf Hering wrote:
> > remove linux/binfmts.h from make headers_install
> >
> > A recent patch added PAGE_SIZE to the part outside of __KERNEL__.
> > qemu and ia32el have their own define of MAX_ARG_PAGES.
>
> Should they use kernel header instead?

No, because the header will disappear. And from my understanding, the
args limit is now gone.

> > No package uses linux/binfmts.h, so it is safe to not provide it.
>
> And? Does it contain stuff which is userspace visible?
> binfmts.h has at least CORENAME_MAX_SIZE and SUID_DUMP_*

Yes, its inside __KERNEL__. Have you read that header already? We are
talking about the part below.
What part is useable for an application?

....
#include <linux/capability.h>

struct pt_regs;

/*
* These are the maximum length and maximum number of strings passed to the
* execve() system call. MAX_ARG_STRLEN is essentially random but serves to
* prevent the kernel from being unduly impacted by misaddressed pointers.
* MAX_ARG_STRINGS is chosen to fit in a signed 32-bit integer.
*/
#define MAX_ARG_STRLEN (PAGE_SIZE * 32)
#define MAX_ARG_STRINGS 0x7FFFFFFF

/* sizeof(linux_binprm->buf) */
#define BINPRM_BUF_SIZE 128
....

> "userspace doesn't use header" is something headers_install has never
> been about.

Thats true.
I remember someone even wrote something for Documentation/ a few days ago.

2007-08-08 07:18:40

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: [PATCH] remove binfmts.h from header exports

On 8/8/07, Olaf Hering <[email protected]> wrote:
> On Wed, Aug 08, Alexey Dobriyan wrote:
>
> > On Tue, Aug 07, 2007 at 10:16:03PM +0200, Olaf Hering wrote:
> > > remove linux/binfmts.h from make headers_install
> > >
> > > A recent patch added PAGE_SIZE to the part outside of __KERNEL__.
> > > qemu and ia32el have their own define of MAX_ARG_PAGES.
> >
> > Should they use kernel header instead?
>
> No, because the header will disappear. And from my understanding, the
> args limit is now gone.

OK.

> > > No package uses linux/binfmts.h, so it is safe to not provide it.
> >
> > And? Does it contain stuff which is userspace visible?
> > binfmts.h has at least CORENAME_MAX_SIZE and SUID_DUMP_*
>
> Yes, its inside __KERNEL__.

Can't you accept for a second that some stuff under __KERNEL__
was put there by mistake?

> We are talking about the part below.

No, we are talking about whole header since you're going
to unexport whole header.

> What part is useable for an application?

>From part you quoted nothing. Otherwise:

fd = open("/proc/sys/fs/suid_dumpable", 1);
snprintf(buf, sizeof(buf), "%d", SUID_DUMP_ROOT);
write(fd, buf, strlen(buf));

> #include <linux/capability.h>
>
> struct pt_regs;
>
> /*
> * These are the maximum length and maximum number of strings passed to the
> * execve() system call. MAX_ARG_STRLEN is essentially random but serves to
> * prevent the kernel from being unduly impacted by misaddressed pointers.
> * MAX_ARG_STRINGS is chosen to fit in a signed 32-bit integer.
> */
> #define MAX_ARG_STRLEN (PAGE_SIZE * 32)
> #define MAX_ARG_STRINGS 0x7FFFFFFF
>
> /* sizeof(linux_binprm->buf) */
> #define BINPRM_BUF_SIZE 128

2007-08-08 17:28:21

by Olaf Hering

[permalink] [raw]
Subject: Re: [PATCH] remove binfmts.h from header exports

On Wed, Aug 08, Alexey Dobriyan wrote:

> > > And? Does it contain stuff which is userspace visible?
> > > binfmts.h has at least CORENAME_MAX_SIZE and SUID_DUMP_*
> >
> > Yes, its inside __KERNEL__.
>
> Can't you accept for a second that some stuff under __KERNEL__
> was put there by mistake?

I wonder why the defines exist at all? My grep doesnt find SUID_DUMP_*.
Looking at the commit message from d6e711448137ca3301512cec41a2c2ce852b3d0a

...
> > if (current->euid == current->uid && current->egid == current->gid)
> > current->mm->dumpable = 1;
>
> Should this be SUID_DUMP_USER?

Actually the feedback I had from last time was that the SUID_ defines
should go because its clearer to follow the numbers. They can go
everywhere (and there are lots of places where dumpable is tested/used
as a bool in untouched code)
...

Exporting a random array size is not very useful. The app and the kernel
has to check the string length anyway.

So lets remove the header and the 3 unused defines.