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;
}
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
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.]
On Mon, 29 Sep 2003 [email protected] 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