status of constification

Hi,

So, I'm trying to come up to speed on what's still outstanding in the effort to constify function pointers. I saw patches from Emese Revfy go in, and I saw Lionel Debroux's recent patches. What already-created work still needs attention? I know there's more code to be written, but I'm trying to find any patches that have already been written but have not gone upstream yet.

Quoted 9 lines

Hi,

So, I'm trying to come up to speed on what's still outstanding in the effort to constify function pointers. I saw patches from Emese Revfy go in, and I saw Lionel Debroux's recent patches. What already-created work still needs attention? I know there's more code to be written, but I'm trying to find any patches that have already been written but have not gone upstream yet.

Hi,

I will gladly break up my current patch for the next -rc by structure type or maintainer (some preferred it one way or the other) and send it in some time next week so that you can handle the upstream submission process (I will continue to maintain my patch in grsecurity).

There are many structures that can be constified, you can use the following command to find most of them (use it on an allyesconfig kernel preferably):

grep _ops System.map |grep -Ewi 'b|d' | awk '{print $3}' | \
while read i ; do cscope -d -L -1 $i | grep -E "struct[ \t]*([^ ]*)[ \t]*" \
--color=none -o | awk '{print $2}' ; done |sort -u

Also there are always new instances of structures going in that should have been constified.

I tried to automate the whole process with Coccinelle but I abandoned it because Coccinelle didn't support recursive header file inclusion at the time. If someone feels like fixing Coccinelle then I would quickly finish my script (it has a few bugs because I could never test it for real), but see the end of the mail for the current version. I think it would be a good idea because it would take a few hours only to generate a constification patch for a new kernel. One thing that probably cannot be automated with Coccinelle is that once the script determines that a given structure cannot be constified, it cannot undo already emitted patches for the given structure so it must be cleaned up by post processing script.

--
Emese

// spatch.opt -sp_file $1 -include_headers -local_includes -all_includes -I "include/" -dir $2

@initialize:python@
noconst = []

@stc@
identifier idtype, y;
type t;
position p;
@@

struct idtype {
	...
	t (*y)(...);@p
	...
};

@notjustfp@
identifier stc.idtype, y;
type t;
position p != stc.p;
@@

struct idtype {
	...
	t y;@p
	...
};

@script:python depends on notjustfp@
@@
cocci.include_match(False)

@variable@
identifier stc.idtype, idvariant, id;
@@
(

	struct idtype idvariant = {
		...
	};
|
	struct idtype idvariant;
|
	struct idtype *idvariant;
|
	struct id {
		...
		struct idtype idvariant;
		...
	};

)

@script:python@
y << variable.idvariant;
@@
if y in noconst:
cocci.include_match(False)

@alreadyconst@
identifier stc.idtype, variable.idvariant, id;
@@
(

	const struct idtype idvariant;
|
	const struct idtype idvariant = {
		...
	};
|
	const struct idtype *idvariant;
|
	struct id {
		...
		const struct idtype idvariant;
		...
	};

)

@script:python depends on alreadyconst@
@@
cocci.include_match(False)

@fn_declaration@
identifier stc.idtype, variable.idvariant, fn;
type t;
@@
t fn(struct idtype *idvariant);

@fn_definition@
identifier stc.idtype, variable.idvariant, fn;
type t;
@@
t fn(struct idtype *idvariant)

{
	...
}

// TODO: handle var.field1.field2, var->field1->field2 @assignement@
identifier variable.idvariant, x, idptr;
@@
(

	idvariant.x = ...;
|
	idvariant->x = ...;
|
	idptr = &idvariant;
	...
	idptr->x = ...;
|
	memcpy(&idvariant, ...);
|
	memcpy(idvariant.x, ...);
|
	memcpy(idvariant->x, ...);
|
	idvariant = kzalloc(...);
|
	idvariant = kmalloc(...);

)

@script:python depends on assignement@
x << stc.idtype;
y << variable.idvariant;
@@
print "Cannot be const: %s-%s" % (x, y)
noconst.append(y)
cocci.include_match(False)

