[RFC 1/1] proc: constify seq_operations

[RFC 1/1] proc: constify seq_operations

[Fabian Frederick]

proc_uid_seq_operations, proc_gid_seq_operations and proc_projid_seq_operations
are only called in proc_id_map_open with seq_open as
const struct seq_operations so we can constify the 3 structures and update
proc_id_map_open prototype.

(
If it's correct, do I have to send separate patches or different subject ?

Thanks,
Fabian

)

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/proc/base.c                 | 2 +-
 include/linux/user_namespace.h | 6 +++---
 kernel/user_namespace.c        | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2d696b0..79df9ff 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2449,7 +2449,7 @@ static int proc_tgid_io_accounting(struct task_struct *task, char *buffer)
 
 #ifdef CONFIG_USER_NS
 static int proc_id_map_open(struct inode *inode, struct file *file,
-	struct seq_operations *seq_ops)
+	const struct seq_operations *seq_ops)
 {
 	struct user_namespace *ns = NULL;
 	struct task_struct *task;
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 4836ba3..e953726 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -57,9 +57,9 @@ static inline void put_user_ns(struct user_namespace *ns)
 }
 
 struct seq_operations;
-extern struct seq_operations proc_uid_seq_operations;
-extern struct seq_operations proc_gid_seq_operations;
-extern struct seq_operations proc_projid_seq_operations;
+extern const struct seq_operations proc_uid_seq_operations;
+extern const struct seq_operations proc_gid_seq_operations;
+extern const struct seq_operations proc_projid_seq_operations;
 extern ssize_t proc_uid_map_write(struct file *, const char __user *, size_t, loff_t *);
 extern ssize_t proc_gid_map_write(struct file *, const char __user *, size_t, loff_t *);
 extern ssize_t proc_projid_map_write(struct file *, const char __user *, size_t, loff_t *);
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index fcc0256..aa312b0 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -526,21 +526,21 @@ static void m_stop(struct seq_file *seq, void *v)
 	return;
 }
 
