[PATCH] fs: Preserve error code in get_empty_filp()

[PATCH] fs: Preserve error code in get_empty_filp()

[anatol.pomozov]

From: Anatol Pomazau <anatol@google.com>

Allocating a file structure in function get_empty_filp() might fail because
of several reasons:
 - not enough memory for file structures
 - operation is not allowed
 - user is over its limit

Currently the function returns NULL in all cases and we loose the exact
reason of the error. All callers of get_empty_filp() assume that the function
can fail with ENFILE only.

Return error through pointer. Change all callers to preserve this error code.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>
---
 arch/ia64/kernel/perfmon.c |  4 ++--
 fs/anon_inodes.c           |  5 +++--
 fs/file_table.c            | 23 +++++++++++++++--------
 fs/hugetlbfs/inode.c       |  5 +++--
 fs/namei.c                 |  4 ++--
 fs/open.c                  |  5 ++---
 fs/pipe.c                  |  9 +++++----
 ipc/shm.c                  |  4 +++-
 mm/shmem.c                 |  5 +++--
 net/socket.c               |  4 ++--
 10 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
index 3fa4bc5..da2dcce 100644
--- a/arch/ia64/kernel/perfmon.c
+++ b/arch/ia64/kernel/perfmon.c
@@ -2221,9 +2221,9 @@ pfm_alloc_file(pfm_context_t *ctx)
 	d_add(path.dentry, inode);
 
 	file = alloc_file(&path, FMODE_READ, &pfm_file_ops);
-	if (!file) {
+	if (IS_ERR(file)) {
 		path_put(&path);
-		return ERR_PTR(-ENFILE);
+		return file;
 	}
 
 	file->f_flags = O_RDONLY;
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 28d39fb..73536e3 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -160,10 +160,11 @@ struct file *anon_inode_getfile(const char *name,
 
 	d_instantiate(path.dentry, anon_inode_inode);
 
-	error = -ENFILE;
 	file = alloc_file(&path, OPEN_FMODE(flags), fops);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto err_dput;
+	}
 	file->f_mapping = anon_inode_inode->i_mapping;
 
 	file->f_pos = 0;
diff --git a/fs/file_table.c b/fs/file_table.c
index b3fc4d6..5e65706 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -94,8 +94,8 @@ int proc_nr_files(ctl_table *table, int write,
 #endif
 
 /* Find an unused file structure and return a pointer to it.
- * Returns NULL, if there are no more free file structures or
- * we run out of memory.
+ * Returns error pointer if some error happend e.g. we over file
+ * structures limit, run out of memory or operation is not permitted.
  *
  * Be very careful using this.  You are responsible for
  * getting write access to any mount that you might assign
@@ -108,6 +108,7 @@ struct file *get_empty_filp(void)
 	const struct cred *cred = current_cred();
 	static long old_max;
 	struct file * f;
+	int error;
 
 	/*
 	 * Privileged users can go above max_files
@@ -117,18 +118,24 @@ struct file *get_empty_filp(void)
 		 * percpu_counters are inaccurate.  Do an expensive check before
 		 * we go and fail.
 		 */
-		if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files)
+		if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files) {
+			error = -ENFILE;
 			goto over;
+		}
 	}
 
 	f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
-	if (f == NULL)
+	if (f == NULL) {
+		error = -ENOMEM;
 		goto fail;
+	}
 
 	percpu_counter_inc(&nr_files);
 	f->f_cred = get_cred(cred);
-	if (security_file_alloc(f))
+	if (security_file_alloc(f)) {
+		error = -EPERM;
 		goto fail_sec;
+	}
 
 	INIT_LIST_HEAD(&f->f_u.fu_list);
 	atomic_long_set(&f->f_count, 1);
@@ -149,7 +156,7 @@ over:
 fail_sec:
 	file_free(f);
 fail:
-	return NULL;
+	return ERR_PTR(error);
 }
 
 /**
@@ -173,8 +180,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
 	struct file *file;
 
 	file = get_empty_filp();
-	if (!file)
-		return NULL;
+	if (IS_ERR(file))
+		return file;
 
 	file->f_path = *path;
 	file->f_mapping = path->dentry->d_inode->i_mapping;
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 8349a89..5ec849b 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -984,11 +984,12 @@ struct file *hugetlb_file_setup(const char *name, unsigned long addr,
 	inode->i_size = size;
 	clear_nlink(inode);
 
-	error = -ENFILE;
 	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 			&hugetlbfs_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto out_dentry; /* inode is already attached */
+	}
 
 	return file;
 
diff --git a/fs/namei.c b/fs/namei.c
index 2ccc35c..b77bdca 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2753,8 +2753,8 @@ static struct file *path_openat(int dfd, const char *pathname,
 	int error;
 
 	file = get_empty_filp();
-	if (!file)
-		return ERR_PTR(-ENFILE);
+	if (IS_ERR(file))
+		return file;
 
 	file->f_flags = op->open_flag;
 
diff --git a/fs/open.c b/fs/open.c
index 1e914b3..195bdab 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -777,10 +777,9 @@ struct file *dentry_open(const struct path *path, int flags,
 	/* We must always pass in a valid mount pointer. */
 	BUG_ON(!path->mnt);
 
-	error = -ENFILE;
 	f = get_empty_filp();
-	if (f == NULL)
-		return ERR_PTR(error);
+	if (IS_ERR(f))
+		return f;
 
 	f->f_flags = flags;
 	f->f_path = *path;
diff --git a/fs/pipe.c b/fs/pipe.c
index 95cbd6b..429f2db 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1037,10 +1037,11 @@ struct file *create_write_pipe(int flags)
 
 	d_instantiate(path.dentry, inode);
 
-	err = -ENFILE;
 	f = alloc_file(&path, FMODE_WRITE, &write_pipefifo_fops);
-	if (!f)
+	if (IS_ERR(f)) {
+		err = PTR_ERR(f);
 		goto err_dentry;
+	}
 	f->f_mapping = inode->i_mapping;
 
 	f->f_flags = O_WRONLY | (flags & (O_NONBLOCK | O_DIRECT));
@@ -1072,8 +1073,8 @@ struct file *create_read_pipe(struct file *wrf, int flags)
 	/* Grab pipe from the writer */
 	struct file *f = alloc_file(&wrf->f_path, FMODE_READ,
 				    &read_pipefifo_fops);
-	if (!f)
-		return ERR_PTR(-ENFILE);
+	if (IS_ERR(f))
+		return f;
 
 	path_get(&wrf->f_path);
 	f->f_flags = O_RDONLY | (flags & O_NONBLOCK);
diff --git a/ipc/shm.c b/ipc/shm.c
index 00faa05..c8eee21 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1039,8 +1039,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 			  is_file_hugepages(shp->shm_file) ?
 				&shm_file_operations_huge :
 				&shm_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		err = PTR_ERR(file);
 		goto out_free;
+	}
 
 	file->private_data = sfd;
 	file->f_mapping = shp->shm_file->f_mapping;
diff --git a/mm/shmem.c b/mm/shmem.c
index d4e184e..cd814ee 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2948,11 +2948,12 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
 		goto put_dentry;
 #endif
 
-	error = -ENFILE;
 	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 		  &shmem_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto put_dentry;
+	}
 
 	return file;
 
diff --git a/net/socket.c b/net/socket.c
index dfe5b66..e80eb83 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -369,12 +369,12 @@ static int sock_alloc_file(struct socket *sock, struct file **f, int flags)
 
 	file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
 		  &socket_file_ops);
