[PATCH 0/4] initramfs: remove sparse warnings

[PATCH 0/4] initramfs: remove sparse warnings

[Namhyung Kim]

Hello,

This patchset removes most of sparse warnings in init/initramfs.c.
Current implementation of initramfs relies on syscall service rountins heavily
so it requires many of arguments to be __user address space pointers but, in
most cases, were missing proper markups. This patchset tries to fix those at
a minimum change.

Namhyung Kim (4):
  initramfs: refactor clean_path()
  initramfs: mark dirp as a __user pointer on clean_rootfs()
  initramfs: mark collect buffers as __user pointers
  initramfs: add missing __user markup

 init/initramfs.c |   69 ++++++++++++++++++++++++++++++-----------------------
 1 files changed, 39 insertions(+), 30 deletions(-)

[PATCH 1/4] initramfs: refactor clean_path()

[Namhyung Kim]

first param, path, is used only on calling syscall routines which means it is
always considered as a user pointer. So add __user markup on parameter
declaration. And &st should be a user pointer also, mark it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 init/initramfs.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index d9c6e78..75a43c5 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -276,11 +276,12 @@ static int __init maybe_link(void)
 	return 0;
 }
 
-static void __init clean_path(char *path, mode_t mode)
+static void __init clean_path(char __user *path, mode_t mode)
 {
 	struct stat st;
 
-	if (!sys_newlstat(path, &st) && (st.st_mode^mode) & S_IFMT) {
+	if (!sys_newlstat(path, (struct stat __user __force *)&st) &&
+	    (st.st_mode^mode) & S_IFMT) {
 		if (S_ISDIR(st.st_mode))
 			sys_rmdir(path);
 		else
-- 
1.7.0.4

[PATCH 2/4] initramfs: mark dirp as a __user pointer on clean_rootfs()

[Namhyung Kim]

dirp is used for calling syscall routines so it would be better to be a user
pointer unless we want to cast it on every syscalls.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 init/initramfs.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 75a43c5..d90d1cf 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -526,7 +526,7 @@ static void __init clean_rootfs(void)
 {
 	int fd;
 	void *buf;
-	struct linux_dirent64 *dirp;
+	struct linux_dirent64 __user *dirp;
 	int num;
 
 	fd = sys_open((const char __user __force *) "/", O_RDONLY, 0);
@@ -540,14 +540,16 @@ static void __init clean_rootfs(void)
 		return;
 	}
 
-	dirp = buf;
+	dirp = (struct linux_dirent64 __user __force *) buf;
 	num = sys_getdents64(fd, dirp, BUF_SIZE);
 	while (num > 0) {
 		while (num > 0) {
 			struct stat st;
 			int ret;
+			unsigned short len;
 
-			ret = sys_newlstat(dirp->d_name, &st);
+			ret = sys_newlstat(dirp->d_name,
+					   (struct stat __user __force *)&st);
 			WARN_ON_ONCE(ret);
 			if (!ret) {
 				if (S_ISDIR(st.st_mode))
@@ -556,10 +558,11 @@ static void __init clean_rootfs(void)
 					sys_unlink(dirp->d_name);
 			}
 
-			num -= dirp->d_reclen;
-			dirp = (void *)dirp + dirp->d_reclen;
+			len = ((struct linux_dirent64 __force *) dirp)->d_reclen;
+			num -= len;
+			dirp = (void __user *)dirp + len;
 		}
-		dirp = buf;
+		dirp = (struct linux_dirent64 __user __force *) buf;
 		memset(buf, 0, BUF_SIZE);
 		num = sys_getdents64(fd, dirp, BUF_SIZE);
 	}
-- 
1.7.0.4

[PATCH 3/4] initramfs: mark collect buffers as __user pointers

[Namhyung Kim]

collected, vcollected, collect are used for syscall routines so it would be
better to be __user pointers. We can add __user/__force markup on every calls
but marking them as user pointers requires less change.

Impact: remove sparse warnings, no functional change
Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 init/initramfs.c |   45 +++++++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index d90d1cf..2aa8c96 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -174,19 +174,19 @@ static inline void __init eat(unsigned n)
 	count -= n;
 }
 
-static __initdata char *vcollected;
-static __initdata char *collected;
+static __initdata char __user *vcollected;
+static __initdata char __user *collected;
 static __initdata int remains;
-static __initdata char *collect;
+static __initdata char __user *collect;
 
 static void __init read_into(char *buf, unsigned size, enum state next)
 {
 	if (count >= size) {
-		collected = victim;
+		collected = (char __user __force *) victim;
 		eat(size);
 		state = next;
 	} else {
-		collect = collected = buf;
+		collect = collected = (char __user __force *) buf;
 		remains = size;
 		next_state = next;
 		state = Collect;
@@ -206,7 +206,7 @@ static int __init do_collect(void)
 	unsigned n = remains;
 	if (count < n)
 		n = count;
-	memcpy(collect, victim, n);
+	copy_to_user(collect, victim, n);
 	eat(n);
 	collect += n;
 	if ((remains -= n) != 0)
@@ -217,15 +217,15 @@ static int __init do_collect(void)
 
 static int __init do_header(void)
 {
-	if (memcmp(collected, "070707", 6)==0) {
+	if (memcmp((char __force *)collected, "070707", 6)==0) {
 		error("incorrect cpio method used: use -H newc option");
 		return 1;
 	}
-	if (memcmp(collected, "070701", 6)) {
+	if (memcmp((char __force *)collected, "070701", 6)) {
 		error("no cpio magic");
 		return 1;
 	}
-	parse_header(collected);
+	parse_header((char __force *)collected);
 	next_header = this_header + N_ALIGN(name_len) + body_len;
 	next_header = (next_header + 3) & ~3;
 	state = SkipIt;
@@ -234,7 +234,7 @@ static int __init do_header(void)
 	if (S_ISLNK(mode)) {
 		if (body_len > PATH_MAX)
 			return 0;
-		collect = collected = symlink_buf;
+		collect = collected = (char __user __force *) symlink_buf;
 		remains = N_ALIGN(name_len) + body_len;
 		next_state = GotSymlink;
 		state = Collect;
@@ -269,9 +269,12 @@ static int __init do_reset(void)
 static int __init maybe_link(void)
 {
 	if (nlink >= 2) {
-		char *old = find_link(major, minor, ino, mode, collected);
-		if (old)
-			return (sys_link(old, collected) < 0) ? -1 : 1;
+		char *old = find_link(major, minor, ino, mode,
+				      (char __force *) collected);
+		if (old) {
+			char __user *uold = (char __user __force *) old;
+			return (sys_link(uold, collected) < 0) ? -1 : 1;
+		}
 	}
 	return 0;
 }
@@ -295,7 +298,7 @@ static int __init do_name(void)
 {
 	state = SkipIt;
 	next_state = Reset;
-	if (strcmp(collected, "TRAILER!!!") == 0) {
+	if (strcmp((char __force *)collected, "TRAILER!!!") == 0) {
 		free_hash();
 		return 0;
 	}
@@ -313,7 +316,9 @@ static int __init do_name(void)
 				sys_fchmod(wfd, mode);
 				if (body_len)
 					sys_ftruncate(wfd, body_len);
-				vcollected = kstrdup(collected, GFP_KERNEL);
+				vcollected = (char __user __force *)
+					kstrdup((char __force *)collected,
+						GFP_KERNEL);
 				state = CopyFile;
 			}
 		}
@@ -321,7 +326,7 @@ static int __init do_name(void)
 		sys_mkdir(collected, mode);
 		sys_chown(collected, uid, gid);
 		sys_chmod(collected, mode);
-		dir_add(collected, mtime);
+		dir_add((char __force *)collected, mtime);
 	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
 		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
 		if (maybe_link() == 0) {
@@ -337,15 +342,15 @@ static int __init do_name(void)
 static int __init do_copy(void)
 {
 	if (count >= body_len) {
-		sys_write(wfd, victim, body_len);
+		sys_write(wfd, (char __user __force *)victim, body_len);
 		sys_close(wfd);
 		do_utime(vcollected, mtime);
-		kfree(vcollected);
+		kfree((void __force *)vcollected);
 		eat(body_len);
 		state = SkipIt;
 		return 0;
 	} else {
-		sys_write(wfd, victim, count);
+		sys_write(wfd, (char __user __force *)victim, count);
 		body_len -= count;
 		eat(count);
 		return 1;
@@ -354,7 +359,7 @@ static int __init do_copy(void)
 
 static int __init do_symlink(void)
 {
-	collected[N_ALIGN(name_len) + body_len] = '\0';
+	put_user('\0', collected + N_ALIGN(name_len) + body_len);
 	clean_path(collected, 0);
 	sys_symlink(collected + N_ALIGN(name_len), collected);
 	sys_lchown(collected, uid, gid);
-- 
1.7.0.4

[PATCH 4/4] initramfs: add missing __user markup

[Namhyung Kim]

do_utime(), sys_write() require their arg to be a user pointer but were
missing __user markups. Add it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 init/initramfs.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 2aa8c96..993ebab 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -109,7 +109,7 @@ static void __init dir_utime(void)
 	struct dir_entry *de, *tmp;
 	list_for_each_entry_safe(de, tmp, &dir_list, list) {
 		list_del(&de->list);
-		do_utime(de->name, de->mtime);
+		do_utime((char __user __force *)de->name, de->mtime);
 		kfree(de->name);
 		kfree(de);
 	}
@@ -602,7 +602,7 @@ static int __init populate_rootfs(void)
 		fd = sys_open((const char __user __force *) "/initrd.image",
 			      O_WRONLY|O_CREAT, 0700);
 		if (fd >= 0) {
-			sys_write(fd, (char *)initrd_start,
+			sys_write(fd, (char __user *)initrd_start,
 					initrd_end - initrd_start);
 			sys_close(fd);
 			free_initrd();
-- 
1.7.0.4

Re: [PATCH 0/4] initramfs: remove sparse warnings

[Arnd Bergmann]

On Thursday 19 August 2010, Namhyung Kim wrote:
> This patchset removes most of sparse warnings in init/initramfs.c.
> Current implementation of initramfs relies on syscall service rountins heavily
> so it requires many of arguments to be __user address space pointers but, in
> most cases, were missing proper markups. This patchset tries to fix those at
> a minimum change.

I'm skeptical about this, you are adding obviously incorrect annotations
to the code to make something work that was written without the awareness
for address spaces.

A better way would be to call path_lookup or kern_path to look up the
path and pass that to a lower-level file I/O function.

	Arnd

Re: [PATCH 0/4] initramfs: remove sparse warnings

[Namhyung Kim]

2010-08-19 (목), 16:38 +0200, Arnd Bergmann:
> On Thursday 19 August 2010, Namhyung Kim wrote:
> > This patchset removes most of sparse warnings in init/initramfs.c.
> > Current implementation of initramfs relies on syscall service rountins heavily
> > so it requires many of arguments to be __user address space pointers but, in
> > most cases, were missing proper markups. This patchset tries to fix those at
> > a minimum change.
> 
> I'm skeptical about this, you are adding obviously incorrect annotations
> to the code to make something work that was written without the awareness
> for address spaces.
> 
> A better way would be to call path_lookup or kern_path to look up the
> path and pass that to a lower-level file I/O function.
> 
> 	Arnd

Thanks for the comment. Then I'll try to refactor/reimplement initramfs
code based on your suggestion.

-- 
Regards,
Namhyung Kim

Re: [PATCH 0/4] initramfs: remove sparse warnings

[Al Viro]

On Thu, Aug 19, 2010 at 04:38:23PM +0200, Arnd Bergmann wrote:
> On Thursday 19 August 2010, Namhyung Kim wrote:
> > This patchset removes most of sparse warnings in init/initramfs.c.
> > Current implementation of initramfs relies on syscall service rountins heavily
> > so it requires many of arguments to be __user address space pointers but, in
> > most cases, were missing proper markups. This patchset tries to fix those at
> > a minimum change.
> 
> I'm skeptical about this, you are adding obviously incorrect annotations
> to the code to make something work that was written without the awareness
> for address spaces.
> 
> A better way would be to call path_lookup or kern_path to look up the
> path and pass that to a lower-level file I/O function.

No.  This code should *NOT* use the VFS guts, TYVM.  The whole fscking point
is that this puppy is a sequence of plain vanilla syscalls, ideally run
simply in userland thread.  We used to have a magical mystery shite in there
and it had been a huge PITA.

Re: [PATCH 0/4] initramfs: remove sparse warnings

[Namhyung Kim]

2010-08-20 (금), 13:00 +0100, Al Viro:
> No.  This code should *NOT* use the VFS guts, TYVM.  The whole fscking point
> is that this puppy is a sequence of plain vanilla syscalls, ideally run
> simply in userland thread.  We used to have a magical mystery shite in there
> and it had been a huge PITA.

So is it worth to work on removing the warnings like this patchset does?
Or else, how can I improve the code even a bit? Can you please give me a
direction?

-- 
Regards,
Namhyung Kim

Re: [PATCH 0/4] initramfs: remove sparse warnings

[Arnd Bergmann]

On Friday 20 August 2010 17:36:41 Namhyung Kim wrote:
> 
> 2010-08-20 (금), 13:00 +0100, Al Viro:
> > No.  This code should NOT use the VFS guts, TYVM.  The whole fscking point
> > is that this puppy is a sequence of plain vanilla syscalls, ideally run
> > simply in userland thread.  We used to have a magical mystery shite in there
> > and it had been a huge PITA.
> 
> So is it worth to work on removing the warnings like this patchset does?
> Or else, how can I improve the code even a bit? Can you please give me a
> direction?

It would be useful to add annotations in those places where they are
obviously just missing but don't require adding __force.
Even better would be patches that fix actual bugs found by sparse.

Simply throwing in extra __force arguments will just make people
nervous, because it is often a sign of papering over a bug instead
of fixing it, and warnings in the core kernel are there exactly
because there is no easy correct fix for them.

Try producing patches that clean up the code and result in using
fewer annotations and especially few __force where possible.
Also, in places where you need __force, make sure that a person
reading that code understands why it's needed and that the use is
correct (or at least permissable).

One possible solution in this particular case would be to add
helper functions like 

/* wrapper for sys_newlstat for use in the init code */
static inline int kern_newlstat(const char * filename, struct stat * statbuf)
{
	mm_segment_t fs = get_fs();
	int ret;

	set_fs(KERNEL_DS);
	ret = sys_newlstat((const char __user __force*)filename,
			   (struct stat __user __force *)statbuf);
	set_fs(fs);

	return ret;
}

Such a function makes it clear that it can only accept a kernel pointer,
and it documents how the conversion to a __user pointer happens (by
calling set_fs).
Then again, it adds some bloat, and it may encourage people to do the
same thing in device drivers, which could be argued against such helpers.

In general, my recommendation would be to leave code alone if you can't
come up with a patch that clearly improves it. There is enough
bad code in the kernel that can use some help along the lines of
your other patches, so you may want to focus on that.

	Arnd

Re: [PATCH 0/4] initramfs: remove sparse warnings

[Namhyung Kim]

2010-08-22 (일), 22:33 +0200, Arnd Bergmann:
> On Friday 20 August 2010 17:36:41 Namhyung Kim wrote:
> > 
> > 2010-08-20 (금), 13:00 +0100, Al Viro:
> > > No.  This code should NOT use the VFS guts, TYVM.  The whole fscking point
> > > is that this puppy is a sequence of plain vanilla syscalls, ideally run
> > > simply in userland thread.  We used to have a magical mystery shite in there
> > > and it had been a huge PITA.
> > 
> > So is it worth to work on removing the warnings like this patchset does?
> > Or else, how can I improve the code even a bit? Can you please give me a
> > direction?
> 
> It would be useful to add annotations in those places where they are
> obviously just missing but don't require adding __force.
> Even better would be patches that fix actual bugs found by sparse.
> 
> Simply throwing in extra __force arguments will just make people
> nervous, because it is often a sign of papering over a bug instead
> of fixing it, and warnings in the core kernel are there exactly
> because there is no easy correct fix for them.
> 
> Try producing patches that clean up the code and result in using
> fewer annotations and especially few __force where possible.
> Also, in places where you need __force, make sure that a person
> reading that code understands why it's needed and that the use is
> correct (or at least permissable).
> 
> One possible solution in this particular case would be to add
> helper functions like 
> 
> /* wrapper for sys_newlstat for use in the init code */
> static inline int kern_newlstat(const char * filename, struct stat * statbuf)
> {
> 	mm_segment_t fs = get_fs();
> 	int ret;
> 
> 	set_fs(KERNEL_DS);
> 	ret = sys_newlstat((const char __user __force*)filename,
> 			   (struct stat __user __force *)statbuf);
> 	set_fs(fs);
> 
> 	return ret;
> }
> 
> Such a function makes it clear that it can only accept a kernel pointer,
> and it documents how the conversion to a __user pointer happens (by
> calling set_fs).
> Then again, it adds some bloat, and it may encourage people to do the
> same thing in device drivers, which could be argued against such helpers.
> 
> In general, my recommendation would be to leave code alone if you can't
> come up with a patch that clearly improves it. There is enough
> bad code in the kernel that can use some help along the lines of
> your other patches, so you may want to focus on that.
> 
> 	Arnd

Thank you Arnd for the precious comments and advices. This really helps
me.

-- 
Regards,
Namhyung Kim