[PATCH] autofs sparse fixes

[PATCH] autofs sparse fixes

[Andries.Brouwer]

diff -u --recursive --new-file -X /linux/dontdiff a/fs/autofs/root.c b/fs/autofs/root.c
--- a/fs/autofs/root.c	Mon Sep 29 01:05:41 2003
+++ b/fs/autofs/root.c	Mon Sep 29 01:08:23 2003
@@ -468,7 +468,7 @@
 
 /* Get/set timeout ioctl() operation */
 static inline int autofs_get_set_timeout(struct autofs_sb_info *sbi,
-					 unsigned long *p)
+					 unsigned long __user *p)
 {
 	unsigned long ntimeout;
 
@@ -494,7 +494,7 @@
 static inline int autofs_expire_run(struct super_block *sb,
 				    struct autofs_sb_info *sbi,
 				    struct vfsmount *mnt,
-				    struct autofs_packet_expire *pkt_p)
+				    struct autofs_packet_expire __user *pkt_p)
 {
 	struct autofs_dir_ent *ent;
 	struct autofs_packet_expire pkt;
@@ -547,10 +547,10 @@
 	case AUTOFS_IOC_PROTOVER: /* Get protocol version */
 		return autofs_get_protover((int *)arg);
 	case AUTOFS_IOC_SETTIMEOUT:
-		return autofs_get_set_timeout(sbi,(unsigned long *)arg);
+		return autofs_get_set_timeout(sbi,(unsigned long __user *)arg);
 	case AUTOFS_IOC_EXPIRE:
 		return autofs_expire_run(inode->i_sb, sbi, filp->f_vfsmnt,
-					 (struct autofs_packet_expire *)arg);
+				 (struct autofs_packet_expire __user *)arg);
 	default:
 		return -ENOSYS;
 	}
diff -u --recursive --new-file -X /linux/dontdiff a/fs/autofs/symlink.c b/fs/autofs/symlink.c
--- a/fs/autofs/symlink.c	Mon Sep 29 01:05:41 2003
+++ b/fs/autofs/symlink.c	Mon Sep 29 01:08:23 2003
@@ -12,7 +12,8 @@
 
 #include "autofs_i.h"
 
-static int autofs_readlink(struct dentry *dentry, char *buffer, int buflen)
+static int
+autofs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
 {
 	char *s=((struct autofs_symlink *)dentry->d_inode->u.generic_ip)->data;
 	return vfs_readlink(dentry, buffer, buflen, s);
diff -u --recursive --new-file -X /linux/dontdiff a/fs/autofs/waitq.c b/fs/autofs/waitq.c
--- a/fs/autofs/waitq.c	Mon Sep 29 01:05:41 2003
+++ b/fs/autofs/waitq.c	Mon Sep 29 01:08:23 2003
@@ -44,12 +44,16 @@
 	autofs_hash_dputall(&sbi->dirhash); /* Remove all dentry pointers */
 }
 
