Hostfs updates

Hostfs updates

[Richard Weinberger]

Host got a visit from the janitor.

[PATCH 01/15] hostfs: hostfs_file_open: Switch to data locking model
[PATCH 02/15] hostfs: hostfs_file_open: Fix a fd leak in
[PATCH 03/15] hostfs: Allow fsync on directories
[PATCH 04/15] hostfs: Handle bogus st.mode
[PATCH 05/15] hostfs: Make hostfs_readpage more readable
[PATCH 06/15] hostfs: Add a BUG_ON to detect behavior changes of
[PATCH 07/15] hostfs: Remove open coded strcpy()
[PATCH 08/15] hostfs: Use __getname() in follow_link
[PATCH 09/15] hostfs: Report append flag in ->show_options()
[PATCH 10/15] hostfs: Remove superfluous test in hostfs_open()
[PATCH 11/15] hostfs: hostfs_open: Reset open flags upon each retry
[PATCH 12/15] hostfs: Remove superfluous initializations in
[PATCH 13/15] hostfs: Set page flags in hostfs_readpage() correctly
[PATCH 14/15] hostfs: Use page_offset()
[PATCH 15/15] hostfs: No need to box and later unbox the file mode

Thanks,
//richard

[PATCH 01/15] hostfs: hostfs_file_open: Switch to data locking model

[Richard Weinberger]

Instead of serializing hostfs_file_open() we can use
a per inode mutex to protect ->mode.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index fd62cae..104d58d 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -24,6 +24,7 @@ struct hostfs_inode_info {
 	int fd;
 	fmode_t mode;
 	struct inode vfs_inode;
+	struct mutex open_mutex;
 };
 
 static inline struct hostfs_inode_info *HOSTFS_I(struct inode *inode)
@@ -225,6 +226,7 @@ static struct inode *hostfs_alloc_inode(struct super_block *sb)
 	hi->fd = -1;
 	hi->mode = 0;
 	inode_init_once(&hi->vfs_inode);
+	mutex_init(&hi->open_mutex);
 	return &hi->vfs_inode;
 }
 
@@ -295,7 +297,6 @@ static int hostfs_readdir(struct file *file, struct dir_context *ctx)
 
 static int hostfs_file_open(struct inode *ino, struct file *file)
 {
-	static DEFINE_MUTEX(open_mutex);
 	char *name;
 	fmode_t mode = 0;
 	int err;
@@ -324,15 +325,15 @@ retry:
 	if (fd < 0)
 		return fd;
 
-	mutex_lock(&open_mutex);
+	mutex_lock(&HOSTFS_I(ino)->open_mutex);
 	/* somebody else had handled it first? */
 	if ((mode & HOSTFS_I(ino)->mode) == mode) {
-		mutex_unlock(&open_mutex);
+		mutex_unlock(&HOSTFS_I(ino)->open_mutex);
 		return 0;
 	}
 	if ((mode | HOSTFS_I(ino)->mode) != mode) {
 		mode |= HOSTFS_I(ino)->mode;
-		mutex_unlock(&open_mutex);
+		mutex_unlock(&HOSTFS_I(ino)->open_mutex);
 		close_file(&fd);
 		goto retry;
 	}
@@ -342,12 +343,12 @@ retry:
 		err = replace_file(fd, HOSTFS_I(ino)->fd);
 		close_file(&fd);
 		if (err < 0) {
-			mutex_unlock(&open_mutex);
+			mutex_unlock(&HOSTFS_I(ino)->open_mutex);
 			return err;
 		}
 	}
 	HOSTFS_I(ino)->mode = mode;
-	mutex_unlock(&open_mutex);
+	mutex_unlock(&HOSTFS_I(ino)->open_mutex);
 
 	return 0;
 }
-- 
2.3.2

[PATCH 02/15] hostfs: hostfs_file_open: Fix a fd leak in hostfs_file_open

[Richard Weinberger]

