2012-08-01 18:19:34

by Anatol Pomozov

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

From: Anatol Pomazau <[email protected]>

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 <[email protected]>
Reviewed-by: "Theodore Ts'o" <[email protected]>
---
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


2012-08-01 18:34:47

by Anatol Pomozov

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

From: Anatol Pomazau <[email protected]>

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 <[email protected]>
Reviewed-by: "Theodore Ts'o" <[email protected]>
---
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

2012-08-03 00:30:19

by Anatol Pomozov

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

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, <[email protected]> wrote:
> From: Anatol Pomazau <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: "Theodore Ts'o" <[email protected]>
> ---
> 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
>

2012-08-03 00:47:38

by Anatol Pomozov

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

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 <[email protected]>
Reviewed-by: "Theodore Ts'o" <[email protected]>
---
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

2012-08-21 10:06:37

by Jan Engelhardt

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


On Wednesday 2012-08-01 20:19, [email protected] 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.

2012-08-21 13:52:03

by Anatol Pomozov

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

Hi

On Tue, Aug 21, 2012 at 3:06 AM, Jan Engelhardt <[email protected]> wrote:
>
>
> On Wednesday 2012-08-01 20:19, [email protected] 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.

2012-10-05 18:16:44

by Anatol Pomozov

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

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
<[email protected]> 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 <[email protected]>
> Reviewed-by: "Theodore Ts'o" <[email protected]>
> ---
> 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
>

2012-10-05 18:39:40

by Theodore Ts'o

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

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