-struct seq_operations proc_uid_seq_operations = {
+const struct seq_operations proc_uid_seq_operations = {
 	.start = uid_m_start,
 	.stop = m_stop,
 	.next = m_next,
 	.show = uid_m_show,
 };
 
-struct seq_operations proc_gid_seq_operations = {
+const struct seq_operations proc_gid_seq_operations = {
 	.start = gid_m_start,
 	.stop = m_stop,
 	.next = m_next,
 	.show = gid_m_show,
 };
 
-struct seq_operations proc_projid_seq_operations = {
+const struct seq_operations proc_projid_seq_operations = {
 	.start = projid_m_start,
 	.stop = m_stop,
 	.next = m_next,
-- 
1.8.4.5

Re: [RFC 1/1] proc: constify seq_operations

[Andrew Morton]

On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <fabf@skynet.be> wrote:

> proc_uid_seq_operations, proc_gid_seq_operations and proc_projid_seq_operations
> are only called in proc_id_map_open with seq_open as
> const struct seq_operations so we can constify the 3 structures and update
> proc_id_map_open prototype.

There are an absolutely enormous number of places where we could
constify things.  For sheer sanity's sake I'm not inclined to churn the
code in this way unless a patch provides some sort of runtime benefit. 
And this particular patch doesn't appear to change the generated code
at all.

Re: [RFC 1/1] proc: constify seq_operations

[Joe Perches]

On Mon, 2014-06-30 at 13:39 -0700, Andrew Morton wrote:
> On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <fabf@skynet.be> wrote:
> 
> > proc_uid_seq_operations, proc_gid_seq_operations and proc_projid_seq_operations
> > are only called in proc_id_map_open with seq_open as
> > const struct seq_operations so we can constify the 3 structures and update
> > proc_id_map_open prototype.
> 
> There are an absolutely enormous number of places where we could
> constify things.

Which would be a good thing.

>   For sheer sanity's sake I'm not inclined to churn the
> code in this way unless a patch provides some sort of runtime benefit. 
> And this particular patch doesn't appear to change the generated code
> at all.

It moves ~100 bytes from data to text
(gcc 4.8)

$ size kernel/user_namespace.o*
   text	   data	    bss	    dec	    hex	filename
   6676	   3107	   2248	  12031	   2eff	kernel/user_namespace.o.new
   6580	   3211	   2248	  12039	   2f07	kernel/user_namespace.o.old

Re: [RFC 1/1] proc: constify seq_operations

[Andrew Morton]

On Mon, 30 Jun 2014 13:49:30 -0700 Joe Perches <joe@perches.com> wrote:

> On Mon, 2014-06-30 at 13:39 -0700, Andrew Morton wrote:
> > On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <fabf@skynet.be> wrote:
> > 
> > > proc_uid_seq_operations, proc_gid_seq_operations and proc_projid_seq_operations
> > > are only called in proc_id_map_open with seq_open as
> > > const struct seq_operations so we can constify the 3 structures and update
> > > proc_id_map_open prototype.
> > 
> > There are an absolutely enormous number of places where we could
> > constify things.
> 
> Which would be a good thing.

These things involve tradeoffs.  A constant dribble of
do-nothing-useful patches just isn't worth the effort on either end,
IMO.

> >   For sheer sanity's sake I'm not inclined to churn the
> > code in this way unless a patch provides some sort of runtime benefit. 
> > And this particular patch doesn't appear to change the generated code
> > at all.
> 
> It moves ~100 bytes from data to text

doh, I only looked at base.o.

I added this to the changelog.  It should have been there originally,
please.

   text    data     bss     dec     hex filename
   6817     404    1984    9205    23f5 kernel/user_namespace.o-before
   6913     308    1984    9205    23f5 kernel/user_namespace.o-after

Re: [RFC 1/1] proc: constify seq_operations

[Joe Perches]

On Mon, 2014-06-30 at 13:57 -0700, Andrew Morton wrote:
> On Mon, 30 Jun 2014 13:49:30 -0700 Joe Perches <joe@perches.com> wrote:

> > It moves ~100 bytes from data to text

> > $ size kernel/user_namespace.o*
> >    text    data     bss     dec     hex filename
> >    6676    3107    2248   12031    2eff kernel/user_namespace.o.new
> >    6580    3211    2248   12039    2f07 kernel/user_namespace.o.old

> doh, I only looked at base.o.
> 
> I added this to the changelog.  It should have been there originally,
> please.
> 
>    text    data     bss     dec     hex filename
>    6817     404    1984    9205    23f5 kernel/user_namespace.o-before
>    6913     308    1984    9205    23f5 kernel/user_namespace.o-after

The delta in our object sizes in curious.

x86-64 allyesconfig here.
What gcc version for you?

Re: [RFC 1/1] proc: constify seq_operations

[Andrew Morton]

On Mon, 30 Jun 2014 14:09:05 -0700 Joe Perches <joe@perches.com> wrote:

> On Mon, 2014-06-30 at 13:57 -0700, Andrew Morton wrote:
> > On Mon, 30 Jun 2014 13:49:30 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > > It moves ~100 bytes from data to text
> 
> > > $ size kernel/user_namespace.o*
> > >    text    data     bss     dec     hex filename
> > >    6676    3107    2248   12031    2eff kernel/user_namespace.o.new
> > >    6580    3211    2248   12039    2f07 kernel/user_namespace.o.old
> 
> > doh, I only looked at base.o.
> > 
> > I added this to the changelog.  It should have been there originally,
> > please.
> > 
> >    text    data     bss     dec     hex filename
> >    6817     404    1984    9205    23f5 kernel/user_namespace.o-before
> >    6913     308    1984    9205    23f5 kernel/user_namespace.o-after
> 
> The delta in our object sizes in curious.
> 
> x86-64 allyesconfig here.
> What gcc version for you?

gcc-4.4.4 allmodconfig, minus CONFIG_DEBUG_INFO 

Re: [RFC 1/1] proc: constify seq_operations

[Christoph Hellwig]

On Mon, Jun 30, 2014 at 01:39:39PM -0700, Andrew Morton wrote:
> On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <fabf@skynet.be> wrote:
> 
> > proc_uid_seq_operations, proc_gid_seq_operations and proc_projid_seq_operations
> > are only called in proc_id_map_open with seq_open as
> > const struct seq_operations so we can constify the 3 structures and update
> > proc_id_map_open prototype.
> 
> There are an absolutely enormous number of places where we could
> constify things.  For sheer sanity's sake I'm not inclined to churn the
> code in this way unless a patch provides some sort of runtime benefit. 
> And this particular patch doesn't appear to change the generated code
> at all.

Unlike a lot of the cleanup patches which provide no benefit at all
constifying op vectors moves them from .text to .data which is not
marked executable and thus reduce the attack vector for kernel exploits.

So I defintively like to see these much more than a lot of the other
things filling up the lists.

Re: [RFC 1/1] proc: constify seq_operations

[Richard Weinberger]

On Tue, Jul 1, 2014 at 10:17 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jun 30, 2014 at 01:39:39PM -0700, Andrew Morton wrote:
>> On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <fabf@skynet.be> wrote:
>>
>> > proc_uid_seq_operations, proc_gid_seq_operations and proc_projid_seq_operations
>> > are only called in proc_id_map_open with seq_open as
>> > const struct seq_operations so we can constify the 3 structures and update
>> > proc_id_map_open prototype.
>>
>> There are an absolutely enormous number of places where we could
>> constify things.  For sheer sanity's sake I'm not inclined to churn the
>> code in this way unless a patch provides some sort of runtime benefit.
>> And this particular patch doesn't appear to change the generated code
>> at all.
>
> Unlike a lot of the cleanup patches which provide no benefit at all
> constifying op vectors moves them from .text to .data which is not
> marked executable and thus reduce the attack vector for kernel exploits.
>
> So I defintively like to see these much more than a lot of the other
> things filling up the lists.

BTW: Daniel Walter and I are currently working with grsec's constify gcc plugin.
This plugin automatically makes structs const which contain only
function pointers.
Our goal is to bring it mainline. We cannot enable it by default as
many gcc toolchains
have plugin support disabled. But at least as tool in tools/ it would
be handy to create
constify patches automatically...

-- 
Thanks,
//richard

Re: [RFC 1/1] proc: constify seq_operations

[Joe Perches]

On Tue, 2014-07-01 at 10:41 +0200, Richard Weinberger wrote:
> BTW: Daniel Walter and I are currently working with grsec's constify gcc plugin.
> This plugin automatically makes structs const which contain only
> function pointers.

http://pax.grsecurity.net/docs/PaXTeam-H2HC13-PaX-gcc-plugins.pdf

Good luck, it might help for a few things.

Re: [RFC 1/1] proc: constify seq_operations

[Fabian Frederick]

> On 01 July 2014 at 10:17 Christoph Hellwig <hch@infradead.org> wrote:
>
>
> On Mon, Jun 30, 2014 at 01:39:39PM -0700, Andrew Morton wrote:
> > On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <fabf@skynet.be> wrote:
> >
> > > proc_uid_seq_operations, proc_gid_seq_operations and
> > > proc_projid_seq_operations
> > > are only called in proc_id_map_open with seq_open as
> > > const struct seq_operations so we can constify the 3 structures and update
> > > proc_id_map_open prototype.
> >
> > There are an absolutely enormous number of places where we could
> > constify things.  For sheer sanity's sake I'm not inclined to churn the
> > code in this way unless a patch provides some sort of runtime benefit.
> > And this particular patch doesn't appear to change the generated code
> > at all.
>
> Unlike a lot of the cleanup patches which provide no benefit at all
> constifying op vectors moves them from .text to .data which is not
> marked executable and thus reduce the attack vector for kernel exploits.

Sorry Christoph but earlier reports by Andrew and Joe show the opposite ie
it moves vectors from data to text (code/executable) segment...

Fabian

>
> So I defintively like to see these much more than a lot of the other
> things filling up the lists.