Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Alexey Dobriyan]

> -   	struct inode *(*alloc_inode)(struct super_block *sb);
> +   	struct inode *(* const alloc_inode)(struct super_block *sb);

Good rule is if adding const doesn't move object from one section
to another, it isn't worth it.

I suggest we stick to it or risk another wave of jumbo patches.

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Emese Revfy]

Alexey Dobriyan wrote:
>> -   	struct inode *(*alloc_inode)(struct super_block *sb);
>> +   	struct inode *(* const alloc_inode)(struct super_block *sb);
> 
> Good rule is if adding const doesn't move object from one section
> to another, it isn't worth it.
> 
> I suggest we stick to it or risk another wave of jumbo patches.
> 
If all instances of a given ops structure are const and we would like to
preserve this policy for the future as well, then  it is very useful
to give future programmers a strong hint about this policy by making
the compiler complain about any violation attempts. Otherwise they may
very well write code that modifies such structures and we will have to
work extra to undo that (or change the policy but in that case it is
good to know why we have to do that).
--
Emese

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Alexey Dobriyan]

On 12/6/09, Emese Revfy <re.emese@gmail.com> wrote:
> Alexey Dobriyan wrote:
>>> -   	struct inode *(*alloc_inode)(struct super_block *sb);
>>> +   	struct inode *(* const alloc_inode)(struct super_block *sb);
>>
>> Good rule is if adding const doesn't move object from one section
>> to another, it isn't worth it.
>>
>> I suggest we stick to it or risk another wave of jumbo patches.
>>
> If all instances of a given ops structure are const and we would like to
> preserve this policy for the future as well, then  it is very useful
> to give future programmers a strong hint about this policy by making
> the compiler complain about any violation attempts. Otherwise they may
> very well write code that modifies such structures and we will have to
> work extra to undo that (or change the policy but in that case it is
> good to know why we have to do that).

You may want to look what filesystems do with superblock operations.
And after super operations were made const writes to it will be caught
with readonly .rodata config option.

You're going too far with these modifiers.

Nothing will be caught.

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Emese Revfy]

Alexey Dobriyan wrote:
> On 12/6/09, Emese Revfy <re.emese@gmail.com> wrote:
>> Alexey Dobriyan wrote:
>>>> -   	struct inode *(*alloc_inode)(struct super_block *sb);
>>>> +   	struct inode *(* const alloc_inode)(struct super_block *sb);
>>> Good rule is if adding const doesn't move object from one section
>>> to another, it isn't worth it.
>>>
>>> I suggest we stick to it or risk another wave of jumbo patches.
>>>
>> If all instances of a given ops structure are const and we would like to
>> preserve this policy for the future as well, then  it is very useful
>> to give future programmers a strong hint about this policy by making
>> the compiler complain about any violation attempts. Otherwise they may
>> very well write code that modifies such structures and we will have to
>> work extra to undo that (or change the policy but in that case it is
>> good to know why we have to do that).
> 
> You may want to look what filesystems do with superblock operations.
> And after super operations were made const writes to it will be caught
> with readonly .rodata config option.
> 
> You're going too far with these modifiers.
> 
> Nothing will be caught.

DEBUG_RODATA catches the unwanted write attempt at runtime whereas
my patch will catch it at compile time. I think it's better to detect
an error as early as possible.
--
Emese

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Al Viro]

On Tue, Dec 08, 2009 at 01:06:38AM +0100, Emese Revfy wrote:

> DEBUG_RODATA catches the unwanted write attempt at runtime whereas
> my patch will catch it at compile time. I think it's better to detect
> an error as early as possible.

Not when the price is readability.  Moreover, you *still* are not
covering the real policy - these suckers should be statically allocated,
not just never modified.

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Emese Revfy]

Al Viro wrote:
> On Tue, Dec 08, 2009 at 01:06:38AM +0100, Emese Revfy wrote:
> 
>> DEBUG_RODATA catches the unwanted write attempt at runtime whereas
>> my patch will catch it at compile time. I think it's better to detect
>> an error as early as possible.
> 
> Not when the price is readability.  

If constifying the function pointer fields reduces readability,
what would you say for turning then into typedefs, something like this:

typedef int (* super_ops_statfs) (struct dentry *, struct kstatfs *);
struct super_operations {
...
	const super_ops_statfs statfs;
...
};

> Moreover, you *still* are not
> covering the real policy - these suckers should be statically allocated,
> not just never modified.

If the super ops are allocated on the stack then they will be overwritten
during later syscalls and will eventually crash the system on a future
dereference, that is, this kind of problem manifests during development.

If the super ops are allocated by kmalloc/etc, then they will have to be
explicitly initialised by writing to specific fields, my patch would prevent
that.

So in the end the programmer is forced to allocate and initialise super ops
statically.
---
Emese

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Al Viro]

On Wed, Dec 09, 2009 at 01:24:34AM +0100, Emese Revfy wrote:

> If constifying the function pointer fields reduces readability,
> what would you say for turning then into typedefs, something like this:
> 
> typedef int (* super_ops_statfs) (struct dentry *, struct kstatfs *);
> struct super_operations {
> ...
> 	const super_ops_statfs statfs;
> ...
> };

Even worse, since one has to go back to typedef to figure out WTF is
going on.
 
> > Moreover, you *still* are not
> > covering the real policy - these suckers should be statically allocated,
> > not just never modified.
> 
> If the super ops are allocated on the stack then they will be overwritten
> during later syscalls and will eventually crash the system on a future
> dereference, that is, this kind of problem manifests during development.
> 
> If the super ops are allocated by kmalloc/etc, then they will have to be
> explicitly initialised by writing to specific fields, my patch would prevent
> that.
> 
> So in the end the programmer is forced to allocate and initialise super ops
> statically.

... unless they go ahead and use memcpy(), etc.

What you really want is
	* no conversions to any other pointer types for pointers to it
and to any aggregate types containing it
	* no conversions from any other pointer types for the same set of
types
	* all objects of that type have static storage duration
	* no lvalues of that type are modifiable

Which is not a job for C compiler.  Yes, (4) means that memcpy() et.al.
give undefined behaviour.  And you get fsck-all satisfaction from knowing
that, since C compiler is not going to warn you about it.  sparse might,
if we teach it to do so.  Preferably - with minimal intrusiveness of
syntax being used.

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Ralf Baechle]

On Sun, Dec 06, 2009 at 08:14:46AM +0300, Alexey Dobriyan wrote:

> Subject: Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1
> Content-Type: text/plain; charset=ISO-8859-1
> 
> > -   	struct inode *(*alloc_inode)(struct super_block *sb);
> > +   	struct inode *(* const alloc_inode)(struct super_block *sb);
> 
> Good rule is if adding const doesn't move object from one section
> to another, it isn't worth it.

On MIPS I've changed a few pointer arguments that frequently were abused
by platform-specific code to const just to make sure such code blows up
at compile time and not later in my mail folder at review time.

  Ralf

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Al Viro]

On Wed, Dec 09, 2009 at 01:31:10AM +0000, Ralf Baechle wrote:
> On Sun, Dec 06, 2009 at 08:14:46AM +0300, Alexey Dobriyan wrote:
> 
> > Subject: Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1
> > Content-Type: text/plain; charset=ISO-8859-1
> > 
> > > -   	struct inode *(*alloc_inode)(struct super_block *sb);
> > > +   	struct inode *(* const alloc_inode)(struct super_block *sb);
> > 
> > Good rule is if adding const doesn't move object from one section
> > to another, it isn't worth it.
> 
> On MIPS I've changed a few pointer arguments that frequently were abused
> by platform-specific code to const just to make sure such code blows up
> at compile time and not later in my mail folder at review time.

Seriously, folks, that looks like a fun sparse project: new type attribute
that makes pointer conversions complain (inherited by aggregates containing
one of such things) + one that restricts storage classes (again, inherited
the same way).  Another fun attribute: "no embedding into other objects".
__force casts would suppress complaints in places where e.g. noconvert
objects get allocated.

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Olivier Galibert]

On Wed, Dec 09, 2009 at 12:47:34AM +0000, Al Viro wrote:
> What you really want is
> 	* no conversions to any other pointer types for pointers to it
> and to any aggregate types containing it
> 	* no conversions from any other pointer types for the same set of
> types
> 	* all objects of that type have static storage duration
> 	* no lvalues of that type are modifiable

What about the ->owner field?

  OG.

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Emese Revfy]

Al Viro wrote:
> On Wed, Dec 09, 2009 at 01:24:34AM +0100, Emese Revfy wrote:
> 
>> If constifying the function pointer fields reduces readability,
>> what would you say for turning then into typedefs, something like this:
>>
>> typedef int (* super_ops_statfs) (struct dentry *, struct kstatfs *);
>> struct super_operations {
>> ...
>> 	const super_ops_statfs statfs;
>> ...
>> };
> 
> Even worse, since one has to go back to typedef to figure out WTF is
> going on.
>  
>>> Moreover, you *still* are not
>>> covering the real policy - these suckers should be statically allocated,
>>> not just never modified.
>> If the super ops are allocated on the stack then they will be overwritten
>> during later syscalls and will eventually crash the system on a future
>> dereference, that is, this kind of problem manifests during development.
>>
>> If the super ops are allocated by kmalloc/etc, then they will have to be
>> explicitly initialised by writing to specific fields, my patch would prevent
>> that.
>>
>> So in the end the programmer is forced to allocate and initialise super ops
>> statically.
> 
> ... unless they go ahead and use memcpy(), etc.
> 
> What you really want is
> 	* no conversions to any other pointer types for pointers to it
> and to any aggregate types containing it
> 	* no conversions from any other pointer types for the same set of
> types
> 	* all objects of that type have static storage duration
> 	* no lvalues of that type are modifiable
> 
> Which is not a job for C compiler.  Yes, (4) means that memcpy() et.al.
> give undefined behaviour.  And you get fsck-all satisfaction from knowing
> that, since C compiler is not going to warn you about it.  sparse might,
> if we teach it to do so.  Preferably - with minimal intrusiveness of
> syntax being used.