-	if (unlikely(!file)) {
+	if (unlikely(IS_ERR(file))) {
 		/* drop dentry, keep inode */
 		ihold(path.dentry->d_inode);
 		path_put(&path);
 		put_unused_fd(fd);
-		return -ENFILE;
+		return PTR_ERR(file);
 	}
 
 	sock->file = file;
-- 
1.7.11.3

[PATCH] fs: Preserve error code in get_empty_filp()

[anatol.pomozov]

From: Anatol Pomazau <anatol@google.com>

Allocating a file structure in function get_empty_filp() might fail because
of several reasons:
 - not enough memory for file structures
 - operation is not allowed
 - user is over its limit

Currently the function returns NULL in all cases and we loose the exact
reason of the error. All callers of get_empty_filp() assume that the function
can fail with ENFILE only.

Return error through pointer. Change all callers to preserve this error code.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>
---
 arch/ia64/kernel/perfmon.c |  4 ++--
 fs/anon_inodes.c           |  5 +++--
 fs/file_table.c            | 22 ++++++++++++++--------
 fs/hugetlbfs/inode.c       |  5 +++--
 fs/namei.c                 |  4 ++--
 fs/open.c                  |  5 ++---
 fs/pipe.c                  |  9 +++++----
 ipc/shm.c                  |  4 +++-
 mm/shmem.c                 |  5 +++--
 net/socket.c               |  4 ++--
 10 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
index 3fa4bc5..da2dcce 100644
--- a/arch/ia64/kernel/perfmon.c
+++ b/arch/ia64/kernel/perfmon.c
@@ -2221,9 +2221,9 @@ pfm_alloc_file(pfm_context_t *ctx)
 	d_add(path.dentry, inode);
 
 	file = alloc_file(&path, FMODE_READ, &pfm_file_ops);
-	if (!file) {
+	if (IS_ERR(file)) {
 		path_put(&path);
-		return ERR_PTR(-ENFILE);
+		return file;
 	}
 
 	file->f_flags = O_RDONLY;
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 28d39fb..73536e3 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -160,10 +160,11 @@ struct file *anon_inode_getfile(const char *name,
 
 	d_instantiate(path.dentry, anon_inode_inode);
 
-	error = -ENFILE;
 	file = alloc_file(&path, OPEN_FMODE(flags), fops);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto err_dput;
+	}
 	file->f_mapping = anon_inode_inode->i_mapping;
 
 	file->f_pos = 0;
diff --git a/fs/file_table.c b/fs/file_table.c
index b3fc4d6..88e8c64 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -94,8 +94,8 @@ int proc_nr_files(ctl_table *table, int write,
 #endif
 
 /* Find an unused file structure and return a pointer to it.
- * Returns NULL, if there are no more free file structures or
- * we run out of memory.
+ * Returns error pointer if some error happend e.g. we over file
+ * structures limit, run out of memory or operation is not permitted.
  *
  * Be very careful using this.  You are responsible for
  * getting write access to any mount that you might assign
@@ -108,6 +108,7 @@ struct file *get_empty_filp(void)
 	const struct cred *cred = current_cred();
 	static long old_max;
 	struct file * f;
+	int error;
 
 	/*
 	 * Privileged users can go above max_files
@@ -117,17 +118,22 @@ struct file *get_empty_filp(void)
 		 * percpu_counters are inaccurate.  Do an expensive check before
 		 * we go and fail.
 		 */
-		if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files)
+		if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files) {
+			error = -ENFILE;
 			goto over;
+		}
 	}
 
 	f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
-	if (f == NULL)
+	if (f == NULL) {
+		error = -ENOMEM;
 		goto fail;
+	}
 
 	percpu_counter_inc(&nr_files);
 	f->f_cred = get_cred(cred);
-	if (security_file_alloc(f))
+	error = security_file_alloc(f);
+	if (error)
 		goto fail_sec;
 
 	INIT_LIST_HEAD(&f->f_u.fu_list);
@@ -149,7 +155,7 @@ over:
 fail_sec:
 	file_free(f);
 fail:
-	return NULL;
+	return ERR_PTR(error);
 }
 
 /**
@@ -173,8 +179,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
 	struct file *file;
 
 	file = get_empty_filp();
-	if (!file)
-		return NULL;
+	if (IS_ERR(file))
+		return file;
 
 	file->f_path = *path;
 	file->f_mapping = path->dentry->d_inode->i_mapping;
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 8349a89..5ec849b 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -984,11 +984,12 @@ struct file *hugetlb_file_setup(const char *name, unsigned long addr,
 	inode->i_size = size;
 	clear_nlink(inode);
 
-	error = -ENFILE;
 	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 			&hugetlbfs_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto out_dentry; /* inode is already attached */
+	}
 
 	return file;
 
diff --git a/fs/namei.c b/fs/namei.c
index 2ccc35c..b77bdca 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2753,8 +2753,8 @@ static struct file *path_openat(int dfd, const char *pathname,
 	int error;
 
 	file = get_empty_filp();
-	if (!file)
-		return ERR_PTR(-ENFILE);
+	if (IS_ERR(file))
+		return file;
 
 	file->f_flags = op->open_flag;
 
diff --git a/fs/open.c b/fs/open.c
index 1e914b3..195bdab 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -777,10 +777,9 @@ struct file *dentry_open(const struct path *path, int flags,
 	/* We must always pass in a valid mount pointer. */
 	BUG_ON(!path->mnt);
 
-	error = -ENFILE;
 	f = get_empty_filp();
-	if (f == NULL)
-		return ERR_PTR(error);
+	if (IS_ERR(f))
+		return f;
 
 	f->f_flags = flags;
 	f->f_path = *path;
diff --git a/fs/pipe.c b/fs/pipe.c
index 95cbd6b..429f2db 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1037,10 +1037,11 @@ struct file *create_write_pipe(int flags)
 
 	d_instantiate(path.dentry, inode);
 
-	err = -ENFILE;
 	f = alloc_file(&path, FMODE_WRITE, &write_pipefifo_fops);
-	if (!f)
+	if (IS_ERR(f)) {
+		err = PTR_ERR(f);
 		goto err_dentry;
+	}
 	f->f_mapping = inode->i_mapping;
 
 	f->f_flags = O_WRONLY | (flags & (O_NONBLOCK | O_DIRECT));
@@ -1072,8 +1073,8 @@ struct file *create_read_pipe(struct file *wrf, int flags)
 	/* Grab pipe from the writer */
 	struct file *f = alloc_file(&wrf->f_path, FMODE_READ,
 				    &read_pipefifo_fops);
-	if (!f)
-		return ERR_PTR(-ENFILE);
+	if (IS_ERR(f))
+		return f;
 
 	path_get(&wrf->f_path);
 	f->f_flags = O_RDONLY | (flags & O_NONBLOCK);
diff --git a/ipc/shm.c b/ipc/shm.c
index 00faa05..c8eee21 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1039,8 +1039,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 			  is_file_hugepages(shp->shm_file) ?
 				&shm_file_operations_huge :
 				&shm_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		err = PTR_ERR(file);
 		goto out_free;
+	}
 
 	file->private_data = sfd;
 	file->f_mapping = shp->shm_file->f_mapping;
diff --git a/mm/shmem.c b/mm/shmem.c
index d4e184e..cd814ee 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2948,11 +2948,12 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
 		goto put_dentry;
 #endif
 
-	error = -ENFILE;
 	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 		  &shmem_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto put_dentry;
+	}
 
 	return file;
 
diff --git a/net/socket.c b/net/socket.c
index dfe5b66..e80eb83 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -369,12 +369,12 @@ static int sock_alloc_file(struct socket *sock, struct file **f, int flags)
 
 	file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
 		  &socket_file_ops);
