1 |
On 5 Jan 2012 at 17:21, Andrea Zuccherelli wrote: |
2 |
|
3 |
> > now if some code needs writable ops structure variables it has 3 options under |
4 |
> > the plugin approach: |
5 |
> > |
6 |
> > 1. add __no_const to the structure declaration |
7 |
> > |
8 |
> > 2. typedef a __no_const version of the constified structure type |
9 |
> > |
10 |
> > 3. use pax_open_kernel/pax_close_kernel to temporarily override |
11 |
> > the (runtime enforced) constness of a given variable |
12 |
> > |
13 |
> > each approach has its own conditions to use, here's a quick summary: |
14 |
> > |
15 |
> > 1. when all instances of the given structure type are runtime allocated |
16 |
> > (i.e., there're no static instances) |
17 |
> > |
18 |
> > 2. when some instances of the given structure type are statically allocated |
19 |
> > it's best to let them be consitified by the plugin and use the typedef'd |
20 |
> > __no_const type for dynamically allocated ones |
21 |
> > |
22 |
> > 3. when some constified statically allocated variables do need to be modified |
23 |
> > at runtime |
24 |
> > |
25 |
> > if you look at PaX carefully, you'll find use cases for each of the above, |
26 |
> > it should be your guidance for patching aufs as well. although i didn't look |
27 |
> > at its code, i think aufs is case #1 or #2. |
28 |
> > |
29 |
> |
30 |
> I agree with option 1: types should be declared const or no_const |
31 |
> As enforcing this should lead to a more robust kernel. |
32 |
> And this means there should not be any constify_plugin trick to infer |
33 |
> if a type is const or not. |
34 |
> The compiler should require attribute declaration. |
35 |
> But this is just utopia.... |
36 |
|
37 |
i think you misunderstood something ;). the above 3 cases are not something you |
38 |
can agree/disagree with, they're cases that may or may not apply to a given piece |
39 |
of code and depending on the exact situation you have 3 ways to adjust the code |
40 |
to work with the constification plugin (it is also a valid/possible adjustment |
41 |
to turn the code from one case to another, e.g., by getting rid of runtime ops |
42 |
structure allocation, PaX has examples of this kind of change too, but i didn't |
43 |
want to complicate my initial explanation ;). |
44 |
|
45 |
so with that said, the constify plugin has to exist exactly because the kernel |
46 |
has all kinds of ops structure types (as in, there're legitimate use cases for |
47 |
each of the 3 situations) and we need the do_const/no_const attributes because |
48 |
the compiler cannot in general determine how a given ops structure type is used |
49 |
in the entire code base (with LTO it may be feasible though). |
50 |
|
51 |
> The problem with option 2 is that this leaves the constification |
52 |
> question unsolved. |
53 |
|
54 |
i don't understand this... |
55 |
|
56 |
> Beside that this forces developers to modify their code either to use |
57 |
> "no_const" types or to write specific patches. |
58 |
|
59 |
exactly. consider this as documenting the behaviour of the code. not unlike |
60 |
you do every time you use 'const' or 'unsigned' or any attributes, etc. |
61 |
|
62 |
> Looking at code I don't understand why, for example, grsecurity patch |
63 |
> provides a "no_const" struct for seq_operations (in include/linux/seq_file.h) |
64 |
|
65 |
it's simple, you'd have had to grep for seq_operations_no_const only :) |
66 |
(hint: it's case #2). |
67 |
|
68 |
> and no special "typedef" for "struct fsnotify_ops"... |
69 |
|
70 |
because fsnotify_ops is not case #2 in the vanilla kernel but now you appear |
71 |
to be saying that aufs makes it into case #2 (can you/anyone confirm it?) in |
72 |
which case i'll add the typedef to PaX and aufs can rely on fsnotify_ops_no_const |
73 |
from then on. |
74 |
|
75 |
and if there's anything else, just let me know (obviously for ops structures |
76 |
declared by aufs itself it's up to aufs to provide/use the typedefs, __no_const, |
77 |
etc). |
78 |
|
79 |
> This means I cannot support option 1 and then the best is to bypass |
80 |
> compiler constification, using Okajima trick... |
81 |
> |
82 |
> //br->br_hfsn_ops = au_hfsn_ops; |
83 |
> BUILD_BUG_ON(sizeof(br->br_hfsn_ops) != sizeof(au_hfsn_ops)); |
84 |
> memcoy((void *)&br->br_hfsn_ops, (void *)&au_hfsn_ops, sizeof(au_hfsn_ops)); |
85 |
> |
86 |
> instead of the more clear: |
87 |
> |
88 |
> br->br_hfsn_ops = au_hfsn_ops; |
89 |
|
90 |
you can keep using the simple code if you use the proper types ;). |
91 |
|
92 |
> Is this going to work? |
93 |
|
94 |
you/someone will have to determine which of the 3 cases you have in aufs (for |
95 |
each affected type). |
96 |
|
97 |
here, if 'br' points to a dynamically allocated structure (think kmalloc) then |
98 |
it means that for this ops type you have case #1 or #2, so you'll either need |
99 |
__no_const on the original declaration or a new typedef'ed no_const type. |
100 |
|
101 |
> Aren't const struct enforced at runtime? |
102 |
|
103 |
it depends on where the given object is stored. runtime enforcement normally |
104 |
applies only to variables of static storage (i.e., variables declared in global |
105 |
scope), local variables and dynamically allocated ones remain writable (as far |
106 |
as the CPU is concerned, that's why cast tricks work there whereas they'd fail |
107 |
at runtime if used against static variables). with some ugly hacks you could make |
108 |
dynamically allocated objects read-only as well, but i don't know of anyone who |
109 |
went that far yet. |