Hi,
Adding more reviewers (found by get_maintainer.pl tool).
On Tue, Sep 4, 2012 at 2:15 PM, Anatol Pomozov <anatol.pomozov@xxxxxxxxx> wrote:
> 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@xxxxxxxxx> 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@xxxxxxxxx>
>> Reviewed-by: "Theodore Ts'o" <tytso@xxxxxxx>
>> ---
>> 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
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
