* Re: [PATCH] anonfd: Make file read-only if fops->write is not set
2009-12-18 6:10 ` Al Viro
@ 2009-12-18 17:12 ` Davide Libenzi
2009-12-18 17:41 ` [PATCH v2] " Roland Dreier
1 sibling, 0 replies; 6+ messages in thread
From: Davide Libenzi @ 2009-12-18 17:12 UTC (permalink / raw)
To: Al Viro; +Cc: Roland Dreier, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] anonfd: Make file read-only if fops->write is not set
2009-12-18 6:10 ` Al Viro
2009-12-18 17:12 ` Davide Libenzi
@ 2009-12-18 17:41 ` Roland Dreier
2009-12-18 18:59 ` Davide Libenzi
1 sibling, 1 reply; 6+ messages in thread
From: Roland Dreier @ 2009-12-18 17:41 UTC (permalink / raw)
To: Al Viro; +Cc: Davide Libenzi, linux-kernel
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 <rolandd@cisco.com>
---
> 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);
^ permalink raw reply related [flat|nested] 6+ messages in thread