@depends on stc && !fn_declaration && !fn_definition@ identifier stc.idtype, variable.idvariant, id; @@
(

(quoted diff) +1 -1

-struct idtype idvariant = {
+const struct idtype idvariant = {
...
 };

|

(quoted diff) +1 -1

-struct idtype idvariant;
+const struct idtype idvariant;

|

(quoted diff) +1 -1

-struct idtype *idvariant;
+const struct idtype *idvariant;

|

(quoted diff) +1 -1

-struct idtype *idvariant = NULL;
+const struct idtype *idvariant = NULL;

|
struct id {
...

(quoted diff) +1 -1

-struct idtype idvariant;
+const struct idtype idvariant;
...
 };

)

@depends on stc && fn_declaration && !fn_definition@ identifier stc.idtype, variable.idvariant, fn_declaration.fn; type fn_declaration.t;
@@
t fn(

(quoted diff) +1 -1

-struct idtype *idvariant
+const struct idtype *idvariant
 );

@depends on stc && !fn_declaration && fn_definition@ identifier stc.idtype, variable.idvariant, fn_definition.fn; type fn_definition.t;
@@
t fn(

(quoted diff) +1 -1

-struct idtype *idvariant
+const struct idtype *idvariant
 )
 {
...
 }

Quoted 10 lines

I tried to automate the whole process with Coccinelle but I abandoned it because Coccinelle didn't support recursive header file inclusion at the time. If someone feels like fixing Coccinelle then I would quickly finish my script (it has a few bugs because I could never test it for real), but see the end of the mail for the current version. I think it would be a good idea because it would take a few hours only to generate a constification patch for a new kernel. One thing that probably cannot be automated with Coccinelle is that once the script determines that a given structure cannot be constified, it cannot undo already emitted patches for the given structure so it must be cleaned up by post processing script.

What would the right approach be? It is not obvious to find 100% of the header files, because some of them depend on information in Makefiles.

You can use that information by running the preprocessor on Coccinelle first, but then the result is only useful for finding files that need changing, but not actually making the changes because Coccinelle does not relate the preprocessed code back to the original code. But if you run the preprocessor, you only get information for your current configuration, which is probably not what you want.

Coccinelle could certainly get a new option -really_all_includes, or something like that, that would recursively include among what it can find and what has not been included already. Would that be what is wanted?

I guess that in practice the includes are only being used for type information? Wouldn't it be safe to run the semantic patch based on the includes that are available?

julia

Quoted 179 lines

--
Emese

// spatch.opt -sp_file $1 -include_headers -local_includes -all_includes -I "include/" -dir $2

@initialize:python@
noconst = []

@stc@
identifier idtype, y;
type t;
position p;
@@

struct idtype {
	...
	t (*y)(...);@p
	...
};

@notjustfp@
identifier stc.idtype, y;
type t;
position p != stc.p;
@@

struct idtype {
	...
	t y;@p
	...
};

@script:python depends on notjustfp@
@@
cocci.include_match(False)

@variable@
identifier stc.idtype, idvariant, id;
@@
(

	struct idtype idvariant = {
		...
	};
|
	struct idtype idvariant;
|
	struct idtype *idvariant;
|
	struct id {
		...
		struct idtype idvariant;
		...
	};

)

@script:python@
y << variable.idvariant;
@@
if y in noconst:
cocci.include_match(False)

@alreadyconst@
identifier stc.idtype, variable.idvariant, id;
@@
(

	const struct idtype idvariant;
|
	const struct idtype idvariant = {
		...
	};
|
	const struct idtype *idvariant;
|
	struct id {
		...
		const struct idtype idvariant;
		...
	};

)

@script:python depends on alreadyconst@
@@
cocci.include_match(False)

@fn_declaration@
identifier stc.idtype, variable.idvariant, fn;
type t;
@@
t fn(struct idtype *idvariant);

@fn_definition@
identifier stc.idtype, variable.idvariant, fn;
type t;
@@
t fn(struct idtype *idvariant)

{
	...
}

// TODO: handle var.field1.field2, var->field1->field2 @assignement@
identifier variable.idvariant, x, idptr;
@@
(

	idvariant.x = ...;
|
	idvariant->x = ...;
|
	idptr = &idvariant;
	...
	idptr->x = ...;
|
	memcpy(&idvariant, ...);
|
	memcpy(idvariant.x, ...);
|
	memcpy(idvariant->x, ...);
|
	idvariant = kzalloc(...);
|
	idvariant = kmalloc(...);

)

@script:python depends on assignement@
x << stc.idtype;
y << variable.idvariant;
@@
print "Cannot be const: %s-%s" % (x, y)
noconst.append(y)
cocci.include_match(False)

@depends on stc && !fn_declaration && !fn_definition@ identifier stc.idtype, variable.idvariant, id; @@
(

(quoted diff) +1 -1

-struct idtype idvariant = {
+const struct idtype idvariant = {
...
 };

|

(quoted diff) +1 -1

-struct idtype idvariant;
+const struct idtype idvariant;

|

(quoted diff) +1 -1

-struct idtype *idvariant;
+const struct idtype *idvariant;

|

(quoted diff) +1 -1

-struct idtype *idvariant = NULL;
+const struct idtype *idvariant = NULL;

|
struct id {
...

(quoted diff) +1 -1

-struct idtype idvariant;
+const struct idtype idvariant;
...
 };

)

@depends on stc && fn_declaration && !fn_definition@ identifier stc.idtype, variable.idvariant, fn_declaration.fn; type fn_declaration.t;
@@
t fn(

(quoted diff) +1 -1

-struct idtype *idvariant
+const struct idtype *idvariant
 );

@depends on stc && !fn_declaration && fn_definition@ identifier stc.idtype, variable.idvariant, fn_definition.fn; type fn_definition.t;
@@
t fn(

(quoted diff) +1 -1

-struct idtype *idvariant
+const struct idtype *idvariant
 )
 {
...
 }

Quoted 11 lines · Julia Lawall

Quoted 10 lines

I tried to automate the whole process with Coccinelle but I abandoned it because Coccinelle didn't support recursive header file inclusion at the time. If someone feels like fixing Coccinelle then I would quickly finish my script (it has a few bugs because I could never test it for real), but see the end of the mail for the current version. I think it would be a good idea because it would take a few hours only to generate a constification patch for a new kernel. One thing that probably cannot be automated with Coccinelle is that once the script determines that a given structure cannot be constified, it cannot undo already emitted patches for the given structure so it must be cleaned up by post processing script.

Quoted 2 lines

What would the right approach be? It is not obvious to find 100% of the header files, because some of them depend on information in Makefiles.

For 100% coverage you can look at how the Linux Makefiles invoke sparse.

Quoted 10 lines

You can use that information by running the preprocessor on Coccinelle first, but then the result is only useful for finding files that need changing, but not actually making the changes because Coccinelle does not relate the preprocessed code back to the original code. But if you run the preprocessor, you only get information for your current configuration, which is probably not what you want.

Coccinelle could certainly get a new option -really_all_includes, or something like that, that would recursively include among what it can find and what has not been included already. Would that be what is wanted?

Yes, this is exactly what I'd like to have, missing the few includes you referred to above is not a problem for my purposes.

Thanks, Emese

Quoted 187 lines

I guess that in practice the includes are only being used for type information? Wouldn't it be safe to run the semantic patch based on the includes that are available?

julia

Quoted 179 lines

--
Emese

// spatch.opt -sp_file $1 -include_headers -local_includes -all_includes -I "include/" -dir $2

@initialize:python@
noconst = []

@stc@
identifier idtype, y;
type t;
position p;
@@

struct idtype {
	...
	t (*y)(...);@p
	...
};

@notjustfp@
identifier stc.idtype, y;
type t;
position p != stc.p;
@@

struct idtype {
	...
	t y;@p
	...
};

@script:python depends on notjustfp@
@@
cocci.include_match(False)

@variable@
identifier stc.idtype, idvariant, id;
@@
(

	struct idtype idvariant = {
		...
	};
|
	struct idtype idvariant;
|
	struct idtype *idvariant;
|
	struct id {
		...
		struct idtype idvariant;
		...
	};

)

@script:python@
y << variable.idvariant;
@@
if y in noconst:
cocci.include_match(False)

@alreadyconst@
identifier stc.idtype, variable.idvariant, id;
@@
(

	const struct idtype idvariant;
|
	const struct idtype idvariant = {
		...
	};
|
	const struct idtype *idvariant;
|
	struct id {
		...
		const struct idtype idvariant;
		...
	};

)

@script:python depends on alreadyconst@
@@
cocci.include_match(False)

@fn_declaration@
identifier stc.idtype, variable.idvariant, fn;
type t;
@@
t fn(struct idtype *idvariant);

@fn_definition@
identifier stc.idtype, variable.idvariant, fn;
type t;
@@
t fn(struct idtype *idvariant)

{
	...
}

// TODO: handle var.field1.field2, var->field1->field2 @assignement@
identifier variable.idvariant, x, idptr;
@@
(

	idvariant.x = ...;
|
	idvariant->x = ...;
|
	idptr = &idvariant;
	...
	idptr->x = ...;
|
	memcpy(&idvariant, ...);
|
	memcpy(idvariant.x, ...);
|
	memcpy(idvariant->x, ...);
|
	idvariant = kzalloc(...);
|
	idvariant = kmalloc(...);

)

@script:python depends on assignement@
x << stc.idtype;
y << variable.idvariant;
@@
print "Cannot be const: %s-%s" % (x, y)
noconst.append(y)
cocci.include_match(False)

@depends on stc && !fn_declaration && !fn_definition@ identifier stc.idtype, variable.idvariant, id; @@
(

(quoted diff) +1 -1

-struct idtype idvariant = {
+const struct idtype idvariant = {
...
 };

|

(quoted diff) +1 -1

-struct idtype idvariant;
+const struct idtype idvariant;

|

(quoted diff) +1 -1

-struct idtype *idvariant;
+const struct idtype *idvariant;

|

(quoted diff) +1 -1

-struct idtype *idvariant = NULL;
+const struct idtype *idvariant = NULL;

|
struct id {
...

(quoted diff) +1 -1

-struct idtype idvariant;
+const struct idtype idvariant;
...
 };

)

@depends on stc && fn_declaration && !fn_definition@ identifier stc.idtype, variable.idvariant, fn_declaration.fn; type fn_declaration.t;
@@
t fn(

(quoted diff) +1 -1

-struct idtype *idvariant
+const struct idtype *idvariant
 );

@depends on stc && !fn_declaration && fn_definition@ identifier stc.idtype, variable.idvariant, fn_definition.fn; type fn_definition.t;
@@
t fn(

(quoted diff) +1 -1

-struct idtype *idvariant
+const struct idtype *idvariant
 )
 {
...
 }

Quoted 4 lines

Quoted 2 lines

What would the right approach be? It is not obvious to find 100% of the header files, because some of them depend on information in Makefiles.

For 100% coverage you can look at how the Linux Makefiles invoke sparse.

I haven't looked at it, but I doubt it gives 100% coverage, because one can have code in both the if and else branches of an ifdef. I would imagine that it gives 100% coverage for whatever architecture you would be compiling for?

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

What would the right approach be? It is not obvious to find 100% of the header files, because some of them depend on information in Makefiles.

For 100% coverage you can look at how the Linux Makefiles invoke sparse.

I haven't looked at it, but I doubt it gives 100% coverage, because one can have code in both the if and else branches of an ifdef. I would imagine that it gives 100% coverage for whatever architecture you would be compiling for?

For my purposes it's enough to find all includes for a given .config.

What would be also useful is if Coccinelle could somehow ignore ifdef's that guard code blocks so that it would analyse all code in a translation unit.

I will gladly break up my current patch for the next -rc by structure type or maintainer (some preferred it one way or the other) and send it in some time next week so that you can handle the upstream submission process (I will continue to maintain my patch in grsecurity).

There are many structures that can be constified, you can use the following command to find most of them (use it on an allyesconfig kernel preferably):

grep _ops System.map |grep -Ewi 'b|d' | awk '{print $3}' | \
while read i ; do cscope -d -L -1 $i | grep -E "struct[ \t]*([^ ]*)[ \t]*" \
--color=none -o | awk '{print $2}' ; done |sort -u

Also there are always new instances of structures going in that should have been constified.

I tried to automate the whole process with Coccinelle but I abandoned it because Coccinelle didn't support recursive header file inclusion at the time. If someone feels like fixing Coccinelle then I would quickly finish my script (it has a few bugs because I could never test it for real), but see the end of the mail for the current version. I think it would be a good idea because it would take a few hours only to generate a constification patch for a new kernel. One thing that probably cannot be automated with Coccinelle is that once the script determines that a given structure cannot be constified, it cannot undo already emitted patches for the given structure so it must be cleaned up by post processing script.

Hi Emese,

I got distracted, but I'd like to get back to this thread...

Quoted 12 lines · Emese Revfy

I will gladly break up my current patch for the next -rc by structure type or maintainer (some preferred it one way or the other) and send it in some time next week so that you can handle the upstream submission process (I will continue to maintain my patch in grsecurity).

There are many structures that can be constified, you can use the following command to find most of them (use it on an allyesconfig kernel preferably):

grep _ops System.map |grep -Ewi 'b|d' | awk '{print $3}' | \
while read i ; do cscope -d -L -1 $i | grep -E "struct[ \t]*([^ ]*)[ \t]*" \
--color=none -o | awk '{print $2}' ; done |sort -u

Quoted 2 lines

Also there are always new instances of structures going in that should have been constified.

Just in my running kernel, I see 56 _ops structures reported from the above search. :)

Do you have a new stack of patches I can help usher into the kernel? I don't want reinvent the wheel if I don't have to. :)

Quoted 10 lines

I tried to automate the whole process with Coccinelle but I abandoned it because Coccinelle didn't support recursive header file inclusion at the time. If someone feels like fixing Coccinelle then I would quickly finish my script (it has a few bugs because I could never test it for real), but see the end of the mail for the current version. I think it would be a good idea because it would take a few hours only to generate a constification patch for a new kernel. One thing that probably cannot be automated with Coccinelle is that once the script determines that a given structure cannot be constified, it cannot undo already emitted patches for the given structure so it must be cleaned up by post processing script.

Has there been any update to your Coccinelle script since the addition of -recursive_includes?

Thanks!

-Kees

Signature · 7 lines

Kees Cook
Ubuntu Security Team
_______________________________________________
Cocci mailing list
[email protected]
http://lists.diku.dk/mailman/listinfo/cocci
(Web access from inside DIKUs LAN only)

Could I see the semantic patch? The clean up issue sounds interesting. Perhaps there is a way around it.

Quoted 2 lines

Could I see the semantic patch? The clean up issue sounds interesting. Perhaps there is a way around it.

Hi,
I no longer use Coccinelle to generate constification patch, but only to find candidate structures for constification. This is because I ran into several issues
some of which I will try describe now from memory (unfortunately I don't have those Cocci scripts anymore):

1. Sometimes (always?) cocci removed NULL initialization while emitting the constified patch:

linux-2.6.39-rc7/arch/frv/mb93090-mb00/pci-vdk.c

(quoted diff) +1 -1

@@ -356,7 +356,7 @@ void __init pcibios_fixup_bus(struct pci
 
 int __init pcibios_init(void)
 {
-       struct pci_ops *dir = NULL;
+       const struct pci_ops *dir;
 
        if (!mb93090_mb00_detected)
                return -ENXIO;

2. Sometimes (always?) cocci joined structure field lines into one line while constifying them:

linux-2.6.39-rc7/arch/x86/include/asm/x86_init.h

(quoted diff) +1 -8

@@ -119,14 +119,7 @@ struct x86_init_pci {
  *
  */
 struct x86_init_ops {
-       struct x86_init_resources       resources;
-       struct x86_init_mpparse         mpparse;
-       struct x86_init_irqs            irqs;
-       struct x86_init_oem             oem;
-       struct x86_init_paging          paging;
-       struct x86_init_timers          timers;
-       struct x86_init_iommu           iommu;
-       struct x86_init_pci             pci;
+       const struct x86_init_resources resources;const struct

x86_init_mpparse mpparse;const struct x86_init_irqs irqs;const struct x86_init_oem oem;const struct x86_init_paging paging;const struct x86_init_timers timers;const struct x86_init_iommu iommu;const struct x86_init_pci pci;
};

/**

3. Sometimes cocci removed comment lines while joining others, maybe related to the previous issue:
linux-2.6.39-rc7/drivers/net/wireless/ath/ath9k/hw.h

(quoted diff) +1 -3

@@ -753,9 +753,7 @@ struct ath_hw {
        } enable_32kHz_clock;
 
        /* Private to hardware code */
-       struct ath_hw_private_ops private_ops;
-       /* Accessed by the lower level driver */
-       struct ath_hw_ops ops;
+       const struct ath_hw_private_ops private_ops;const struct ath_hw_ops
 ops;
 
        /* Used to program the radio on non single-chip devices */

        u32 *analogBank0Data;

4. My constification patch generator cocci script didn't find all structures that it should have, I don't know whether it was a bug in my script or in cocci but in the end I just gave it up.

--
Emese

Quoted 2 lines

Could I see the semantic patch? The clean up issue sounds interesting. Perhaps there is a way around it.

Hi,
I no longer use Coccinelle to generate constification patch, but only to find candidate structures for constification. This is because I ran into several issues
some of which I will try describe now from memory (unfortunately I don't have those Cocci scripts anymore):

1. Sometimes (always?) cocci removed NULL initialization while emitting the constified patch:

linux-2.6.39-rc7/arch/frv/mb93090-mb00/pci-vdk.c

(quoted diff) +1 -1

@@ -356,7 +356,7 @@ void __init pcibios_fixup_bus(struct pci
 
 int __init pcibios_init(void)
 {
-       struct pci_ops *dir = NULL;
+       const struct pci_ops *dir;
 
        if (!mb93090_mb00_detected)
                return -ENXIO;

2. Sometimes (always?) cocci joined structure field lines into one line while constifying them:

linux-2.6.39-rc7/arch/x86/include/asm/x86_init.h

(quoted diff) +1 -8

@@ -119,14 +119,7 @@ struct x86_init_pci {
  *
  */
 struct x86_init_ops {
-       struct x86_init_resources       resources;
-       struct x86_init_mpparse         mpparse;
-       struct x86_init_irqs            irqs;
-       struct x86_init_oem             oem;
-       struct x86_init_paging          paging;
-       struct x86_init_timers          timers;
-       struct x86_init_iommu           iommu;
-       struct x86_init_pci             pci;
+       const struct x86_init_resources resources;const struct

x86_init_mpparse mpparse;const struct x86_init_irqs irqs;const struct x86_init_oem oem;const struct x86_init_paging paging;const struct x86_init_timers timers;const struct x86_init_iommu iommu;const struct x86_init_pci pci;
};

/**

3. Sometimes cocci removed comment lines while joining others, maybe related to the previous issue:
linux-2.6.39-rc7/drivers/net/wireless/ath/ath9k/hw.h

(quoted diff) +1 -3

@@ -753,9 +753,7 @@ struct ath_hw {
        } enable_32kHz_clock;
 
        /* Private to hardware code */
-       struct ath_hw_private_ops private_ops;
-       /* Accessed by the lower level driver */
-       struct ath_hw_ops ops;
+       const struct ath_hw_private_ops private_ops;const struct ath_hw_ops
 ops;
 
        /* Used to program the radio on non single-chip devices */

        u32 *analogBank0Data;

4. My constification patch generator cocci script didn't find all structures that it should have, I don't know whether it was a bug in my script or in cocci but in the end I just gave it up.

--
Emese

This problem seem to be solved. At least, I don't see the problem for the following semantic patch:

@@
identifier i,x;
type T;
@@

struct i {
   ...
- T x;
+ const T x;
   ...
};

on the following code:

struct i {
   int a;
   int b;
   /* a comment */
   int c;
   int d;
};

The comment is preserved as well.