-	if (unlikely(!file)) {
+	if (unlikely(IS_ERR(file))) {
 		/* drop dentry, keep inode */
 		ihold(path.dentry->d_inode);
 		path_put(&path);
 		put_unused_fd(fd);
-		return -ENFILE;
+		return PTR_ERR(file);
 	}
 
 	sock->file = file;
-- 
1.7.11.3

Re: [PATCH] fs: Preserve error code in get_empty_filp()

[Anatol Pomozov]

Hi

Current HEAD contains conflicting changes in fs/pipe.c. I am going to
rebase my patch and resend it.

On Wed, Aug 1, 2012 at 11:34 AM,  <anatol.pomozov@gmail.com> wrote:
> From: Anatol Pomazau <anatol@google.com>
>
> Allocating a file structure in function get_empty_filp() might fail because
> of several reasons:
>  - not enough memory for file structures
>  - operation is not allowed
>  - user is over its limit
>
> Currently the function returns NULL in all cases and we loose the exact
> reason of the error. All callers of get_empty_filp() assume that the function
> can fail with ENFILE only.
>
> Return error through pointer. Change all callers to preserve this error code.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  arch/ia64/kernel/perfmon.c |  4 ++--
>  fs/anon_inodes.c           |  5 +++--
>  fs/file_table.c            | 22 ++++++++++++++--------
>  fs/hugetlbfs/inode.c       |  5 +++--
>  fs/namei.c                 |  4 ++--
>  fs/open.c                  |  5 ++---
>  fs/pipe.c                  |  9 +++++----
>  ipc/shm.c                  |  4 +++-
>  mm/shmem.c                 |  5 +++--
>  net/socket.c               |  4 ++--
>  10 files changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
> index 3fa4bc5..da2dcce 100644
> --- a/arch/ia64/kernel/perfmon.c
> +++ b/arch/ia64/kernel/perfmon.c
> @@ -2221,9 +2221,9 @@ pfm_alloc_file(pfm_context_t *ctx)
>         d_add(path.dentry, inode);
>
>         file = alloc_file(&path, FMODE_READ, &pfm_file_ops);
> -       if (!file) {
> +       if (IS_ERR(file)) {
>                 path_put(&path);
> -               return ERR_PTR(-ENFILE);
> +               return file;
>         }
>
>         file->f_flags = O_RDONLY;
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 28d39fb..73536e3 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -160,10 +160,11 @@ struct file *anon_inode_getfile(const char *name,
>
>         d_instantiate(path.dentry, anon_inode_inode);
>
> -       error = -ENFILE;
>         file = alloc_file(&path, OPEN_FMODE(flags), fops);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               error = PTR_ERR(file);
>                 goto err_dput;
> +       }
>         file->f_mapping = anon_inode_inode->i_mapping;
>
>         file->f_pos = 0;
> diff --git a/fs/file_table.c b/fs/file_table.c
> index b3fc4d6..88e8c64 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -94,8 +94,8 @@ int proc_nr_files(ctl_table *table, int write,
>  #endif
>
>  /* Find an unused file structure and return a pointer to it.
> - * Returns NULL, if there are no more free file structures or
> - * we run out of memory.
> + * Returns error pointer if some error happend e.g. we over file
> + * structures limit, run out of memory or operation is not permitted.
>   *
>   * Be very careful using this.  You are responsible for
>   * getting write access to any mount that you might assign
> @@ -108,6 +108,7 @@ struct file *get_empty_filp(void)
>         const struct cred *cred = current_cred();
>         static long old_max;
>         struct file * f;
> +       int error;
>
>         /*
>          * Privileged users can go above max_files
> @@ -117,17 +118,22 @@ struct file *get_empty_filp(void)
>                  * percpu_counters are inaccurate.  Do an expensive check before
>                  * we go and fail.
>                  */
> -               if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files)
> +               if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files) {
> +                       error = -ENFILE;
>                         goto over;
> +               }
>         }
>
>         f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
> -       if (f == NULL)
> +       if (f == NULL) {
> +               error = -ENOMEM;
>                 goto fail;
> +       }
>
>         percpu_counter_inc(&nr_files);
>         f->f_cred = get_cred(cred);
> -       if (security_file_alloc(f))
> +       error = security_file_alloc(f);
> +       if (error)
>                 goto fail_sec;
>
>         INIT_LIST_HEAD(&f->f_u.fu_list);
> @@ -149,7 +155,7 @@ over:
>  fail_sec:
>         file_free(f);
>  fail:
> -       return NULL;
> +       return ERR_PTR(error);
>  }
>
>  /**
> @@ -173,8 +179,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
>         struct file *file;
>
>         file = get_empty_filp();
> -       if (!file)
> -               return NULL;
> +       if (IS_ERR(file))
> +               return file;
>
>         file->f_path = *path;
>         file->f_mapping = path->dentry->d_inode->i_mapping;
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 8349a89..5ec849b 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -984,11 +984,12 @@ struct file *hugetlb_file_setup(const char *name, unsigned long addr,
>         inode->i_size = size;
>         clear_nlink(inode);
>
> -       error = -ENFILE;
>         file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
>                         &hugetlbfs_file_operations);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               error = PTR_ERR(file);
>                 goto out_dentry; /* inode is already attached */
> +       }
>
>         return file;
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 2ccc35c..b77bdca 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2753,8 +2753,8 @@ static struct file *path_openat(int dfd, const char *pathname,
>         int error;
>
>         file = get_empty_filp();
> -       if (!file)
> -               return ERR_PTR(-ENFILE);
> +       if (IS_ERR(file))
> +               return file;
>
>         file->f_flags = op->open_flag;
>
> diff --git a/fs/open.c b/fs/open.c
> index 1e914b3..195bdab 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -777,10 +777,9 @@ struct file *dentry_open(const struct path *path, int flags,
>         /* We must always pass in a valid mount pointer. */
>         BUG_ON(!path->mnt);
>
> -       error = -ENFILE;
>         f = get_empty_filp();
> -       if (f == NULL)
> -               return ERR_PTR(error);
> +       if (IS_ERR(f))
> +               return f;
>
>         f->f_flags = flags;
>         f->f_path = *path;
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 95cbd6b..429f2db 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1037,10 +1037,11 @@ struct file *create_write_pipe(int flags)
>
>         d_instantiate(path.dentry, inode);
>
> -       err = -ENFILE;
>         f = alloc_file(&path, FMODE_WRITE, &write_pipefifo_fops);
> -       if (!f)
> +       if (IS_ERR(f)) {
> +               err = PTR_ERR(f);
>                 goto err_dentry;
> +       }
>         f->f_mapping = inode->i_mapping;
>
>         f->f_flags = O_WRONLY | (flags & (O_NONBLOCK | O_DIRECT));
> @@ -1072,8 +1073,8 @@ struct file *create_read_pipe(struct file *wrf, int flags)
>         /* Grab pipe from the writer */
>         struct file *f = alloc_file(&wrf->f_path, FMODE_READ,
>                                     &read_pipefifo_fops);
> -       if (!f)
> -               return ERR_PTR(-ENFILE);
> +       if (IS_ERR(f))
> +               return f;
>
>         path_get(&wrf->f_path);
>         f->f_flags = O_RDONLY | (flags & O_NONBLOCK);
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 00faa05..c8eee21 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1039,8 +1039,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>                           is_file_hugepages(shp->shm_file) ?
>                                 &shm_file_operations_huge :
>                                 &shm_file_operations);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               err = PTR_ERR(file);
>                 goto out_free;
> +       }
>
>         file->private_data = sfd;
>         file->f_mapping = shp->shm_file->f_mapping;
> diff --git a/mm/shmem.c b/mm/shmem.c
> index d4e184e..cd814ee 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2948,11 +2948,12 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
>                 goto put_dentry;
>  #endif
>
> -       error = -ENFILE;
>         file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
>                   &shmem_file_operations);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               error = PTR_ERR(file);
>                 goto put_dentry;
> +       }
>
>         return file;
>
> diff --git a/net/socket.c b/net/socket.c
> index dfe5b66..e80eb83 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -369,12 +369,12 @@ static int sock_alloc_file(struct socket *sock, struct file **f, int flags)
>
>         file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
>                   &socket_file_ops);
> -       if (unlikely(!file)) {
> +       if (unlikely(IS_ERR(file))) {
>                 /* drop dentry, keep inode */
>                 ihold(path.dentry->d_inode);
>                 path_put(&path);
>                 put_unused_fd(fd);
> -               return -ENFILE;
> +               return PTR_ERR(file);
>         }
>
>         sock->file = file;
> --
> 1.7.11.3
>

[PATCH] fs: Preserve error code in get_empty_filp()

[Anatol Pomozov]

Allocating a file structure in function get_empty_filp() might fail because
of several reasons:
 - not enough memory for file structures
 - operation is not allowed
 - user is over its limit

Currently the function returns NULL in all cases and we loose the exact
reason of the error. All callers of get_empty_filp() assume that the function
can fail with ENFILE only.

Return error through pointer. Change all callers to preserve this error code.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>
---
 arch/ia64/kernel/perfmon.c |  4 ++--
 fs/anon_inodes.c           |  5 +++--
 fs/file_table.c            | 22 ++++++++++++++--------
 fs/hugetlbfs/inode.c       |  5 +++--
 fs/namei.c                 |  4 ++--
 fs/open.c                  |  5 ++---
 fs/pipe.c                  |  9 ++++++---
 ipc/shm.c                  |  4 +++-
 mm/shmem.c                 |  5 +++--
 net/socket.c               |  4 ++--
 10 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
index 3fa4bc5..da2dcce 100644
--- a/arch/ia64/kernel/perfmon.c
+++ b/arch/ia64/kernel/perfmon.c
@@ -2221,9 +2221,9 @@ pfm_alloc_file(pfm_context_t *ctx)
 	d_add(path.dentry, inode);
 
 	file = alloc_file(&path, FMODE_READ, &pfm_file_ops);
-	if (!file) {
+	if (IS_ERR(file)) {
 		path_put(&path);
-		return ERR_PTR(-ENFILE);
+		return file;
 	}
 
 	file->f_flags = O_RDONLY;
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 28d39fb..73536e3 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -160,10 +160,11 @@ struct file *anon_inode_getfile(const char *name,
 
 	d_instantiate(path.dentry, anon_inode_inode);
 
-	error = -ENFILE;
 	file = alloc_file(&path, OPEN_FMODE(flags), fops);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto err_dput;
+	}
 	file->f_mapping = anon_inode_inode->i_mapping;
 
 	file->f_pos = 0;
diff --git a/fs/file_table.c b/fs/file_table.c
index 701985e..3b3f4d7 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -94,8 +94,8 @@ int proc_nr_files(ctl_table *table, int write,
 #endif
 
 /* Find an unused file structure and return a pointer to it.
- * Returns NULL, if there are no more free file structures or
- * we run out of memory.
+ * Returns an error pointer if some error happend e.g. we over file
+ * structures limit, run out of memory or operation is not permitted.
  *
  * Be very careful using this.  You are responsible for
  * getting write access to any mount that you might assign
@@ -108,6 +108,7 @@ struct file *get_empty_filp(void)
 	const struct cred *cred = current_cred();
 	static long old_max;
 	struct file * f;
+	int error;
 
 	/*
 	 * Privileged users can go above max_files
@@ -117,17 +118,22 @@ struct file *get_empty_filp(void)
 		 * percpu_counters are inaccurate.  Do an expensive check before
 		 * we go and fail.
 		 */
-		if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files)
+		if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files) {
+			error = -ENFILE;
 			goto over;
+		}
 	}
 
 	f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