In case of a race between to callers of hostfs_file_open()
it can happen that a file describtor is leaked.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 104d58d..112ba5a 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -329,6 +329,7 @@ retry:
 	/* somebody else had handled it first? */
 	if ((mode & HOSTFS_I(ino)->mode) == mode) {
 		mutex_unlock(&HOSTFS_I(ino)->open_mutex);
+		close_file(&fd);
 		return 0;
 	}
 	if ((mode | HOSTFS_I(ino)->mode) != mode) {
-- 
2.3.2

[PATCH 03/15] hostfs: Allow fsync on directories

[Richard Weinberger]

Historically hostfs did not open directories on the host filesystem
for performance and memory reasons.
But it turned out that this optimization has a drawback.
Calling fsync() on a hostfs directory returns immediately
with -EINVAL as fsync is not implemented.
While this is behavior is strictly speaking correct common userspace
like dpkg(1) stumbles over that and makes it impossible to use
hostfs as root filesystem.
The fix is easy, wire up the existing host open/fsync functions
to the directory file operations.

Reported-by: Daniel Gröber <dxld@darkboxed.org>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 112ba5a..92b008f 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -295,7 +295,7 @@ static int hostfs_readdir(struct file *file, struct dir_context *ctx)
 	return 0;
 }
 
-static int hostfs_file_open(struct inode *ino, struct file *file)
+static int hostfs_open(struct inode *ino, struct file *file)
 {
 	char *name;
 	fmode_t mode = 0;
@@ -386,7 +386,7 @@ static const struct file_operations hostfs_file_fops = {
 	.write_iter	= generic_file_write_iter,
 	.write		= new_sync_write,
 	.mmap		= generic_file_mmap,
-	.open		= hostfs_file_open,
+	.open		= hostfs_open,
 	.release	= hostfs_file_release,
 	.fsync		= hostfs_fsync,
 };
@@ -395,6 +395,8 @@ static const struct file_operations hostfs_dir_fops = {
 	.llseek		= generic_file_llseek,
 	.iterate	= hostfs_readdir,
 	.read		= generic_read_dir,
+	.open		= hostfs_open,
+	.fsync		= hostfs_fsync,
 };
 
 static int hostfs_writepage(struct page *page, struct writeback_control *wbc)
-- 
2.3.2

[PATCH 04/15] hostfs: Handle bogus st.mode

[Richard Weinberger]

Make sure that we return EIO if one passes an invalid st.mode
into hostfs.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 92b008f..8163aac 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -534,11 +534,13 @@ static int read_name(struct inode *ino, char *name)
 		init_special_inode(ino, st.mode & S_IFMT, rdev);
 		ino->i_op = &hostfs_iops;
 		break;
-
-	default:
+	case S_IFREG:
 		ino->i_op = &hostfs_iops;
 		ino->i_fop = &hostfs_file_fops;
 		ino->i_mapping->a_ops = &hostfs_aops;
+		break;
+	default:
+		return -EIO;
 	}
 
 	ino->i_ino = st.ino;
-- 
2.3.2

[PATCH 05/15] hostfs: Make hostfs_readpage more readable

[Richard Weinberger]

