[PATCH 1/3] btrfs: don't allow trans ioctl on a directory

[PATCH 1/3] btrfs: don't allow trans ioctl on a directory

[josef]

From: Josef Bacik <jbacik@fb.com>

We need to use file->private_data for readdir on directories, so just
don't allow user space transactions on directories.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index bedeec6..ddb3811 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file)
 	struct btrfs_trans_handle *trans;
 	int ret;
 
+	if (S_ISDIR(inode->i_mode))
+		return -EINVAL;
+
 	ret = -EPERM;
 	if (!capable(CAP_SYS_ADMIN))
 		goto out;
-- 
2.7.4

[PATCH 2/3] btrfs: fix readdir deadlock with pagefault

[josef]

From: Josef Bacik <jbacik@fb.com>

Readdir does dir_emit while under the btree lock.  dir_emit can trigger
the page fault which means we can deadlock.  Fix this by allocating a
buffer on opening a directory and copying the readdir into this buffer
and doing dir_emit from outside of the tree lock.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/inode.c | 110 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 83 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 9a4413a..61396e3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5877,6 +5877,56 @@ unsigned char btrfs_filetype_table[] = {
 	DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
 };
 
+/*
+ * All this infrastructure exists because dir_emit can fault, and we are holding
+ * the tree lock when doing readdir.  For now just allocate a buffer and copy
+ * our information into that, and then dir_emit from the buffer.  This is
+ * similar to what NFS does, only we don't keep the buffer around in pagecache
+ * because I'm afraid I'll fuck that up.  Long term we need to make filldir do
+ * copy_to_user_inatomic so we don't have to worry about page faulting under the
+ * tree lock.
+ */
+static int btrfs_opendir(struct inode *inode, struct file *file)
+{
+	struct page *page;
+
+	page = alloc_page(GFP_KERNEL);
+	if (!page)
+		return -ENOMEM;
+	file->private_data = page;
+	return 0;
+}
+
+static int btrfs_closedir(struct inode *inode, struct file *file)
+{
+	if (file->private_data) {
+		__free_page((struct page *)file->private_data);
+		file->private_data = NULL;
+	}
+	return 0;
+}
+
+struct dir_entry {
+	u64 ino;
+	u64 offset;
+	unsigned type;
+	int name_len;
+};
+
+static int btrfs_filldir(void *addr, int entries, struct dir_context *ctx)
+{
+	while (entries--) {
+		struct dir_entry *entry = addr;
+		char *name = (char *)(entry + 1);
+		ctx->pos = entry->offset;
+		if (!dir_emit(ctx, name, entry->name_len, entry->ino,
+			      entry->type))
+			return 1;
+		ctx->pos++;
+	}
+	return 0;
+}
+
 static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct inode *inode = file_inode(file);
@@ -5886,16 +5936,17 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 	struct btrfs_key key;
 	struct btrfs_key found_key;
 	struct btrfs_path *path;
+	struct page *page = file->private_data;
+	void *addr, *start_addr;
 	struct list_head ins_list;
 	struct list_head del_list;
 	int ret;
 	struct extent_buffer *leaf;
 	int slot;
-	unsigned char d_type;
-	int over = 0;
-	char tmp_name[32];
 	char *name_ptr;
 	int name_len;
+	int entries = 0;
+	int total_len = 0;
 	bool put = false;
 	struct btrfs_key location;
 
@@ -5906,6 +5957,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 	if (!path)
 		return -ENOMEM;
 
+	start_addr = addr = kmap(page);
 	path->reada = READA_FORWARD;
 
 	INIT_LIST_HEAD(&ins_list);