-	if (f == NULL)
+	if (f == NULL) {
+		error = -ENOMEM;
 		goto fail;
+	}
 
 	percpu_counter_inc(&nr_files);
 	f->f_cred = get_cred(cred);
-	if (security_file_alloc(f))
+	error = security_file_alloc(f);
+	if (error)
 		goto fail_sec;
 
 	INIT_LIST_HEAD(&f->f_u.fu_list);
@@ -149,7 +155,7 @@ over:
 fail_sec:
 	file_free(f);
 fail:
-	return NULL;
+	return ERR_PTR(error);
 }
 
 /**
@@ -173,8 +179,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
 	struct file *file;
 
 	file = get_empty_filp();
-	if (!file)
-		return NULL;
+	if (IS_ERR(file))
+		return file;
 
 	file->f_path = *path;
 	file->f_mapping = path->dentry->d_inode->i_mapping;
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 8349a89..5ec849b 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -984,11 +984,12 @@ struct file *hugetlb_file_setup(const char *name, unsigned long addr,
 	inode->i_size = size;
 	clear_nlink(inode);
 
-	error = -ENFILE;
 	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 			&hugetlbfs_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto out_dentry; /* inode is already attached */
+	}
 
 	return file;
 
diff --git a/fs/namei.c b/fs/namei.c
index 1b46439..85544e9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2879,8 +2879,8 @@ static struct file *path_openat(int dfd, const char *pathname,
 	int error;
 
 	file = get_empty_filp();
-	if (!file)
-		return ERR_PTR(-ENFILE);
+	if (IS_ERR(file))
+		return file;
 
 	file->f_flags = op->open_flag;
 
diff --git a/fs/open.c b/fs/open.c
index f3d96e7..1587b2d 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -781,10 +781,9 @@ struct file *dentry_open(const struct path *path, int flags,
 	/* We must always pass in a valid mount pointer. */
 	BUG_ON(!path->mnt);
 
-	error = -ENFILE;
 	f = get_empty_filp();
-	if (f == NULL)
-		return ERR_PTR(error);
+	if (IS_ERR(f))
+		return f;
 
 	f->f_flags = flags;
 	f->f_path = *path;
diff --git a/fs/pipe.c b/fs/pipe.c
index 8d85d70..e420908 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1035,16 +1035,19 @@ int create_pipe_files(struct file **res, int flags)
 
 	d_instantiate(path.dentry, inode);
 
-	err = -ENFILE;
 	f = alloc_file(&path, FMODE_WRITE, &write_pipefifo_fops);
-	if (!f)
+	if (IS_ERR(f)) {
+		err = PTR_ERR(f);
 		goto err_dentry;
+	}
 
 	f->f_flags = O_WRONLY | (flags & (O_NONBLOCK | O_DIRECT));
 
 	res[0] = alloc_file(&path, FMODE_READ, &read_pipefifo_fops);
-	if (!res[0])
+	if (IS_ERR(res[0])) {
+		err = PTR_ERR(res[0]);
 		goto err_file;
+	}
 
 	path_get(&path);
 	res[0]->f_flags = O_RDONLY | (flags & O_NONBLOCK);
diff --git a/ipc/shm.c b/ipc/shm.c
index 00faa05..c8eee21 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1039,8 +1039,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 			  is_file_hugepages(shp->shm_file) ?
 				&shm_file_operations_huge :
 				&shm_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		err = PTR_ERR(file);
 		goto out_free;
+	}
 
 	file->private_data = sfd;
 	file->f_mapping = shp->shm_file->f_mapping;
diff --git a/mm/shmem.c b/mm/shmem.c
index d4e184e..cd814ee 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2948,11 +2948,12 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
 		goto put_dentry;
 #endif
 
-	error = -ENFILE;
 	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 		  &shmem_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto put_dentry;
+	}
 
 	return file;
 
diff --git a/net/socket.c b/net/socket.c
index dfe5b66..e80eb83 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -369,12 +369,12 @@ static int sock_alloc_file(struct socket *sock, struct file **f, int flags)
 
 	file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
 		  &socket_file_ops);
