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 <[email protected]>
---
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
On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <[email protected]> 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.
On Mon, 2014-06-30 at 13:39 -0700, Andrew Morton wrote:
> On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <[email protected]> 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
On Mon, 30 Jun 2014 13:49:30 -0700 Joe Perches <[email protected]> wrote:
> On Mon, 2014-06-30 at 13:39 -0700, Andrew Morton wrote:
> > On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <[email protected]> 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
On Mon, 2014-06-30 at 13:57 -0700, Andrew Morton wrote:
> On Mon, 30 Jun 2014 13:49:30 -0700 Joe Perches <[email protected]> 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?
On Mon, 30 Jun 2014 14:09:05 -0700 Joe Perches <[email protected]> wrote:
> On Mon, 2014-06-30 at 13:57 -0700, Andrew Morton wrote:
> > On Mon, 30 Jun 2014 13:49:30 -0700 Joe Perches <[email protected]> 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
On Mon, Jun 30, 2014 at 01:39:39PM -0700, Andrew Morton wrote:
> On Mon, 30 Jun 2014 21:03:17 +0200 Fabian Frederick <[email protected]> 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.
On Tue, Jul 1, 2014 at 10:17 AM, Christoph Hellwig <[email protected]> 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 <[email protected]> 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
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.
> On 01 July 2014 at 10:17 Christoph Hellwig <[email protected]> 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 <[email protected]> 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.