@@ -5921,6 +5973,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 		goto err;
 
 	while (1) {
+		struct dir_entry *entry;
 		leaf = path->nodes[0];
 		slot = path->slots[0];
 		if (slot >= btrfs_header_nritems(leaf)) {
@@ -5942,41 +5995,42 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 			goto next;
 		if (btrfs_should_delete_dir_index(&del_list, found_key.offset))
 			goto next;
-
-		ctx->pos = found_key.offset;
-
 		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
 		if (verify_dir_item(fs_info, leaf, slot, di))
 			goto next;
 
 		name_len = btrfs_dir_name_len(leaf, di);
-		if (name_len <= sizeof(tmp_name)) {
-			name_ptr = tmp_name;
-		} else {
-			name_ptr = kmalloc(name_len, GFP_KERNEL);
-			if (!name_ptr) {
-				ret = -ENOMEM;
-				goto err;
-			}
+		if ((total_len + sizeof(struct dir_entry) + name_len) >=
+		    PAGE_SIZE) {
+			btrfs_release_path(path);
+			ret = btrfs_filldir(start_addr, entries, ctx);
+			if (ret)
+				goto nopos;
+			addr = start_addr;
+			entries = 0;
+			total_len = 0;
 		}
+
+		entry = addr;
+		entry->name_len = name_len;
+		name_ptr = (char *)(entry + 1);
 		read_extent_buffer(leaf, name_ptr, (unsigned long)(di + 1),
 				   name_len);
-
-		d_type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
+		entry->type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
 		btrfs_dir_item_key_to_cpu(leaf, di, &location);
-
-		over = !dir_emit(ctx, name_ptr, name_len, location.objectid,
-				 d_type);
-
-		if (name_ptr != tmp_name)
-			kfree(name_ptr);
-
-		if (over)
-			goto nopos;
-		ctx->pos++;
+		entry->ino = location.objectid;
+		entry->offset = found_key.offset;
+		entries++;
+		addr += sizeof(struct dir_entry) + name_len;
+		total_len += sizeof(struct dir_entry) + name_len;
 next:
 		path->slots[0]++;
 	}
+	btrfs_release_path(path);
+
+	ret = btrfs_filldir(start_addr, entries, ctx);
+	if (ret)
+		goto nopos;
 
 	ret = btrfs_readdir_delayed_dir_index(ctx, &ins_list);
 	if (ret)
@@ -6006,6 +6060,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 nopos:
 	ret = 0;
 err:
+	kunmap(page);
 	if (put)
 		btrfs_readdir_put_delayed_items(inode, &ins_list, &del_list);
 	btrfs_free_path(path);
@@ -10777,11 +10832,12 @@ static const struct file_operations btrfs_dir_file_operations = {
 	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.iterate_shared	= btrfs_real_readdir,
+	.open		= btrfs_opendir,
 	.unlocked_ioctl	= btrfs_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= btrfs_compat_ioctl,
 #endif
-	.release        = btrfs_release_file,
+	.release        = btrfs_closedir,
 	.fsync		= btrfs_sync_file,
 };
 
-- 
2.7.4

[PATCH 3/3] btrfs: increase ctx->pos for delayed dir index

[josef]

From: Josef Bacik <jbacik@fb.com>

Our dir_context->pos is supposed to hold the next position we're
supposed to look.  If we successfully insert a delayed dir index we
could end up with a duplicate entry because we don't increase ctx->pos
after doing the dir_emit.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/delayed-inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 8ae409b..19e4ad2 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1727,6 +1727,7 @@ int btrfs_readdir_delayed_dir_index(struct dir_context *ctx,
 
 		if (over)
 			return 1;
+		ctx->pos++;
 	}
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 2/3] btrfs: fix readdir deadlock with pagefault

[Josef Bacik]

On Fri, Jul 21, 2017 at 01:29:08PM -0400, josef@toxicpanda.com wrote:
> From: Josef Bacik <jbacik@fb.com>
> 
> Readdir does dir_emit while under the btree lock.  dir_emit can trigger
> the page fault which means we can deadlock.  Fix this by allocating a
> buffer on opening a directory and copying the readdir into this buffer
> and doing dir_emit from outside of the tree lock.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>

This is broken, don't anybody actually run this, I'm more interested in comments
on the overall solution.  I'll get it working and post a V2 after a few days if
nobody has objections to the approach as a whole.  Thanks,

Josef

Re: [PATCH 2/3] btrfs: fix readdir deadlock with pagefault

[Nikolay Borisov]

On 21.07.2017 20:29, josef@toxicpanda.com wrote:
> From: Josef Bacik <jbacik@fb.com>
> 
> Readdir does dir_emit while under the btree lock.  dir_emit can trigger
> the page fault which means we can deadlock.  Fix this by allocating a
> buffer on opening a directory and copying the readdir into this buffer
> and doing dir_emit from outside of the tree lock.

So dir_emit essentially calls filldir which can fault on the user
provided addresses. How could a fault there recurse back to the filesystem?

> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/inode.c | 110 +++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 83 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 9a4413a..61396e3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5877,6 +5877,56 @@ unsigned char btrfs_filetype_table[] = {
>  	DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
>  };
>  
> +/*
> + * All this infrastructure exists because dir_emit can fault, and we are holding
> + * the tree lock when doing readdir.  For now just allocate a buffer and copy
> + * our information into that, and then dir_emit from the buffer.  This is
> + * similar to what NFS does, only we don't keep the buffer around in pagecache
> + * because I'm afraid I'll fuck that up.  Long term we need to make filldir do
> + * copy_to_user_inatomic so we don't have to worry about page faulting under the
> + * tree lock.
> + */
> +static int btrfs_opendir(struct inode *inode, struct file *file)
> +{
> +	struct page *page;
> +
> +	page = alloc_page(GFP_KERNEL);

Use straight kzalloc(GFP_KERNEL). That will save you a kmap/kunmap in
btrfs_real_filldir

> +	if (!page)
> +		return -ENOMEM;
> +	file->private_data = page;
> +	return 0;
> +}
> +
> +static int btrfs_closedir(struct inode *inode, struct file *file)
> +{
> +	if (file->private_data) {
> +		__free_page((struct page *)file->private_data);
> +		file->private_data = NULL;
> +	}
> +	return 0;
> +}
> +
> +struct dir_entry {
> +	u64 ino;
> +	u64 offset;
> +	unsigned type;
> +	int name_len;
> +};
> +
> +static int btrfs_filldir(void *addr, int entries, struct dir_context *ctx)
> +{
> +	while (entries--) {
> +		struct dir_entry *entry = addr;
> +		char *name = (char *)(entry + 1);
> +		ctx->pos = entry->offset;
> +		if (!dir_emit(ctx, name, entry->name_len, entry->ino,
> +			      entry->type))
> +			return 1;
> +		ctx->pos++;
> +	}
> +	return 0;
> +}
> +
>  static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  {
>  	struct inode *inode = file_inode(file);
> @@ -5886,16 +5936,17 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  	struct btrfs_key key;
>  	struct btrfs_key found_key;
>  	struct btrfs_path *path;
> +	struct page *page = file->private_data;
> +	void *addr, *start_addr;
>  	struct list_head ins_list;
>  	struct list_head del_list;
>  	int ret;
>  	struct extent_buffer *leaf;
>  	int slot;
> -	unsigned char d_type;
> -	int over = 0;
> -	char tmp_name[32];
>  	char *name_ptr;
>  	int name_len;
> +	int entries = 0;
> +	int total_len = 0;
>  	bool put = false;
>  	struct btrfs_key location;
>  
> @@ -5906,6 +5957,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  	if (!path)
>  		return -ENOMEM;
>  
> +	start_addr = addr = kmap(page);
>  	path->reada = READA_FORWARD;
>  
>  	INIT_LIST_HEAD(&ins_list);
> @@ -5921,6 +5973,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  		goto err;
>  
>  	while (1) {
> +		struct dir_entry *entry;
>  		leaf = path->nodes[0];
>  		slot = path->slots[0];
>  		if (slot >= btrfs_header_nritems(leaf)) {
> @@ -5942,41 +5995,42 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  			goto next;
>  		if (btrfs_should_delete_dir_index(&del_list, found_key.offset))
>  			goto next;
> -
> -		ctx->pos = found_key.offset;
> -
>  		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
>  		if (verify_dir_item(fs_info, leaf, slot, di))
>  			goto next;
>  
>  		name_len = btrfs_dir_name_len(leaf, di);
> -		if (name_len <= sizeof(tmp_name)) {
> -			name_ptr = tmp_name;
> -		} else {
> -			name_ptr = kmalloc(name_len, GFP_KERNEL);
> -			if (!name_ptr) {
> -				ret = -ENOMEM;
> -				goto err;
> -			}
> +		if ((total_len + sizeof(struct dir_entry) + name_len) >=
> +		    PAGE_SIZE) {
> +			btrfs_release_path(path);
> +			ret = btrfs_filldir(start_addr, entries, ctx);
> +			if (ret)
> +				goto nopos;
> +			addr = start_addr;
> +			entries = 0;
> +			total_len = 0;
>  		}
> +
> +		entry = addr;
> +		entry->name_len = name_len;
> +		name_ptr = (char *)(entry + 1);
>  		read_extent_buffer(leaf, name_ptr, (unsigned long)(di + 1),
>  				   name_len);
> -
> -		d_type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
> +		entry->type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
>  		btrfs_dir_item_key_to_cpu(leaf, di, &location);
> -
> -		over = !dir_emit(ctx, name_ptr, name_len, location.objectid,
> -				 d_type);
> -
> -		if (name_ptr != tmp_name)
> -			kfree(name_ptr);
> -
> -		if (over)
> -			goto nopos;
> -		ctx->pos++;
> +		entry->ino = location.objectid;
> +		entry->offset = found_key.offset;
> +		entries++;
> +		addr += sizeof(struct dir_entry) + name_len;
> +		total_len += sizeof(struct dir_entry) + name_len;
>  next:
>  		path->slots[0]++;
>  	}
> +	btrfs_release_path(path);
> +
> +	ret = btrfs_filldir(start_addr, entries, ctx);
> +	if (ret)
> +		goto nopos;
>  
>  	ret = btrfs_readdir_delayed_dir_index(ctx, &ins_list);
>  	if (ret)
> @@ -6006,6 +6060,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  nopos:
>  	ret = 0;
>  err:
> +	kunmap(page);
>  	if (put)
>  		btrfs_readdir_put_delayed_items(inode, &ins_list, &del_list);
>  	btrfs_free_path(path);
> @@ -10777,11 +10832,12 @@ static const struct file_operations btrfs_dir_file_operations = {
>  	.llseek		= generic_file_llseek,
>  	.read		= generic_read_dir,
>  	.iterate_shared	= btrfs_real_readdir,
> +	.open		= btrfs_opendir,
>  	.unlocked_ioctl	= btrfs_ioctl,
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl	= btrfs_compat_ioctl,
>  #endif
> -	.release        = btrfs_release_file,
> +	.release        = btrfs_closedir,
>  	.fsync		= btrfs_sync_file,
>  };
>  
> 

Re: [PATCH 1/3] btrfs: don't allow trans ioctl on a directory

[David Sterba]

On Fri, Jul 21, 2017 at 01:29:07PM -0400, josef@toxicpanda.com wrote:
> From: Josef Bacik <jbacik@fb.com>
> 
> We need to use file->private_data for readdir on directories, so just
> don't allow user space transactions on directories.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index bedeec6..ddb3811 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file)
>  	struct btrfs_trans_handle *trans;
>  	int ret;
>  
> +	if (S_ISDIR(inode->i_mode))
> +		return -EINVAL;

You can't do this, starting a transaction on a directory needs to work.
The most natural way to run the ioctl is on the mount point.

The file private data would need to be able to hold multipe values, so
you can add

struct btrfs_inode {
	...
	struct priv_data {
		void *for_readdir;
		void *for_tranc_ioctl;
	};
	...
};

then set file->file_private = &btrfs_inode->priv_data; and update all
uses to check for the embedded pointers.

> +
>  	ret = -EPERM;
>  	if (!capable(CAP_SYS_ADMIN))
>  		goto out;
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] btrfs: fix readdir deadlock with pagefault

[David Sterba]

On Fri, Jul 21, 2017 at 01:29:08PM -0400, josef@toxicpanda.com wrote:
> From: Josef Bacik <jbacik@fb.com>
> 
> Readdir does dir_emit while under the btree lock.  dir_emit can trigger
> the page fault which means we can deadlock.  Fix this by allocating a
> buffer on opening a directory and copying the readdir into this buffer
> and doing dir_emit from outside of the tree lock.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/inode.c | 110 +++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 83 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 9a4413a..61396e3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5877,6 +5877,56 @@ unsigned char btrfs_filetype_table[] = {
>  	DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
>  };
>  
> +/*
> + * All this infrastructure exists because dir_emit can fault, and we are holding
> + * the tree lock when doing readdir.  For now just allocate a buffer and copy
> + * our information into that, and then dir_emit from the buffer.  This is
> + * similar to what NFS does, only we don't keep the buffer around in pagecache
> + * because I'm afraid I'll fuck that up.  Long term we need to make filldir do
> + * copy_to_user_inatomic so we don't have to worry about page faulting under the
> + * tree lock.
> + */
> +static int btrfs_opendir(struct inode *inode, struct file *file)
> +{
> +	struct page *page;
> +
> +	page = alloc_page(GFP_KERNEL);
> +	if (!page)
> +		return -ENOMEM;
> +	file->private_data = page;
> +	return 0;
> +}
> +
> +static int btrfs_closedir(struct inode *inode, struct file *file)
> +{
> +	if (file->private_data) {
> +		__free_page((struct page *)file->private_data);
> +		file->private_data = NULL;
> +	}
> +	return 0;
> +}
> +
> +struct dir_entry {
> +	u64 ino;
> +	u64 offset;
> +	unsigned type;
> +	int name_len;
> +};
> +
> +static int btrfs_filldir(void *addr, int entries, struct dir_context *ctx)
> +{
> +	while (entries--) {
> +		struct dir_entry *entry = addr;
> +		char *name = (char *)(entry + 1);
> +		ctx->pos = entry->offset;
> +		if (!dir_emit(ctx, name, entry->name_len, entry->ino,
> +			      entry->type))
> +			return 1;
> +		ctx->pos++;
> +	}
> +	return 0;
> +}
> +
>  static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  {
>  	struct inode *inode = file_inode(file);
> @@ -5886,16 +5936,17 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  	struct btrfs_key key;
>  	struct btrfs_key found_key;
>  	struct btrfs_path *path;
> +	struct page *page = file->private_data;
> +	void *addr, *start_addr;
>  	struct list_head ins_list;
>  	struct list_head del_list;
>  	int ret;
>  	struct extent_buffer *leaf;
>  	int slot;
> -	unsigned char d_type;
> -	int over = 0;
> -	char tmp_name[32];
>  	char *name_ptr;
>  	int name_len;
> +	int entries = 0;
> +	int total_len = 0;
>  	bool put = false;
>  	struct btrfs_key location;
>  
> @@ -5906,6 +5957,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  	if (!path)
>  		return -ENOMEM;
>  
> +	start_addr = addr = kmap(page);
>  	path->reada = READA_FORWARD;
>  
>  	INIT_LIST_HEAD(&ins_list);
> @@ -5921,6 +5973,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  		goto err;
>  
>  	while (1) {
> +		struct dir_entry *entry;
>  		leaf = path->nodes[0];
>  		slot = path->slots[0];
>  		if (slot >= btrfs_header_nritems(leaf)) {
> @@ -5942,41 +5995,42 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  			goto next;
>  		if (btrfs_should_delete_dir_index(&del_list, found_key.offset))
>  			goto next;
> -
> -		ctx->pos = found_key.offset;
> -
>  		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
>  		if (verify_dir_item(fs_info, leaf, slot, di))
>  			goto next;
>  
>  		name_len = btrfs_dir_name_len(leaf, di);
> -		if (name_len <= sizeof(tmp_name)) {
> -			name_ptr = tmp_name;
> -		} else {
> -			name_ptr = kmalloc(name_len, GFP_KERNEL);
> -			if (!name_ptr) {
> -				ret = -ENOMEM;
> -				goto err;
> -			}
> +		if ((total_len + sizeof(struct dir_entry) + name_len) >=
> +		    PAGE_SIZE) {

How does this work for filenames that of PATH_MAX length? Regardless of
total_len size, the rest will always overflow PAGE_SIZE if it's 4k.

> +			btrfs_release_path(path);
> +			ret = btrfs_filldir(start_addr, entries, ctx);
> +			if (ret)
> +				goto nopos;
> +			addr = start_addr;
> +			entries = 0;
> +			total_len = 0;
>  		}
> +
> +		entry = addr;
> +		entry->name_len = name_len;
> +		name_ptr = (char *)(entry + 1);
>  		read_extent_buffer(leaf, name_ptr, (unsigned long)(di + 1),
>  				   name_len);
> -
> -		d_type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
> +		entry->type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
>  		btrfs_dir_item_key_to_cpu(leaf, di, &location);
> -
> -		over = !dir_emit(ctx, name_ptr, name_len, location.objectid,
> -				 d_type);
> -
> -		if (name_ptr != tmp_name)
> -			kfree(name_ptr);
> -
> -		if (over)
> -			goto nopos;
> -		ctx->pos++;
> +		entry->ino = location.objectid;
> +		entry->offset = found_key.offset;
> +		entries++;
> +		addr += sizeof(struct dir_entry) + name_len;
> +		total_len += sizeof(struct dir_entry) + name_len;
>  next:
>  		path->slots[0]++;
>  	}
> +	btrfs_release_path(path);
> +
> +	ret = btrfs_filldir(start_addr, entries, ctx);
> +	if (ret)
> +		goto nopos;
>  
>  	ret = btrfs_readdir_delayed_dir_index(ctx, &ins_list);
>  	if (ret)
> @@ -6006,6 +6060,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  nopos:
>  	ret = 0;
>  err:
> +	kunmap(page);
>  	if (put)
>  		btrfs_readdir_put_delayed_items(inode, &ins_list, &del_list);
>  	btrfs_free_path(path);
> @@ -10777,11 +10832,12 @@ static const struct file_operations btrfs_dir_file_operations = {
>  	.llseek		= generic_file_llseek,
>  	.read		= generic_read_dir,
>  	.iterate_shared	= btrfs_real_readdir,
> +	.open		= btrfs_opendir,
>  	.unlocked_ioctl	= btrfs_ioctl,
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl	= btrfs_compat_ioctl,
>  #endif
> -	.release        = btrfs_release_file,
> +	.release        = btrfs_closedir,
>  	.fsync		= btrfs_sync_file,
>  };
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] btrfs: don't allow trans ioctl on a directory

[David Sterba]

On Mon, Jul 24, 2017 at 02:42:29PM +0200, David Sterba wrote:
> On Fri, Jul 21, 2017 at 01:29:07PM -0400, josef@toxicpanda.com wrote:
> > From: Josef Bacik <jbacik@fb.com>
> > 
> > We need to use file->private_data for readdir on directories, so just
> > don't allow user space transactions on directories.
> > 
> > Signed-off-by: Josef Bacik <jbacik@fb.com>
> > ---
> >  fs/btrfs/ioctl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index bedeec6..ddb3811 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file)
> >  	struct btrfs_trans_handle *trans;
> >  	int ret;
> >  
> > +	if (S_ISDIR(inode->i_mode))
> > +		return -EINVAL;
> 
> You can't do this, starting a transaction on a directory needs to work.
> The most natural way to run the ioctl is on the mount point.
> 
> The file private data would need to be able to hold multipe values, so
> you can add
> 
> struct btrfs_inode {
> 	...
> 	struct priv_data {
> 		void *for_readdir;
> 		void *for_tranc_ioctl;
> 	};
> 	...
> };
> 
> then set file->file_private = &btrfs_inode->priv_data; and update all
> uses to check for the embedded pointers.

So this cannot be attached to the inode but to struct file itself,
otherwise this won't work for parallel readdir obviously.

Re: [PATCH 2/3] btrfs: fix readdir deadlock with pagefault

[David Sterba]

On Mon, Jul 24, 2017 at 02:50:50PM +0200, David Sterba wrote:
> On Fri, Jul 21, 2017 at 01:29:08PM -0400, josef@toxicpanda.com wrote:
> > From: Josef Bacik <jbacik@fb.com>
> > 
> > Readdir does dir_emit while under the btree lock.  dir_emit can trigger
> > the page fault which means we can deadlock.  Fix this by allocating a
> > buffer on opening a directory and copying the readdir into this buffer
> > and doing dir_emit from outside of the tree lock.
> > 
> > Signed-off-by: Josef Bacik <jbacik@fb.com>
> > ---
> >  fs/btrfs/inode.c | 110 +++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 83 insertions(+), 27 deletions(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 9a4413a..61396e3 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -5877,6 +5877,56 @@ unsigned char btrfs_filetype_table[] = {
> >  	DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
> >  };
> >  
> > +/*
> > + * All this infrastructure exists because dir_emit can fault, and we are holding
> > + * the tree lock when doing readdir.  For now just allocate a buffer and copy
> > + * our information into that, and then dir_emit from the buffer.  This is
> > + * similar to what NFS does, only we don't keep the buffer around in pagecache
> > + * because I'm afraid I'll fuck that up.

Can you please explain the concern in more detail?

> > Long term we need to make filldir do
> > + * copy_to_user_inatomic so we don't have to worry about page faulting under the
> > + * tree lock.
> > + */
> > +static int btrfs_opendir(struct inode *inode, struct file *file)
> > +{
> > +	struct page *page;
> > +
> > +	page = alloc_page(GFP_KERNEL);
> > +	if (!page)
> > +		return -ENOMEM;
> > +	file->private_data = page;
> > +	return 0;
> > +}
> > +
> > +static int btrfs_closedir(struct inode *inode, struct file *file)
> > +{
> > +	if (file->private_data) {
> > +		__free_page((struct page *)file->private_data);
> > +		file->private_data = NULL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +struct dir_entry {
> > +	u64 ino;
> > +	u64 offset;
> > +	unsigned type;
> > +	int name_len;
> > +};
> > +
> > +static int btrfs_filldir(void *addr, int entries, struct dir_context *ctx)
> > +{
> > +	while (entries--) {
> > +		struct dir_entry *entry = addr;
> > +		char *name = (char *)(entry + 1);
> > +		ctx->pos = entry->offset;
> > +		if (!dir_emit(ctx, name, entry->name_len, entry->ino,
> > +			      entry->type))
> > +			return 1;
> > +		ctx->pos++;
> > +	}
> > +	return 0;
> > +}
> > +
> >  static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
> >  {
> >  	struct inode *inode = file_inode(file);
> > @@ -5886,16 +5936,17 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
> >  	struct btrfs_key key;
> >  	struct btrfs_key found_key;
> >  	struct btrfs_path *path;
> > +	struct page *page = file->private_data;
> > +	void *addr, *start_addr;
> >  	struct list_head ins_list;
> >  	struct list_head del_list;
> >  	int ret;
> >  	struct extent_buffer *leaf;
> >  	int slot;
> > -	unsigned char d_type;
> > -	int over = 0;
> > -	char tmp_name[32];
> >  	char *name_ptr;
> >  	int name_len;
> > +	int entries = 0;
> > +	int total_len = 0;
> >  	bool put = false;
> >  	struct btrfs_key location;
> >  
> > @@ -5906,6 +5957,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
> >  	if (!path)
> >  		return -ENOMEM;
> >  
> > +	start_addr = addr = kmap(page);
> >  	path->reada = READA_FORWARD;
> >  
> >  	INIT_LIST_HEAD(&ins_list);
> > @@ -5921,6 +5973,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
> >  		goto err;
> >  
> >  	while (1) {
> > +		struct dir_entry *entry;
> >  		leaf = path->nodes[0];
> >  		slot = path->slots[0];
> >  		if (slot >= btrfs_header_nritems(leaf)) {
> > @@ -5942,41 +5995,42 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
> >  			goto next;
> >  		if (btrfs_should_delete_dir_index(&del_list, found_key.offset))
> >  			goto next;
> > -
> > -		ctx->pos = found_key.offset;
> > -
> >  		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
> >  		if (verify_dir_item(fs_info, leaf, slot, di))
> >  			goto next;
> >  
> >  		name_len = btrfs_dir_name_len(leaf, di);
> > -		if (name_len <= sizeof(tmp_name)) {
> > -			name_ptr = tmp_name;
> > -		} else {
> > -			name_ptr = kmalloc(name_len, GFP_KERNEL);
> > -			if (!name_ptr) {
> > -				ret = -ENOMEM;
> > -				goto err;
> > -			}
> > +		if ((total_len + sizeof(struct dir_entry) + name_len) >=
> > +		    PAGE_SIZE) {
> 
> How does this work for filenames that of PATH_MAX length? Regardless of
> total_len size, the rest will always overflow PAGE_SIZE if it's 4k.

Ah no, it's filename, so the buffer will be always sufficient. So the
overall approach looks ok to me.

Re: [PATCH 2/3] btrfs: fix readdir deadlock with pagefault

[Josef Bacik]

On Mon, Jul 24, 2017 at 11:26:49AM +0300, Nikolay Borisov wrote:
> 
> 
> On 21.07.2017 20:29, josef@toxicpanda.com wrote:
> > From: Josef Bacik <jbacik@fb.com>
> > 
> > Readdir does dir_emit while under the btree lock.  dir_emit can trigger
> > the page fault which means we can deadlock.  Fix this by allocating a
> > buffer on opening a directory and copying the readdir into this buffer
> > and doing dir_emit from outside of the tree lock.
> 
> So dir_emit essentially calls filldir which can fault on the user
> provided addresses. How could a fault there recurse back to the filesystem?
> 

Thread A
readdir  <holding tree lock>
  dir_emit
    <page fault>
      down_read(mmap_sem)

Thread B
mmap write
  down_write(mmap_sem)
    page_mkwrite
      wait_ordered_extents

Process C
finish_ordered_extent
  insert_reserved_file_extent
   try to lock leaf <hang>

Thanks,

Josef

Re: [PATCH 2/3] btrfs: fix readdir deadlock with pagefault

[Josef Bacik]

On Mon, Jul 24, 2017 at 03:14:08PM +0200, David Sterba wrote:
> On Mon, Jul 24, 2017 at 02:50:50PM +0200, David Sterba wrote:
> > On Fri, Jul 21, 2017 at 01:29:08PM -0400, josef@toxicpanda.com wrote:
> > > From: Josef Bacik <jbacik@fb.com>
> > > 
> > > Readdir does dir_emit while under the btree lock.  dir_emit can trigger
> > > the page fault which means we can deadlock.  Fix this by allocating a
> > > buffer on opening a directory and copying the readdir into this buffer
> > > and doing dir_emit from outside of the tree lock.
> > > 
> > > Signed-off-by: Josef Bacik <jbacik@fb.com>
> > > ---
> > >  fs/btrfs/inode.c | 110 +++++++++++++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 83 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > index 9a4413a..61396e3 100644
> > > --- a/fs/btrfs/inode.c
> > > +++ b/fs/btrfs/inode.c
> > > @@ -5877,6 +5877,56 @@ unsigned char btrfs_filetype_table[] = {
> > >  	DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
> > >  };
> > >  
> > > +/*
> > > + * All this infrastructure exists because dir_emit can fault, and we are holding
> > > + * the tree lock when doing readdir.  For now just allocate a buffer and copy
> > > + * our information into that, and then dir_emit from the buffer.  This is
> > > + * similar to what NFS does, only we don't keep the buffer around in pagecache
> > > + * because I'm afraid I'll fuck that up.
> 
> Can you please explain the concern in more detail?
> 

If we keep the cache I'll have to have mechanisms to invalidate the page cache
so it can be regenerated at the next readdir.  Then I also have to wire up
releasepage and stuff for directories and make sure it doesn't do anything
bonkers like accidently try to write the data out for a directory.  All in all
it's not worth the headache I don't think.  Thanks,

Josef

Re: [PATCH 1/3] btrfs: don't allow trans ioctl on a directory

[Josef Bacik]

On Mon, Jul 24, 2017 at 02:42:29PM +0200, David Sterba wrote:
> On Fri, Jul 21, 2017 at 01:29:07PM -0400, josef@toxicpanda.com wrote:
> > From: Josef Bacik <jbacik@fb.com>
> > 
> > We need to use file->private_data for readdir on directories, so just
> > don't allow user space transactions on directories.
> > 
> > Signed-off-by: Josef Bacik <jbacik@fb.com>
> > ---
> >  fs/btrfs/ioctl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index bedeec6..ddb3811 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file)
> >  	struct btrfs_trans_handle *trans;
> >  	int ret;
> >  
> > +	if (S_ISDIR(inode->i_mode))
> > +		return -EINVAL;
> 
> You can't do this, starting a transaction on a directory needs to work.
> The most natural way to run the ioctl is on the mount point.
> 
> The file private data would need to be able to hold multipe values, so
> you can add
> 
> struct btrfs_inode {
> 	...
> 	struct priv_data {
> 		void *for_readdir;
> 		void *for_tranc_ioctl;
> 	};
> 	...
> };
> 
> then set file->file_private = &btrfs_inode->priv_data; and update all
> uses to check for the embedded pointers.
> 

Blah I really want to just jetison the user space transaction stuff altogether
so I was hoping this would be a first step.  But yeah we can do it your way too.
Thanks,

Josef

Re: [PATCH 1/3] btrfs: don't allow trans ioctl on a directory

[David Sterba]

On Mon, Jul 24, 2017 at 10:02:30AM -0400, Josef Bacik wrote:
> On Mon, Jul 24, 2017 at 02:42:29PM +0200, David Sterba wrote:
> > On Fri, Jul 21, 2017 at 01:29:07PM -0400, josef@toxicpanda.com wrote:
> > > From: Josef Bacik <jbacik@fb.com>
> > > 
> > > We need to use file->private_data for readdir on directories, so just
> > > don't allow user space transactions on directories.
> > > 
> > > Signed-off-by: Josef Bacik <jbacik@fb.com>
> > > ---
> > >  fs/btrfs/ioctl.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > > index bedeec6..ddb3811 100644
> > > --- a/fs/btrfs/ioctl.c
> > > +++ b/fs/btrfs/ioctl.c
> > > @@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file)
> > >  	struct btrfs_trans_handle *trans;
> > >  	int ret;
> > >  
> > > +	if (S_ISDIR(inode->i_mode))
> > > +		return -EINVAL;
> > 
> > You can't do this, starting a transaction on a directory needs to work.
> > The most natural way to run the ioctl is on the mount point.
> > 
> > The file private data would need to be able to hold multipe values, so
> > you can add
> > 
> > struct btrfs_inode {
> > 	...
> > 	struct priv_data {
> > 		void *for_readdir;
> > 		void *for_tranc_ioctl;
> > 	};
> > 	...
> > };
> > 
> > then set file->file_private = &btrfs_inode->priv_data; and update all
> > uses to check for the embedded pointers.
> 
> Blah I really want to just jetison the user space transaction stuff altogether
> so I was hoping this would be a first step.  But yeah we can do it your way too.

I'm fine with removing the trans ioctl, ceph does not use it. We may
need one or two release cycles when we mark it deprecated and
WARN_ON_ONCE when used. But as it's undocumented and tricky to use I
guess nobody will notice.  Unfortunatelly this means you still have to
add the extra structures for readdir.