-	if (unlikely(!file)) {
+	if (unlikely(IS_ERR(file))) {
 		/* drop dentry, keep inode */
 		ihold(path.dentry->d_inode);
 		path_put(&path);
 		put_unused_fd(fd);
-		return -ENFILE;
+		return PTR_ERR(file);
 	}
 
 	sock->file = file;
-- 
1.7.11.4

Re: [PATCH] fs: Preserve error code in get_empty_filp()

[Jan Engelhardt]

On Wednesday 2012-08-01 20:19, anatol.pomozov@gmail.com wrote:
>Allocating a file structure in function get_empty_filp() might fail because
>of several reasons:
> - not enough memory for file structures
> - operation is not allowed
> - user is over its limit
>
>Currently the function returns NULL in all cases and we loose the exact
>reason of the error. All callers of get_empty_filp() assume that the function
>can fail with ENFILE only.
>
>Return error through pointer. Change all callers to preserve this error code.

> 	percpu_counter_inc(&nr_files);
> 	f->f_cred = get_cred(cred);
>-	if (security_file_alloc(f))
>+	if (security_file_alloc(f)) {
>+		error = -EPERM;
> 		goto fail_sec;
>+	}

You are not preserving the error code from security_file_alloc here.

In particular, apparmoar/lsm.c: file_alloc_security can return -ENOMEM,
for example.

Re: [PATCH] fs: Preserve error code in get_empty_filp()

[Anatol Pomozov]

Hi

On Tue, Aug 21, 2012 at 3:06 AM, Jan Engelhardt <jengelh@inai.de> wrote:
>
>
> On Wednesday 2012-08-01 20:19, anatol.pomozov@gmail.com wrote:
> >Allocating a file structure in function get_empty_filp() might fail
> > because
> >of several reasons:
> > - not enough memory for file structures
> > - operation is not allowed
> > - user is over its limit
> >
> >Currently the function returns NULL in all cases and we loose the exact
> >reason of the error. All callers of get_empty_filp() assume that the
> > function
> >can fail with ENFILE only.
> >
> >Return error through pointer. Change all callers to preserve this error
> > code.
>
> >       percpu_counter_inc(&nr_files);
> >       f->f_cred = get_cred(cred);
> >-      if (security_file_alloc(f))
> >+      if (security_file_alloc(f)) {
> >+              error = -EPERM;
> >               goto fail_sec;
> >+      }
>
> You are not preserving the error code from security_file_alloc here.
>
> In particular, apparmoar/lsm.c: file_alloc_security can return -ENOMEM,
> for example.

Yep, this was a bug in the first version of the patch. It the latest
version (from Aug 2) it is fixed, please review this version instead.

Re: [PATCH] fs: Preserve error code in get_empty_filp()

[Anatol Pomozov]

Hi, Al

Do you have some time to review this change? Does it look good?

On Thu, Aug 2, 2012 at 5:47 PM, Anatol Pomozov <anatol.pomozov@gmail.com> wrote:
> Allocating a file structure in function get_empty_filp() might fail because
> of several reasons:
>  - not enough memory for file structures
>  - operation is not allowed
>  - user is over its limit
>
> Currently the function returns NULL in all cases and we loose the exact
> reason of the error. All callers of get_empty_filp() assume that the function
> can fail with ENFILE only.
>
> Return error through pointer. Change all callers to preserve this error code.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  arch/ia64/kernel/perfmon.c |  4 ++--
>  fs/anon_inodes.c           |  5 +++--
>  fs/file_table.c            | 22 ++++++++++++++--------
>  fs/hugetlbfs/inode.c       |  5 +++--
>  fs/namei.c                 |  4 ++--
>  fs/open.c                  |  5 ++---
>  fs/pipe.c                  |  9 ++++++---
>  ipc/shm.c                  |  4 +++-
>  mm/shmem.c                 |  5 +++--
>  net/socket.c               |  4 ++--
>  10 files changed, 40 insertions(+), 27 deletions(-)
>
> diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
> index 3fa4bc5..da2dcce 100644
> --- a/arch/ia64/kernel/perfmon.c
> +++ b/arch/ia64/kernel/perfmon.c
> @@ -2221,9 +2221,9 @@ pfm_alloc_file(pfm_context_t *ctx)
>         d_add(path.dentry, inode);
>
>         file = alloc_file(&path, FMODE_READ, &pfm_file_ops);
> -       if (!file) {
> +       if (IS_ERR(file)) {
>                 path_put(&path);
> -               return ERR_PTR(-ENFILE);
> +               return file;
>         }
>
>         file->f_flags = O_RDONLY;
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 28d39fb..73536e3 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -160,10 +160,11 @@ struct file *anon_inode_getfile(const char *name,
>
>         d_instantiate(path.dentry, anon_inode_inode);
>
> -       error = -ENFILE;
>         file = alloc_file(&path, OPEN_FMODE(flags), fops);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               error = PTR_ERR(file);
>                 goto err_dput;
> +       }
>         file->f_mapping = anon_inode_inode->i_mapping;
>
>         file->f_pos = 0;
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 701985e..3b3f4d7 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -94,8 +94,8 @@ int proc_nr_files(ctl_table *table, int write,
>  #endif
>
>  /* Find an unused file structure and return a pointer to it.
> - * Returns NULL, if there are no more free file structures or
> - * we run out of memory.
> + * Returns an error pointer if some error happend e.g. we over file
> + * structures limit, run out of memory or operation is not permitted.
>   *
>   * Be very careful using this.  You are responsible for
>   * getting write access to any mount that you might assign
> @@ -108,6 +108,7 @@ struct file *get_empty_filp(void)
>         const struct cred *cred = current_cred();
>         static long old_max;
>         struct file * f;
> +       int error;
>
>         /*
>          * Privileged users can go above max_files
> @@ -117,17 +118,22 @@ struct file *get_empty_filp(void)
>                  * percpu_counters are inaccurate.  Do an expensive check before
>                  * we go and fail.
>                  */
> -               if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files)
> +               if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files) {
> +                       error = -ENFILE;
>                         goto over;
> +               }
>         }
>
>         f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
> -       if (f == NULL)
> +       if (f == NULL) {
> +               error = -ENOMEM;
>                 goto fail;
> +       }
>
>         percpu_counter_inc(&nr_files);
>         f->f_cred = get_cred(cred);
> -       if (security_file_alloc(f))
> +       error = security_file_alloc(f);
> +       if (error)
>                 goto fail_sec;
>
>         INIT_LIST_HEAD(&f->f_u.fu_list);
> @@ -149,7 +155,7 @@ over:
>  fail_sec:
>         file_free(f);
>  fail:
> -       return NULL;
> +       return ERR_PTR(error);
>  }
>
>  /**
> @@ -173,8 +179,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
>         struct file *file;
>
>         file = get_empty_filp();
> -       if (!file)
> -               return NULL;
> +       if (IS_ERR(file))
> +               return file;
>
>         file->f_path = *path;
>         file->f_mapping = path->dentry->d_inode->i_mapping;
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 8349a89..5ec849b 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -984,11 +984,12 @@ struct file *hugetlb_file_setup(const char *name, unsigned long addr,
>         inode->i_size = size;
>         clear_nlink(inode);
>
> -       error = -ENFILE;
>         file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
>                         &hugetlbfs_file_operations);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               error = PTR_ERR(file);
>                 goto out_dentry; /* inode is already attached */
> +       }
>
>         return file;
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 1b46439..85544e9 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2879,8 +2879,8 @@ static struct file *path_openat(int dfd, const char *pathname,
>         int error;
>
>         file = get_empty_filp();
> -       if (!file)
> -               return ERR_PTR(-ENFILE);
> +       if (IS_ERR(file))
> +               return file;
>
>         file->f_flags = op->open_flag;
>
> diff --git a/fs/open.c b/fs/open.c
> index f3d96e7..1587b2d 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -781,10 +781,9 @@ struct file *dentry_open(const struct path *path, int flags,
>         /* We must always pass in a valid mount pointer. */
>         BUG_ON(!path->mnt);
>
> -       error = -ENFILE;
>         f = get_empty_filp();
> -       if (f == NULL)
> -               return ERR_PTR(error);
> +       if (IS_ERR(f))
> +               return f;
>
>         f->f_flags = flags;
>         f->f_path = *path;
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 8d85d70..e420908 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1035,16 +1035,19 @@ int create_pipe_files(struct file **res, int flags)
>
>         d_instantiate(path.dentry, inode);
>
> -       err = -ENFILE;
>         f = alloc_file(&path, FMODE_WRITE, &write_pipefifo_fops);
> -       if (!f)
> +       if (IS_ERR(f)) {
> +               err = PTR_ERR(f);
>                 goto err_dentry;
> +       }
>
>         f->f_flags = O_WRONLY | (flags & (O_NONBLOCK | O_DIRECT));
>
>         res[0] = alloc_file(&path, FMODE_READ, &read_pipefifo_fops);
> -       if (!res[0])
> +       if (IS_ERR(res[0])) {
> +               err = PTR_ERR(res[0]);
>                 goto err_file;
> +       }
>
>         path_get(&path);
>         res[0]->f_flags = O_RDONLY | (flags & O_NONBLOCK);
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 00faa05..c8eee21 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1039,8 +1039,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>                           is_file_hugepages(shp->shm_file) ?
>                                 &shm_file_operations_huge :
>                                 &shm_file_operations);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               err = PTR_ERR(file);
>                 goto out_free;
> +       }
>
>         file->private_data = sfd;
>         file->f_mapping = shp->shm_file->f_mapping;
> diff --git a/mm/shmem.c b/mm/shmem.c
> index d4e184e..cd814ee 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2948,11 +2948,12 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
>                 goto put_dentry;
>  #endif
>
> -       error = -ENFILE;
>         file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
>                   &shmem_file_operations);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               error = PTR_ERR(file);
>                 goto put_dentry;
> +       }
>
>         return file;
>
> diff --git a/net/socket.c b/net/socket.c
> index dfe5b66..e80eb83 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -369,12 +369,12 @@ static int sock_alloc_file(struct socket *sock, struct file **f, int flags)
>
>         file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
>                   &socket_file_ops);
> -       if (unlikely(!file)) {
> +       if (unlikely(IS_ERR(file))) {
>                 /* drop dentry, keep inode */
>                 ihold(path.dentry->d_inode);
>                 path_put(&path);
>                 put_unused_fd(fd);
> -               return -ENFILE;
> +               return PTR_ERR(file);
>         }
>
>         sock->file = file;
> --
> 1.7.11.4
>

Re: [PATCH] fs: Preserve error code in get_empty_filp()

[Theodore Ts'o]

> > A little bit of context for this change. We at Google work on a test
> > framework that shows how kernel behaves under memory pressure. In the
> > codepath that I am fixing the syscalls return ENFILE error, but in
> > fact the correct error would be ENOMEM. get_empty_filp() should
> > preserve the original error and not to replace all errors with ENFILE.
> 
> The trouble is, you are introducing previously impossible return values
> for pipe(2).  The rest of it is probably OK (even though I'd prefer to
> split get_empty_filp() part into a separate commit), but this one has
> potential for breaking previously correct userland code.  OTOH, FreeBSD has
> done that a while ago and they apparently had been able to cope with the
> fallout.

Sure, but Posix/SUSv3 has always said that system calls can return
error values that aren't listed in the standard (or the man page).
Given that most applications check for an error, and then use the
errno to log an error which a human can interpret, it would seem to me
to be better to return ENOMEM rather than to return the clearly wrong
ENFILE; after all, we could potentially have only a handful of file
descriptors open at the time when pipe(2) fails due to lack of memory,
and the error code:

       ENFILE The system limit on the total number of open files has
              been reached.

is clearly wrong.

Are you aware of any applications that would blow up if pipe(2)
returned any possible error other than ENFILE?

						- Ted

Re: [PATCH] fs: Preserve error code in get_empty_filp()

[Al Viro]

On Fri, Oct 05, 2012 at 11:16:12AM -0700, Anatol Pomozov wrote:
> Hi, AlViro
> 
> Is any reason why this change is ignored? For me it looks like a
> straightforward bugfix.
> 
> A little bit of context for this change. We at Google work on a test
> framework that shows how kernel behaves under memory pressure. In the
> codepath that I am fixing the syscalls return ENFILE error, but in
> fact the correct error would be ENOMEM. get_empty_filp() should
> preserve the original error and not to replace all errors with ENFILE.

The trouble is, you are introducing previously impossible return values
for pipe(2).  The rest of it is probably OK (even though I'd prefer to
split get_empty_filp() part into a separate commit), but this one has
potential for breaking previously correct userland code.  OTOH, FreeBSD has
done that a while ago and they apparently had been able to cope with the
fallout.

I'll put that thing sans the fs/pipe.c part in for-next, but IMO the
change of pipe(2) shouldn't go in at the moment.

Re: [PATCH] fs: Preserve error code in get_empty_filp()

[Theodore Ts'o]

On Fri, Oct 05, 2012 at 11:16:12AM -0700, Anatol Pomozov wrote:
> Hi, AlViro
> 
> Is any reason why this change is ignored? For me it looks like a
> straightforward bugfix.
> 
> A little bit of context for this change. We at Google work on a test
> framework that shows how kernel behaves under memory pressure. In the
> codepath that I am fixing the syscalls return ENFILE error, but in
> fact the correct error would be ENOMEM. get_empty_filp() should
> preserve the original error and not to replace all errors with ENFILE.

Anatol,

I suggest that you rebase this patch against the latest kernel and
then resend it; I just tried editing out the angle brackets, and the
patch completely failed to apply; every single patch hunk was rejected
by patch(1).  This may be because when you forwarded the patch as a
reply, the whitespace got corrupted --- but that's why I never
recommend resending a patch as part of a reply.  Instead, send a new
patch as a new thread, freshly regenerated against whatever is most
likely to be useful to the maintainer (aviro in this case) and then
put a ping as a comment or in the subject line.

Cheers,

						- Ted

% patch -b -p1 < /tmp/patch 
patching file arch/ia64/kernel/perfmon.c
Hunk #1 FAILED at 2221.
1 out of 1 hunk FAILED -- saving rejects to file arch/ia64/kernel/perfmon.c.rej
patching file fs/anon_inodes.c
Hunk #1 FAILED at 160.
1 out of 1 hunk FAILED -- saving rejects to file fs/anon_inodes.c.rej
patching file fs/file_table.c
Hunk #2 FAILED at 108.
Hunk #3 FAILED at 117.
Hunk #4 FAILED at 149.
Hunk #5 FAILED at 173.
4 out of 5 hunks FAILED -- saving rejects to file fs/file_table.c.rej
patching file fs/hugetlbfs/inode.c
Hunk #1 FAILED at 984.
1 out of 1 hunk FAILED -- saving rejects to file fs/hugetlbfs/inode.c.rej
patching file fs/namei.c
Hunk #1 FAILED at 2885.
1 out of 1 hunk FAILED -- saving rejects to file fs/namei.c.rej
patching file fs/open.c
Hunk #1 FAILED at 781.
1 out of 1 hunk FAILED -- saving rejects to file fs/open.c.rej
patching file fs/pipe.c
Hunk #1 FAILED at 1035.
1 out of 1 hunk FAILED -- saving rejects to file fs/pipe.c.rej
patching file ipc/shm.c
Hunk #1 FAILED at 1039.
1 out of 1 hunk FAILED -- saving rejects to file ipc/shm.c.rej
patching file mm/shmem.c
Hunk #1 FAILED at 2948.
1 out of 1 hunk FAILED -- saving rejects to file mm/shmem.c.rej
patching file net/socket.c
Hunk #1 FAILED at 369.
1 out of 1 hunk FAILED -- saving rejects to file net/socket.c.rej

Re: [PATCH] fs: Preserve error code in get_empty_filp()

[Anatol Pomozov]

Hi, AlViro

Is any reason why this change is ignored? For me it looks like a
straightforward bugfix.

A little bit of context for this change. We at Google work on a test
framework that shows how kernel behaves under memory pressure. In the
codepath that I am fixing the syscalls return ENFILE error, but in
fact the correct error would be ENOMEM. get_empty_filp() should
preserve the original error and not to replace all errors with ENFILE.

On Wed, Sep 12, 2012 at 8:11 PM, Anatol Pomozov
<anatol.pomozov@gmail.com> wrote:
> Allocating a file structure in function get_empty_filp() might fail because
> of several reasons:
>  - not enough memory for file structures
>  - operation is not allowed
>  - user is over its limit
>
> Currently the function returns NULL in all cases and we loose the exact
> reason of the error. All callers of get_empty_filp() assume that the function
> can fail with ENFILE only.
>
> Return error through pointer. Change all callers to preserve this error code.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  arch/ia64/kernel/perfmon.c |  4 ++--
>  fs/anon_inodes.c           |  5 +++--
>  fs/file_table.c            | 22 ++++++++++++++--------
>  fs/hugetlbfs/inode.c       |  5 +++--
>  fs/namei.c                 |  4 ++--
>  fs/open.c                  |  5 ++---
>  fs/pipe.c                  |  9 ++++++---
>  ipc/shm.c                  |  4 +++-
>  mm/shmem.c                 |  5 +++--
>  net/socket.c               |  4 ++--
>  10 files changed, 40 insertions(+), 27 deletions(-)
>
> diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
> index 3fa4bc5..da2dcce 100644
> --- a/arch/ia64/kernel/perfmon.c
> +++ b/arch/ia64/kernel/perfmon.c
> @@ -2221,9 +2221,9 @@ pfm_alloc_file(pfm_context_t *ctx)
>         d_add(path.dentry, inode);
>
>         file = alloc_file(&path, FMODE_READ, &pfm_file_ops);
> -       if (!file) {
> +       if (IS_ERR(file)) {
>                 path_put(&path);
> -               return ERR_PTR(-ENFILE);
> +               return file;
>         }
>
>         file->f_flags = O_RDONLY;
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 28d39fb..73536e3 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -160,10 +160,11 @@ struct file *anon_inode_getfile(const char *name,
>
>         d_instantiate(path.dentry, anon_inode_inode);
>
> -       error = -ENFILE;
>         file = alloc_file(&path, OPEN_FMODE(flags), fops);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               error = PTR_ERR(file);
>                 goto err_dput;
> +       }
>         file->f_mapping = anon_inode_inode->i_mapping;
>
>         file->f_pos = 0;
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 701985e..3b3f4d7 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -94,8 +94,8 @@ int proc_nr_files(ctl_table *table, int write,
>  #endif
>
>  /* Find an unused file structure and return a pointer to it.
> - * Returns NULL, if there are no more free file structures or
> - * we run out of memory.
> + * Returns an error pointer if some error happend e.g. we over file
> + * structures limit, run out of memory or operation is not permitted.
>   *
>   * Be very careful using this.  You are responsible for
>   * getting write access to any mount that you might assign
> @@ -108,6 +108,7 @@ struct file *get_empty_filp(void)
>         const struct cred *cred = current_cred();
>         static long old_max;
>         struct file * f;
> +       int error;
>
>         /*
>          * Privileged users can go above max_files
> @@ -117,17 +118,22 @@ struct file *get_empty_filp(void)
>                  * percpu_counters are inaccurate.  Do an expensive check before
>                  * we go and fail.
>                  */
> -               if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files)
> +               if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files) {
> +                       error = -ENFILE;
>                         goto over;
> +               }
>         }
>
>         f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
> -       if (f == NULL)
> +       if (f == NULL) {
> +               error = -ENOMEM;
>                 goto fail;
> +       }
>
>         percpu_counter_inc(&nr_files);
>         f->f_cred = get_cred(cred);
> -       if (security_file_alloc(f))
> +       error = security_file_alloc(f);
> +       if (error)
>                 goto fail_sec;
>
>         INIT_LIST_HEAD(&f->f_u.fu_list);
> @@ -149,7 +155,7 @@ over:
>  fail_sec:
>         file_free(f);
>  fail:
> -       return NULL;
> +       return ERR_PTR(error);
>  }
>
>  /**
> @@ -173,8 +179,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
>         struct file *file;
>
>         file = get_empty_filp();
> -       if (!file)
> -               return NULL;
> +       if (IS_ERR(file))
> +               return file;
>
>         file->f_path = *path;
>         file->f_mapping = path->dentry->d_inode->i_mapping;
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 8349a89..5ec849b 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -984,11 +984,12 @@ struct file *hugetlb_file_setup(const char *name, unsigned long addr,
>         inode->i_size = size;
>         clear_nlink(inode);
>
> -       error = -ENFILE;
>         file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
>                         &hugetlbfs_file_operations);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               error = PTR_ERR(file);
>                 goto out_dentry; /* inode is already attached */
> +       }
>
>         return file;
>
> diff --git a/fs/namei.c b/fs/namei.c
> index dd1ed1b..0160da0 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2885,8 +2885,8 @@ static struct file *path_openat(int dfd, const char *pathname,
>         int error;
>
>         file = get_empty_filp();
> -       if (!file)
> -               return ERR_PTR(-ENFILE);
> +       if (IS_ERR(file))
> +               return file;
>
>         file->f_flags = op->open_flag;
>
> diff --git a/fs/open.c b/fs/open.c
> index e1f2cdb..cb9a385 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -781,10 +781,9 @@ struct file *dentry_open(const struct path *path, int flags,
>         /* We must always pass in a valid mount pointer. */
>         BUG_ON(!path->mnt);
>
> -       error = -ENFILE;
>         f = get_empty_filp();
> -       if (f == NULL)
> -               return ERR_PTR(error);
> +       if (IS_ERR(f))
> +               return f;
>
>         f->f_flags = flags;
>         f->f_path = *path;
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 8d85d70..e420908 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1035,16 +1035,19 @@ int create_pipe_files(struct file **res, int flags)
>
>         d_instantiate(path.dentry, inode);
>
> -       err = -ENFILE;
>         f = alloc_file(&path, FMODE_WRITE, &write_pipefifo_fops);
> -       if (!f)
> +       if (IS_ERR(f)) {
> +               err = PTR_ERR(f);
>                 goto err_dentry;
> +       }
>
>         f->f_flags = O_WRONLY | (flags & (O_NONBLOCK | O_DIRECT));
>
>         res[0] = alloc_file(&path, FMODE_READ, &read_pipefifo_fops);
> -       if (!res[0])
> +       if (IS_ERR(res[0])) {
> +               err = PTR_ERR(res[0]);
>                 goto err_file;
> +       }
>
>         path_get(&path);
>         res[0]->f_flags = O_RDONLY | (flags & O_NONBLOCK);
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 00faa05..c8eee21 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1039,8 +1039,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>                           is_file_hugepages(shp->shm_file) ?
>                                 &shm_file_operations_huge :
>                                 &shm_file_operations);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               err = PTR_ERR(file);
>                 goto out_free;
> +       }
>
>         file->private_data = sfd;
>         file->f_mapping = shp->shm_file->f_mapping;
> diff --git a/mm/shmem.c b/mm/shmem.c
> index d4e184e..cd814ee 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2948,11 +2948,12 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
>                 goto put_dentry;
>  #endif
>
> -       error = -ENFILE;
>         file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
>                   &shmem_file_operations);
> -       if (!file)
> +       if (IS_ERR(file)) {
> +               error = PTR_ERR(file);
>                 goto put_dentry;
> +       }
>
>         return file;
>
> diff --git a/net/socket.c b/net/socket.c
> index edc3c4a..dc1d071 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -369,12 +369,12 @@ static int sock_alloc_file(struct socket *sock, struct file **f, int flags)
>
>         file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
>                   &socket_file_ops);
> -       if (unlikely(!file)) {
> +       if (unlikely(IS_ERR(file))) {
>                 /* drop dentry, keep inode */
>                 ihold(path.dentry->d_inode);
>                 path_put(&path);
>                 put_unused_fd(fd);
> -               return -ENFILE;
> +               return PTR_ERR(file);
>         }
>
>         sock->file = file;
> --
> 1.7.12
>

[PATCH] fs: Preserve error code in get_empty_filp()

[Anatol Pomozov]

Allocating a file structure in function get_empty_filp() might fail because
of several reasons:
 - not enough memory for file structures
 - operation is not allowed
 - user is over its limit

Currently the function returns NULL in all cases and we loose the exact
reason of the error. All callers of get_empty_filp() assume that the function
can fail with ENFILE only.

Return error through pointer. Change all callers to preserve this error code.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>
---
 arch/ia64/kernel/perfmon.c |  4 ++--
 fs/anon_inodes.c           |  5 +++--
 fs/file_table.c            | 22 ++++++++++++++--------
 fs/hugetlbfs/inode.c       |  5 +++--
 fs/namei.c                 |  4 ++--
 fs/open.c                  |  5 ++---
 fs/pipe.c                  |  9 ++++++---
 ipc/shm.c                  |  4 +++-
 mm/shmem.c                 |  5 +++--
 net/socket.c               |  4 ++--
 10 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
index 3fa4bc5..da2dcce 100644
--- a/arch/ia64/kernel/perfmon.c
+++ b/arch/ia64/kernel/perfmon.c
@@ -2221,9 +2221,9 @@ pfm_alloc_file(pfm_context_t *ctx)
 	d_add(path.dentry, inode);
 
 	file = alloc_file(&path, FMODE_READ, &pfm_file_ops);
-	if (!file) {
+	if (IS_ERR(file)) {
 		path_put(&path);
-		return ERR_PTR(-ENFILE);
+		return file;
 	}
 
 	file->f_flags = O_RDONLY;
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 28d39fb..73536e3 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -160,10 +160,11 @@ struct file *anon_inode_getfile(const char *name,
 
 	d_instantiate(path.dentry, anon_inode_inode);
 
-	error = -ENFILE;
 	file = alloc_file(&path, OPEN_FMODE(flags), fops);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto err_dput;
+	}
 	file->f_mapping = anon_inode_inode->i_mapping;
 
 	file->f_pos = 0;
diff --git a/fs/file_table.c b/fs/file_table.c
index 701985e..3b3f4d7 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -94,8 +94,8 @@ int proc_nr_files(ctl_table *table, int write,
 #endif
 
 /* Find an unused file structure and return a pointer to it.
- * Returns NULL, if there are no more free file structures or
- * we run out of memory.
+ * Returns an error pointer if some error happend e.g. we over file
+ * structures limit, run out of memory or operation is not permitted.
  *
  * Be very careful using this.  You are responsible for
  * getting write access to any mount that you might assign
@@ -108,6 +108,7 @@ struct file *get_empty_filp(void)
 	const struct cred *cred = current_cred();
 	static long old_max;
 	struct file * f;
+	int error;
 
 	/*
 	 * Privileged users can go above max_files
@@ -117,17 +118,22 @@ struct file *get_empty_filp(void)
 		 * percpu_counters are inaccurate.  Do an expensive check before
 		 * we go and fail.
 		 */
-		if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files)
+		if (percpu_counter_sum_positive(&nr_files) >= files_stat.max_files) {
+			error = -ENFILE;
 			goto over;
+		}
 	}
 
 	f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
-	if (f == NULL)
+	if (f == NULL) {
+		error = -ENOMEM;
 		goto fail;
+	}
 
 	percpu_counter_inc(&nr_files);
 	f->f_cred = get_cred(cred);
-	if (security_file_alloc(f))
+	error = security_file_alloc(f);
+	if (error)
 		goto fail_sec;
 
 	INIT_LIST_HEAD(&f->f_u.fu_list);
@@ -149,7 +155,7 @@ over:
 fail_sec:
 	file_free(f);
 fail:
-	return NULL;
+	return ERR_PTR(error);
 }
 
 /**
@@ -173,8 +179,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
 	struct file *file;
 
 	file = get_empty_filp();
-	if (!file)
-		return NULL;
+	if (IS_ERR(file))
+		return file;
 
 	file->f_path = *path;
 	file->f_mapping = path->dentry->d_inode->i_mapping;
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 8349a89..5ec849b 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -984,11 +984,12 @@ struct file *hugetlb_file_setup(const char *name, unsigned long addr,
 	inode->i_size = size;
 	clear_nlink(inode);
 
-	error = -ENFILE;
 	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 			&hugetlbfs_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto out_dentry; /* inode is already attached */
+	}
 
 	return file;
 
diff --git a/fs/namei.c b/fs/namei.c
index dd1ed1b..0160da0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2885,8 +2885,8 @@ static struct file *path_openat(int dfd, const char *pathname,
 	int error;
 
 	file = get_empty_filp();
-	if (!file)
-		return ERR_PTR(-ENFILE);
+	if (IS_ERR(file))
+		return file;
 
 	file->f_flags = op->open_flag;
 
diff --git a/fs/open.c b/fs/open.c
index e1f2cdb..cb9a385 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -781,10 +781,9 @@ struct file *dentry_open(const struct path *path, int flags,
 	/* We must always pass in a valid mount pointer. */
 	BUG_ON(!path->mnt);
 
-	error = -ENFILE;
 	f = get_empty_filp();
-	if (f == NULL)
-		return ERR_PTR(error);
+	if (IS_ERR(f))
+		return f;
 
 	f->f_flags = flags;
 	f->f_path = *path;
diff --git a/fs/pipe.c b/fs/pipe.c
index 8d85d70..e420908 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1035,16 +1035,19 @@ int create_pipe_files(struct file **res, int flags)
 
 	d_instantiate(path.dentry, inode);
 
-	err = -ENFILE;
 	f = alloc_file(&path, FMODE_WRITE, &write_pipefifo_fops);
-	if (!f)
+	if (IS_ERR(f)) {
+		err = PTR_ERR(f);
 		goto err_dentry;
+	}
 
 	f->f_flags = O_WRONLY | (flags & (O_NONBLOCK | O_DIRECT));
 
 	res[0] = alloc_file(&path, FMODE_READ, &read_pipefifo_fops);
-	if (!res[0])
+	if (IS_ERR(res[0])) {
+		err = PTR_ERR(res[0]);
 		goto err_file;
+	}
 
 	path_get(&path);
 	res[0]->f_flags = O_RDONLY | (flags & O_NONBLOCK);
diff --git a/ipc/shm.c b/ipc/shm.c
index 00faa05..c8eee21 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1039,8 +1039,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 			  is_file_hugepages(shp->shm_file) ?
 				&shm_file_operations_huge :
 				&shm_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		err = PTR_ERR(file);
 		goto out_free;
+	}
 
 	file->private_data = sfd;
 	file->f_mapping = shp->shm_file->f_mapping;
diff --git a/mm/shmem.c b/mm/shmem.c
index d4e184e..cd814ee 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2948,11 +2948,12 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
 		goto put_dentry;
 #endif
 
-	error = -ENFILE;
 	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 		  &shmem_file_operations);
-	if (!file)
+	if (IS_ERR(file)) {
+		error = PTR_ERR(file);
 		goto put_dentry;
+	}
 
 	return file;
 
diff --git a/net/socket.c b/net/socket.c
index edc3c4a..dc1d071 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -369,12 +369,12 @@ static int sock_alloc_file(struct socket *sock, struct file **f, int flags)
 
 	file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
 		  &socket_file_ops);
-	if (unlikely(!file)) {
+	if (unlikely(IS_ERR(file))) {
 		/* drop dentry, keep inode */
 		ihold(path.dentry->d_inode);
 		path_put(&path);
 		put_unused_fd(fd);
-		return -ENFILE;
+		return PTR_ERR(file);
 	}
 
 	sock->file = file;
-- 
1.7.12