I think, all these instruments (constification, sparse, etc.) are not 
for preventing a programmer from circumventing the policy (that's impossible),
but to make it easy for the reviewer to notice it when he does so.
My patch achieves this in a very simple way for the currently uncovered case of dynamically
allocated ops structures.
--
Emese

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Emese Revfy]

Al Viro wrote:
> On Fri, Dec 04, 2009 at 11:47:15PM +0100, Emese Revfy wrote:
>> From: Emese Revfy <re.emese@gmail.com>
>>
>> Constify struct super_operations.
> 
> What the _hell_ is that?
> 
>> +   	struct inode *(* const alloc_inode)(struct super_block *sb);
> 
> This is ugly and has all marks of cargo-cult patches.  NAKed at least
> until you give exceptionally good reasons for doing that.

My idea was that since each instance of super_operations is const,
I figured that there is an implicit policy of not wanting writable
super_operations structures in the kernel. If this is actually the
case then my patch makes the compiler enforce this policy, otherwise
please ignore it.
--
Emese

Re: [PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Al Viro]

On Fri, Dec 04, 2009 at 11:47:15PM +0100, Emese Revfy wrote:
> From: Emese Revfy <re.emese@gmail.com>
> 
> Constify struct super_operations.

What the _hell_ is that?

> +   	struct inode *(* const alloc_inode)(struct super_block *sb);

This is ugly and has all marks of cargo-cult patches.  NAKed at least
until you give exceptionally good reasons for doing that.

[PATCH 28/31] Constify struct super_operations for 2.6.32 v1

[Emese Revfy]

From: Emese Revfy <re.emese@gmail.com>

Constify struct super_operations.

Signed-off-by: Emese Revfy <re.emese@gmail.com>
---
 include/linux/fs.h |   44 ++++++++++++++++++++++----------------------
 1 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2620a8c..4e23c70 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1553,30 +1553,30 @@ extern ssize_t vfs_writev(struct file *, const struct iovec __user *,
 		unsigned long, loff_t *);
 
 struct super_operations {
-   	struct inode *(*alloc_inode)(struct super_block *sb);
-	void (*destroy_inode)(struct inode *);
-
-   	void (*dirty_inode) (struct inode *);
-	int (*write_inode) (struct inode *, int);
-	void (*drop_inode) (struct inode *);
-	void (*delete_inode) (struct inode *);
-	void (*put_super) (struct super_block *);
-	void (*write_super) (struct super_block *);
-	int (*sync_fs)(struct super_block *sb, int wait);
-	int (*freeze_fs) (struct super_block *);
-	int (*unfreeze_fs) (struct super_block *);
-	int (*statfs) (struct dentry *, struct kstatfs *);
-	int (*remount_fs) (struct super_block *, int *, char *);
-	void (*clear_inode) (struct inode *);
-	void (*umount_begin) (struct super_block *);
-
-	int (*show_options)(struct seq_file *, struct vfsmount *);
-	int (*show_stats)(struct seq_file *, struct vfsmount *);
+   	struct inode *(* const alloc_inode)(struct super_block *sb);
+	void (* const destroy_inode)(struct inode *);
+
+   	void (* const dirty_inode) (struct inode *);
+	int (* const write_inode) (struct inode *, int);
+	void (* const drop_inode) (struct inode *);
+	void (* const delete_inode) (struct inode *);
+	void (* const put_super) (struct super_block *);
+	void (* const write_super) (struct super_block *);
+	int (* const sync_fs)(struct super_block *sb, int wait);
+	int (* const freeze_fs) (struct super_block *);
+	int (* const unfreeze_fs) (struct super_block *);
+	int (* const statfs) (struct dentry *, struct kstatfs *);
+	int (* const remount_fs) (struct super_block *, int *, char *);
+	void (* const clear_inode) (struct inode *);
+	void (* const umount_begin) (struct super_block *);
+
+	int (* const show_options)(struct seq_file *, struct vfsmount *);
+	int (* const show_stats)(struct seq_file *, struct vfsmount *);
 #ifdef CONFIG_QUOTA
-	ssize_t (*quota_read)(struct super_block *, int, char *, size_t, loff_t);
-	ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
+	ssize_t (* const quota_read)(struct super_block *, int, char *, size_t, loff_t);
+	ssize_t (* const quota_write)(struct super_block *, int, const char *, size_t, loff_t);
 #endif
-	int (*bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
+	int (* const bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
 };
 
 /*