+/*
+ * Note: addr not in user space
+ */
 static int autofs_write(struct file *file, const void *addr, int bytes)
 {
 	unsigned long sigpipe, flags;
 	mm_segment_t fs;
 	const char *data = (const char *)addr;
 	ssize_t wr = 0;
+	ssize_t (*write)(struct file *, const char *, size_t, loff_t *);
 
 	/** WARNING: this is not safe for writing more than PIPE_BUF bytes! **/
 
@@ -59,8 +63,12 @@
 	fs = get_fs();
 	set_fs(KERNEL_DS);
 
+	/* Our write does not write to user space */
+	write = (ssize_t (*)(struct file *, const char *, size_t, loff_t *))
+		file->f_op->write;
+
 	while (bytes &&
-	       (wr = file->f_op->write(file,data,bytes,&file->f_pos)) > 0) {
+	       (wr = write(file,data,bytes,&file->f_pos)) > 0) {
 		data += wr;
 		bytes -= wr;
 	}

Re: [PATCH] autofs sparse fixes

[Linus Torvalds]

For cases like this, where we use a kernel pointer and do the 
"set_fs(KERNEL_DS)" thing to mak ea user-pointer routing write to kernel 
space, I suggest casting the pointer instead of the function, along with a 
comment on the "set_fs()".

Basically, something like this instead:

  static int autofs_write(struct file *file, const void *addr, int bytes)
  {
	const void __user *data;
	...

	set_fs(KERNEL_DS);
	/* This cast is legal due to the set_fs()! */
	data = (const void __user *) addr;

	...->write(file, data, bytes);
	set_fs(old_fs);
  }

See? That makes the cast more obvious, and it also makes it obvious _why_ 
the cast from kernel->user pointer is ok in this case.

		Linus

Re: [PATCH] autofs sparse fixes

[Linus Torvalds]

On Mon, 29 Sep 2003 Andries.Brouwer@cwi.nl wrote:
> 
> I think I prefer two rights above two wrongs - the declaration
> already gives addr the right type - it really does point to kernel space -
> but we inherit an incorrect type for file->f_op->write, so have to cast
> that.

No, the type for file->f_op->write is of the _rigth_ type.

It literally _does_ get the data from user space.

The thing is, what "set_fs(KERNEL_DS)" does is to say "kernel space is now 
user space". So the _caller_ will have made user space and kernel space be 
the same mapping, exactly so that the write will do the right thing.

[ Yeah, we long since renamed all the "get_fs_byte()" calls to a much more
  natural "get_user()" macro, and the "set_fs()" thing should be renamed
  too. It obviously makes no sense any more, since we don't use the '%fs'
  segment register even on x86 these days, and on other architectures it
  never made any sense in the first place. ]

So as a result of the "set_fs()" the kernel pointer literally _becomes_ a 
user-pointer. That's what set_fs() is all about. And that's why it is 
correct to cast the pointer - but not the function. The function still 
takes a user pointer.

			Linus

Re: [PATCH] autofs sparse fixes

[Andries.Brouwer]

aeb:

        /* Our write does not write to user space */
        write = (ssize_t (*)(struct file *, const char *, size_t, loff_t *))
                file->f_op->write;

lbt:

	For cases like this, where we use a kernel pointer and do the 
	"set_fs(KERNEL_DS)" thing to make a user-pointer routine write
	to kernel space, I suggest casting the pointer instead of
	the function, along with a comment on the "set_fs()".

		set_fs(KERNEL_DS);

		/* This cast is legal due to the set_fs()! */
		data = (const void __user *) addr;

		...->write(file, data, bytes);
		set_fs(old_fs);

	See? That makes the cast more obvious, and it also makes it obvious
	_why_ the cast from kernel->user pointer is ok in this case.

Hmm. Yes. Hmm.
I have to admit that my cast is complicated, one might even say messy,
but as always - when something seems messy it can be made to look
less so by using more lines of code. E.g.,

	write = (write_to_kernel_t) file->f_op->write;

looks less intimidating, and write_to_kernel_t can be defined next to
write_proc_t that we have already.

I think I prefer two rights above two wrongs - the declaration
already gives addr the right type - it really does point to kernel space -
but we inherit an incorrect type for file->f_op->write, so have to cast
that.

Hmm. In reality maybe I have no strong feelings either way.
Will see what you do and possibly send some other version
of what you did not apply.

Andries


[Soon things get more interesting. These were the trivial cases,
with set_fs nearby and to a constant value. In other words, cases
where we know precisely what happens and only have to add a cast.
Intermezzo has a lot of code where the same code may be executed
with kernel and with user pointers.
Sendfile can use kernel or user pointers. It is declared with __user
so requires

        retval = in_file->f_op->sendfile(in_file, ppos, count, file_send_actor,
                                         (void __user *) out_file);

(fs/read_write.c:585 -- two wrongs indeed) where the type of pointer
is implicit in the actor.  Struct tty_operations has the field
	int  (*write)(struct tty_struct *tty, int from_user,
		      const unsigned char *buf, int count);
Etc. Lots of places where static markup does not suffice to show
which pointers point to user space. Pity. That diminishes the value
of __user markup greatly.]