...to make life easier for future readers of that code.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 8163aac..67e556c 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -439,25 +439,27 @@ static int hostfs_readpage(struct file *file, struct page *page)
 {
 	char *buffer;
 	long long start;
-	int err = 0;
+	int bytes_read, ret;
 
 	start = (long long) page->index << PAGE_CACHE_SHIFT;
 	buffer = kmap(page);
-	err = read_file(FILE_HOSTFS_I(file)->fd, &start, buffer,
+	bytes_read = read_file(FILE_HOSTFS_I(file)->fd, &start, buffer,
 			PAGE_CACHE_SIZE);
-	if (err < 0)
+	if (bytes_read < 0) {
+		ret = bytes_read;
 		goto out;
+	}
 
-	memset(&buffer[err], 0, PAGE_CACHE_SIZE - err);
+	memset(buffer + bytes_read, 0, PAGE_CACHE_SIZE - bytes_read);
 
 	flush_dcache_page(page);
 	SetPageUptodate(page);
 	if (PageError(page)) ClearPageError(page);
-	err = 0;
+	ret = 0;
  out:
 	kunmap(page);
 	unlock_page(page);
-	return err;
+	return ret;
 }
 
 static int hostfs_write_begin(struct file *file, struct address_space *mapping,
-- 
2.3.2

[PATCH 06/15] hostfs: Add a BUG_ON to detect behavior changes of dentry_path_raw()

[Richard Weinberger]

hostfs' __dentry_name() relies on the fact that dentry_path_raw() will place
the path name at the end of the provided buffer.
While this is okay, add a BUG_ON() to detect behavior changes as soon
as possible.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 67e556c..3082a7e 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -93,6 +93,13 @@ static char *__dentry_name(struct dentry *dentry, char *name)
 		__putname(name);
 		return NULL;
 	}
+
+	/*
+	 * This function relies on the fact that dentry_path_raw() will place
+	 * the path name at the end of the provided buffer.
+	 */
+	BUG_ON(p + strlen(p) + 1 != name + PATH_MAX);
+
 	strlcpy(name, root, PATH_MAX);
 	if (len > p - name) {
 		__putname(name);
-- 
2.3.2

[PATCH 07/15] hostfs: Remove open coded strcpy()

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 3082a7e..7260f16 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -105,11 +105,10 @@ static char *__dentry_name(struct dentry *dentry, char *name)
 		__putname(name);
 		return NULL;
 	}
-	if (p > name + len) {
-		char *s = name + len;
-		while ((*s++ = *p++) != '\0')
-			;
-	}
+
+	if (p > name + len)
+		strcpy(name + len, p);
+
 	return name;
 }
 
-- 
2.3.2

[PATCH 08/15] hostfs: Use __getname() in follow_link

[Richard Weinberger]

Be consistent with all other functions in hostfs and just
use __getname().

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 7260f16..c60d886 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -142,21 +142,19 @@ static char *follow_link(char *link)
 	int len, n;
 	char *name, *resolved, *end;
 
-	len = 64;
-	while (1) {
+	name = __getname();
+	if (!name) {
 		n = -ENOMEM;
-		name = kmalloc(len, GFP_KERNEL);
-		if (name == NULL)
-			goto out;
-
-		n = hostfs_do_readlink(link, name, len);
-		if (n < len)
-			break;
-		len *= 2;
-		kfree(name);
+		goto out_free;
 	}
+
+	n = hostfs_do_readlink(link, name, PATH_MAX);
 	if (n < 0)
 		goto out_free;
+	else if (n == PATH_MAX) {
+		n = -E2BIG;
+		goto out_free;
+	}
 
 	if (*name == '/')
 		return name;
@@ -175,13 +173,12 @@ static char *follow_link(char *link)
 	}
 
 	sprintf(resolved, "%s%s", link, name);
-	kfree(name);
+	__putname(name);
 	kfree(link);
 	return resolved;
 
  out_free:
-	kfree(name);
- out:
+	__putname(name);
 	return ERR_PTR(n);
 }
 
-- 
2.3.2

[PATCH 09/15] hostfs: Report append flag in ->show_options()

[Richard Weinberger]

hostfs has an "append" mount option. Report it.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index c60d886..06b3e3f 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -262,6 +262,9 @@ static int hostfs_show_options(struct seq_file *seq, struct dentry *root)
 	if (strlen(root_path) > offset)
 		seq_printf(seq, ",%s", root_path + offset);
 
+	if (append)
+		seq_puts(seq, ",append");
+
 	return 0;
 }
 
-- 
2.3.2

[PATCH 10/15] hostfs: Remove superfluous test in hostfs_open()

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 06b3e3f..8bbceae 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -318,9 +318,7 @@ retry:
 	if (mode & FMODE_READ)
 		r = 1;
 	if (mode & FMODE_WRITE)
-		w = 1;
-	if (w)
-		r = 1;
+		r = w = 1;
 
 	name = dentry_name(file->f_path.dentry);
 	if (name == NULL)
-- 
2.3.2

[PATCH 11/15] hostfs: hostfs_open: Reset open flags upon each retry

[Richard Weinberger]

...otherwise we might end up with an incorrect mode mode.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 8bbceae..80ced3d 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -315,6 +315,8 @@ static int hostfs_open(struct inode *ino, struct file *file)
 	mode |= HOSTFS_I(ino)->mode;
 
 retry:
+	r = w = 0;
+
 	if (mode & FMODE_READ)
 		r = 1;
 	if (mode & FMODE_WRITE)
-- 
2.3.2

[PATCH 12/15] hostfs: Remove superfluous initializations in hostfs_open()

[Richard Weinberger]

Only initialize what we really need.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 80ced3d..cf80a30 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -304,9 +304,9 @@ static int hostfs_readdir(struct file *file, struct dir_context *ctx)
 static int hostfs_open(struct inode *ino, struct file *file)
 {
 	char *name;
-	fmode_t mode = 0;
+	fmode_t mode;
 	int err;
-	int r = 0, w = 0, fd;
+	int r, w, fd;
 
 	mode = file->f_mode & (FMODE_READ | FMODE_WRITE);
 	if ((mode & HOSTFS_I(ino)->mode) == mode)
-- 
2.3.2

[PATCH 13/15] hostfs: Set page flags in hostfs_readpage() correctly

[Richard Weinberger]

In case of an error set the page error flag and clear the up-to-date
flag.
If the read was successful clear the error flag unconditionally.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index cf80a30..f154747 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -445,24 +445,26 @@ static int hostfs_readpage(struct file *file, struct page *page)
 {
 	char *buffer;
 	long long start;
-	int bytes_read, ret;
+	int bytes_read, ret = 0;
 
 	start = (long long) page->index << PAGE_CACHE_SHIFT;
 	buffer = kmap(page);
 	bytes_read = read_file(FILE_HOSTFS_I(file)->fd, &start, buffer,
 			PAGE_CACHE_SIZE);
 	if (bytes_read < 0) {
+		ClearPageUptodate(page);
+		SetPageError(page);
 		ret = bytes_read;
 		goto out;
 	}
 
 	memset(buffer + bytes_read, 0, PAGE_CACHE_SIZE - bytes_read);
 
-	flush_dcache_page(page);
+	ClearPageError(page);
 	SetPageUptodate(page);
-	if (PageError(page)) ClearPageError(page);
-	ret = 0;
+
  out:
+	flush_dcache_page(page);
 	kunmap(page);
 	unlock_page(page);
 	return ret;
-- 
2.3.2

[PATCH 14/15] hostfs: Use page_offset()

[Richard Weinberger]

The kernel offers a helper function for that, use it.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs_kern.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index f154747..f82f98a5 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -410,7 +410,7 @@ static int hostfs_writepage(struct page *page, struct writeback_control *wbc)
 	struct address_space *mapping = page->mapping;
 	struct inode *inode = mapping->host;
 	char *buffer;
-	unsigned long long base;
+	loff_t base = page_offset(page);
 	int count = PAGE_CACHE_SIZE;
 	int end_index = inode->i_size >> PAGE_CACHE_SHIFT;
 	int err;
@@ -419,7 +419,6 @@ static int hostfs_writepage(struct page *page, struct writeback_control *wbc)
 		count = inode->i_size & (PAGE_CACHE_SIZE-1);
 
 	buffer = kmap(page);
-	base = ((unsigned long long) page->index) << PAGE_CACHE_SHIFT;
 
 	err = write_file(HOSTFS_I(inode)->fd, &base, buffer, count);
 	if (err != count) {
@@ -444,10 +443,9 @@ static int hostfs_writepage(struct page *page, struct writeback_control *wbc)
 static int hostfs_readpage(struct file *file, struct page *page)
 {
 	char *buffer;
-	long long start;
+	loff_t start = page_offset(page);
 	int bytes_read, ret = 0;
 
-	start = (long long) page->index << PAGE_CACHE_SHIFT;
 	buffer = kmap(page);
 	bytes_read = read_file(FILE_HOSTFS_I(file)->fd, &start, buffer,
 			PAGE_CACHE_SIZE);
-- 
2.3.2

[PATCH 15/15] hostfs: No need to box and later unbox the file mode

[Richard Weinberger]

There is really no point in having a function with 10
arguments.

Reported-by: Daniel Walter <d.walter@0x90.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/hostfs/hostfs.h      |  3 +--
 fs/hostfs/hostfs_kern.c |  5 +----
 fs/hostfs/hostfs_user.c | 17 +++--------------
 3 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
index 4fcd40d..493262e 100644
--- a/fs/hostfs/hostfs.h
+++ b/fs/hostfs/hostfs.h
@@ -77,8 +77,7 @@ extern int write_file(int fd, unsigned long long *offset, const char *buf,
 		      int len);
 extern int lseek_file(int fd, long long offset, int whence);
 extern int fsync_file(int fd, int datasync);
-extern int file_create(char *name, int ur, int uw, int ux, int gr,
-		       int gw, int gx, int or, int ow, int ox);
+extern int file_create(char *name, int mode);
 extern int set_attr(const char *file, struct hostfs_iattr *attrs, int fd);
 extern int make_symlink(const char *from, const char *to);
 extern int unlink_file(const char *file);
diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index f82f98a5..e77da44 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -582,10 +582,7 @@ static int hostfs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 	if (name == NULL)
 		goto out_put;
 
-	fd = file_create(name,
-			 mode & S_IRUSR, mode & S_IWUSR, mode & S_IXUSR,
-			 mode & S_IRGRP, mode & S_IWGRP, mode & S_IXGRP,
-			 mode & S_IROTH, mode & S_IWOTH, mode & S_IXOTH);
+	fd = file_create(name, mode & S_IFMT);
 	if (fd < 0)
 		error = fd;
 	else
diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
index 9765dab..34ab81b 100644
--- a/fs/hostfs/hostfs_user.c
+++ b/fs/hostfs/hostfs_user.c
@@ -175,21 +175,10 @@ void close_dir(void *stream)
 	closedir(stream);
 }
 
-int file_create(char *name, int ur, int uw, int ux, int gr,
-		int gw, int gx, int or, int ow, int ox)
+int file_create(char *name, int mode)
 {
-	int mode, fd;
-
-	mode = 0;
-	mode |= ur ? S_IRUSR : 0;
-	mode |= uw ? S_IWUSR : 0;
-	mode |= ux ? S_IXUSR : 0;
-	mode |= gr ? S_IRGRP : 0;
-	mode |= gw ? S_IWGRP : 0;
-	mode |= gx ? S_IXGRP : 0;
-	mode |= or ? S_IROTH : 0;
-	mode |= ow ? S_IWOTH : 0;
-	mode |= ox ? S_IXOTH : 0;
+	int fd;
+
 	fd = open64(name, O_CREAT | O_RDWR, mode);
 	if (fd < 0)
 		return -errno;
-- 
2.3.2

Re: [uml-devel] [PATCH 07/15] hostfs: Remove open coded strcpy()

[Geert Uytterhoeven]

On Mon, Mar 16, 2015 at 12:41 PM, Richard Weinberger <richard@nod.at> wrote:
> --- a/fs/hostfs/hostfs_kern.c
> +++ b/fs/hostfs/hostfs_kern.c
> @@ -105,11 +105,10 @@ static char *__dentry_name(struct dentry *dentry, char *name)

This code looks fishy to me...

First we have:

    len = strlen(root);
    strlcpy(name, root, PATH_MAX);

(I notice the code used strncpy() before. One difference with strlcpy()
 is that strncpy() fills the remaining of the destination buffer with zeroes.)

Then:

>                 __putname(name);
>                 return NULL;
>         }
> -       if (p > name + len) {
> -               char *s = name + len;

Unless strlcpy() truncated the string (which is unlikely, as root
cannot be longer
than PATH_MAX?), s = name + len now points to the zero terminator.
So the below would copy just one single byte:

> -               while ((*s++ = *p++) != '\0')
> -                       ;
> -       }
> +
> +       if (p > name + len)
> +               strcpy(name + len, p);
> +

What is this code really supposed to do?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [uml-devel] [PATCH 07/15] hostfs: Remove open coded strcpy()

[Richard Weinberger]

Am 16.03.2015 um 13:03 schrieb Geert Uytterhoeven:
> On Mon, Mar 16, 2015 at 12:41 PM, Richard Weinberger <richard@nod.at> wrote:
>> --- a/fs/hostfs/hostfs_kern.c
>> +++ b/fs/hostfs/hostfs_kern.c
>> @@ -105,11 +105,10 @@ static char *__dentry_name(struct dentry *dentry, char *name)
> 
> This code looks fishy to me...
> 
> First we have:
> 
>     len = strlen(root);
>     strlcpy(name, root, PATH_MAX);
> 
> (I notice the code used strncpy() before. One difference with strlcpy()
>  is that strncpy() fills the remaining of the destination buffer with zeroes.)
> 
> Then:
> 
>>                 __putname(name);
>>                 return NULL;
>>         }
>> -       if (p > name + len) {
>> -               char *s = name + len;
> 
> Unless strlcpy() truncated the string (which is unlikely, as root
> cannot be longer
> than PATH_MAX?), s = name + len now points to the zero terminator.
> So the below would copy just one single byte:
> 
>> -               while ((*s++ = *p++) != '\0')
>> -                       ;
>> -       }
>> +
>> +       if (p > name + len)
>> +               strcpy(name + len, p);
>> +
> 
> What is this code really supposed to do?

Hostfs' __dentry_name() builds the real path. i.e, the prefix on the host side
plus the requested path in UML.

"strlcpy(name, root, PATH_MAX);" copies the host prefix into name and then
the "strcpy(name + len, p);" copies the requested path into it.

The trick is that both share the same buffer, allocated by dentry_path_raw().
Therefore this bounds check works:
        if (len > p - name) {
                __putname(name);
                return NULL;
        }

Is it now clearer or did I miss something?
I agree that this code is tricky. :)

Thanks,
//richard

Re: [uml-devel] [PATCH 07/15] hostfs: Remove open coded strcpy()

[Geert Uytterhoeven]

Hi Richard,

On Mon, Mar 16, 2015 at 1:18 PM, Richard Weinberger <richard@nod.at> wrote:
> Am 16.03.2015 um 13:03 schrieb Geert Uytterhoeven:
>> On Mon, Mar 16, 2015 at 12:41 PM, Richard Weinberger <richard@nod.at> wrote:
>>> --- a/fs/hostfs/hostfs_kern.c
>>> +++ b/fs/hostfs/hostfs_kern.c
>>> @@ -105,11 +105,10 @@ static char *__dentry_name(struct dentry *dentry, char *name)
>>
>> This code looks fishy to me...
>>
>> First we have:
>>
>>     len = strlen(root);
>>     strlcpy(name, root, PATH_MAX);
>>
>> (I notice the code used strncpy() before. One difference with strlcpy()
>>  is that strncpy() fills the remaining of the destination buffer with zeroes.)
>>
>> Then:
>>
>>>                 __putname(name);
>>>                 return NULL;
>>>         }
>>> -       if (p > name + len) {
>>> -               char *s = name + len;
>>
>> Unless strlcpy() truncated the string (which is unlikely, as root
>> cannot be longer
>> than PATH_MAX?), s = name + len now points to the zero terminator.
>> So the below would copy just one single byte:

Oops, that's of course not true, as s is the destination, not the source,
of the copy operation.

>>> -               while ((*s++ = *p++) != '\0')
>>> -                       ;
>>> -       }
>>> +
>>> +       if (p > name + len)
>>> +               strcpy(name + len, p);
>>> +
>>
>> What is this code really supposed to do?
>
> Hostfs' __dentry_name() builds the real path. i.e, the prefix on the host side
> plus the requested path in UML.
>
> "strlcpy(name, root, PATH_MAX);" copies the host prefix into name and then
> the "strcpy(name + len, p);" copies the requested path into it.
>
> The trick is that both share the same buffer, allocated by dentry_path_raw().

Ah, so the path is stored in the end of the buffer...

> Therefore this bounds check works:
>         if (len > p - name) {

... and if this is true, prefix and path would overlap, which means there's\
not enough space in the buffer.

>                 __putname(name);
>                 return NULL;
>         }
>
> Is it now clearer or did I miss something?
> I agree that this code is tricky. :)

Yes, thanks for your explanation!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds