LinuxLists.cc - [PATCH] anonfd: Make file read-only if fops->write is not set

2009-12-18 05:46:14

Subject: [PATCH] anonfd: Make file read-only if fops->write is not set

2009-12-18 06:10:55

Subject: Re: [PATCH] anonfd: Make file read-only if fops->write is not set

On Thu, Dec 17, 2009 at 09:46:11PM -0800, Roland Dreier wrote:
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 2c99459..097f91f 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -121,13 +121,13 @@ struct file *anon_inode_getfile(const char *name,
> d_instantiate(path.dentry, anon_inode_inode);
>
> error = -ENFILE;
> - file = alloc_file(&path, FMODE_READ | FMODE_WRITE, fops);
> + file = alloc_file(&path, FMODE_READ | (fops->write ? FMODE_WRITE : 0), fops);

Good grief, what for? We are already passing O_NDELAY in flags argument,
why not add O_RDWR/O_RDONLY to it? It's not as if we had that many
callers, after all...

2009-12-18 17:12:37

by Davide Libenzi

[permalink] [raw]

Subject: Re: [PATCH] anonfd: Make file read-only if fops->write is not set

On Fri, 18 Dec 2009, Al Viro wrote:

> On Thu, Dec 17, 2009 at 09:46:11PM -0800, Roland Dreier wrote:
> > diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> > index 2c99459..097f91f 100644
> > --- a/fs/anon_inodes.c
> > +++ b/fs/anon_inodes.c
> > @@ -121,13 +121,13 @@ struct file *anon_inode_getfile(const char *name,
> > d_instantiate(path.dentry, anon_inode_inode);
> >
> > error = -ENFILE;
> > - file = alloc_file(&path, FMODE_READ | FMODE_WRITE, fops);
> > + file = alloc_file(&path, FMODE_READ | (fops->write ? FMODE_WRITE : 0), fops);
>
> Good grief, what for? We are already passing O_NDELAY in flags argument,
> why not add O_RDWR/O_RDONLY to it? It's not as if we had that many
> callers, after all...

I agree. We're better off explicitly passing the flags instead of doing
some detection magic.

- Davide

2009-12-18 17:41:28

by Roland Dreier

[permalink] [raw]

Subject: [PATCH v2] anonfd: Make file read-only if fops->write is not set

It seems a couple places such as arch/ia64/kernel/perfmon.c and
drivers/infiniband/core/uverbs_main.c could use anon_inode_getfile()
instead of a private pseudo-fs + alloc_file(), if only there were a way
to get a read-only file. So provide this by having anon_inode_getfile()
create a read-only file if the fops->write passed in is NULL.

Signed-off-by: Roland Dreier <[email protected]>
---
> Good grief, what for? We are already passing O_NDELAY in flags argument,
> why not add O_RDWR/O_RDONLY to it? It's not as if we had that many
> callers, after all...

fair enough, I wasn't sure if I really liked changing the meaning of the
flags argument without any indication of that in the function signature,
but definitely the code is nicer.

so would something like this make sense? if so I'll send follow-ups
converting ia64 / perfmon and infiniband / uverbs to anon_inodes.

fs/anon_inodes.c | 12 ++++++++++--
fs/eventfd.c | 2 +-
fs/eventpoll.c | 2 +-
fs/signalfd.c | 2 +-
fs/timerfd.c | 2 +-
kernel/perf_event.c | 2 +-
virt/kvm/kvm_main.c | 4 ++--
7 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 2c99459..598237e 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -89,11 +89,19 @@ struct file *anon_inode_getfile(const char *name,
struct qstr this;
struct path path;
struct file *file;
+ fmode_t mode;
int error;

if (IS_ERR(anon_inode_inode))
return ERR_PTR(-ENODEV);

+ switch (flags & O_ACCMODE) {
+ case O_RDONLY: mode = FMODE_READ; break;
+ case O_WRONLY: mode = FMODE_WRITE; break;
+ case O_RDWR: mode = FMODE_READ | FMODE_WRITE; break;
+ default: return ERR_PTR(-EINVAL);
+ }
+
if (fops->owner && !try_module_get(fops->owner))
return ERR_PTR(-ENOENT);

@@ -121,13 +129,13 @@ struct file *anon_inode_getfile(const char *name,
d_instantiate(path.dentry, anon_inode_inode);

error = -ENFILE;
- file = alloc_file(&path, FMODE_READ | FMODE_WRITE, fops);
+ file = alloc_file(&path, mode, fops);
if (!file)
goto err_dput;
file->f_mapping = anon_inode_inode->i_mapping;

file->f_pos = 0;
- file->f_flags = O_RDWR | (flags & O_NONBLOCK);
+ file->f_flags = flags & (O_ACCMODE | O_NONBLOCK);
file->f_version = 0;
file->private_data = priv;

diff --git a/fs/eventfd.c b/fs/eventfd.c
index 8b47e42..d26402f 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -339,7 +339,7 @@ struct file *eventfd_file_create(unsigned int count, int flags)
ctx->flags = flags;

file = anon_inode_getfile("[eventfd]", &eventfd_fops, ctx,
- flags & EFD_SHARED_FCNTL_FLAGS);
+ O_RDWR | (flags & EFD_SHARED_FCNTL_FLAGS));
if (IS_ERR(file))
eventfd_free_ctx(ctx);

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 366c503..bd056a5 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1206,7 +1206,7 @@ SYSCALL_DEFINE1(epoll_create1, int, flags)
* a file structure and a free file descriptor.
*/
error = anon_inode_getfd("[eventpoll]", &eventpoll_fops, ep,
- flags & O_CLOEXEC);
+ O_RDWR | (flags & O_CLOEXEC));
if (error < 0)
ep_free(ep);

diff --git a/fs/signalfd.c b/fs/signalfd.c
index b07565c..1dabe4e 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -236,7 +236,7 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
* anon_inode_getfd() will install the fd.
*/
ufd = anon_inode_getfd("[signalfd]", &signalfd_fops, ctx,
- flags & (O_CLOEXEC | O_NONBLOCK));
+ O_RDWR | (flags & (O_CLOEXEC | O_NONBLOCK)));
if (ufd < 0)
kfree(ctx);
} else {
diff --git a/fs/timerfd.c b/fs/timerfd.c
index b042bd7..1bfc95a 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -200,7 +200,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clockid, int, flags)
hrtimer_init(&ctx->tmr, clockid, HRTIMER_MODE_ABS);

ufd = anon_inode_getfd("[timerfd]", &timerfd_fops, ctx,
- flags & TFD_SHARED_FCNTL_FLAGS);
+ O_RDWR | (flags & TFD_SHARED_FCNTL_FLAGS));
if (ufd < 0)
kfree(ctx);

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 97d1a3d..67eed80 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -4720,7 +4720,7 @@ SYSCALL_DEFINE5(perf_event_open,
if (IS_ERR(event))
goto err_put_context;

- err = anon_inode_getfd("[perf_event]", &perf_fops, event, 0);
+ err = anon_inode_getfd("[perf_event]", &perf_fops, event, O_RDWR);
if (err < 0)
goto err_free_put_context;

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e1f2bf8..b5af881 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1177,7 +1177,7 @@ static struct file_operations kvm_vcpu_fops = {
*/
static int create_vcpu_fd(struct kvm_vcpu *vcpu)
{
- return anon_inode_getfd("kvm-vcpu", &kvm_vcpu_fops, vcpu, 0);
+ return anon_inode_getfd("kvm-vcpu", &kvm_vcpu_fops, vcpu, O_RDWR);
}

/*
@@ -1638,7 +1638,7 @@ static int kvm_dev_ioctl_create_vm(void)
kvm = kvm_create_vm();
if (IS_ERR(kvm))
return PTR_ERR(kvm);
- fd = anon_inode_getfd("kvm-vm", &kvm_vm_fops, kvm, 0);
+ fd = anon_inode_getfd("kvm-vm", &kvm_vm_fops, kvm, O_RDWR);
if (fd < 0)
kvm_put_kvm(kvm);

2009-12-18 18:59:07

by Davide Libenzi

[permalink] [raw]

Subject: Re: [PATCH v2] anonfd: Make file read-only if fops->write is not set

On Fri, 18 Dec 2009, Roland Dreier wrote:

> It seems a couple places such as arch/ia64/kernel/perfmon.c and
> drivers/infiniband/core/uverbs_main.c could use anon_inode_getfile()
> instead of a private pseudo-fs + alloc_file(), if only there were a way
> to get a read-only file. So provide this by having anon_inode_getfile()
> create a read-only file if the fops->write passed in is NULL.
>
> Signed-off-by: Roland Dreier <[email protected]>
> ---
> > Good grief, what for? We are already passing O_NDELAY in flags argument,
> > why not add O_RDWR/O_RDONLY to it? It's not as if we had that many
> > callers, after all...
>
> fair enough, I wasn't sure if I really liked changing the meaning of the
> flags argument without any indication of that in the function signature,
> but definitely the code is nicer.
>
> so would something like this make sense? if so I'll send follow-ups
> converting ia64 / perfmon and infiniband / uverbs to anon_inodes.

Looks OK to me.

- Davide

2009-12-18 19:14:15

by Al Viro

[permalink] [raw]

Subject: Re: [PATCH v2] anonfd: Make file read-only if fops->write is not set

On Fri, Dec 18, 2009 at 10:59:00AM -0800, Davide Libenzi wrote:
> > so would something like this make sense? if so I'll send follow-ups
> > converting ia64 / perfmon and infiniband / uverbs to anon_inodes.
>
> Looks OK to me.

fmode_t calculation looks ugly; I'll update and put into VFS tree