[RFC][PATCH 0/4] Check O_* flags set with fcntl() on anon_inode files.

[RFC][PATCH 0/4] Check O_* flags set with fcntl() on anon_inode files.

[Matt Helsley]

anon_inode interfaces often do not support flags that can be set
by fcntl(). Right now using fcntl() to set these flags falsely
reports success for things like O_ASYNC yet SIGIO is not delivered.

I relied on the flags allowed by the syscalls that create
these files to determine the flags that are allowed to be set by
fcntl().

Each patch checks flags for one anonymous inode interface:

[PATCH 1/4] signalfd
[PATCH 2/4] timerfd
[PATCH 3/4] epoll
[PATCH 4/4] eventfd

I did not check the perf, kvm-vm, or kvm-vcpu uses of anon_inodes.

Cheers,
	-Matt Helsley

[RFC][PATCH 1/4] anon_inode fcntl() checks: report failure for fcntl(F_SETFL) on signalfd

[Matt Helsley]

anon_inode interfaces often do not support flags that can be set
by fcntl(). Right now using fcntl() to set these flags falsely
reports success for things like O_ASYNC (yet SIGIO is not delivered).

Report failure when userspace attempts to set unsupported flags
on signalfd files with fcntl().

Signed-off-by: Matt Helsley <matthltc@us.ibm.com>
Cc: Davide Libenzi <davidel@xmailserver.org>
---
 fs/signalfd.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/fs/signalfd.c b/fs/signalfd.c
index 1dabe4e..3016f3b 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -199,7 +199,19 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
 	return total ? total: ret;
 }
 
+static int signalfd_check_flags(int flags)
+{
+	/* Check the SFD_* constants for consistency.  */
+	BUILD_BUG_ON(SFD_CLOEXEC != O_CLOEXEC);
+	BUILD_BUG_ON(SFD_NONBLOCK != O_NONBLOCK);
+
+	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK))
+		return -EINVAL;
+	return 0;
+}
+
 static const struct file_operations signalfd_fops = {
+	.check_flags    = signalfd_check_flags,
 	.release	= signalfd_release,
 	.poll		= signalfd_poll,
 	.read		= signalfd_read,
@@ -211,13 +223,8 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 	sigset_t sigmask;
 	struct signalfd_ctx *ctx;
 
-	/* Check the SFD_* constants for consistency.  */
-	BUILD_BUG_ON(SFD_CLOEXEC != O_CLOEXEC);
-	BUILD_BUG_ON(SFD_NONBLOCK != O_NONBLOCK);
-
-	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK))
+	if (signalfd_check_flags(flags))
 		return -EINVAL;
-
 	if (sizemask != sizeof(sigset_t) ||
 	    copy_from_user(&sigmask, user_mask, sizeof(sigmask)))
 		return -EINVAL;
-- 
1.6.3.3

[RFC][PATCH 2/4] anon_inode fcntl() checks: report failure for fcntl(F_SETFL) on timerfd

[Matt Helsley]

Report failure when userspace attempts to set unsupported flags
on timerfd files with fcntl().

Signed-off-by: Matt Helsley <matthltc@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Davide Libenzi <davidel@xmailserver.org>
---
 fs/timerfd.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/timerfd.c b/fs/timerfd.c
index 1bfc95a..198a564 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -156,7 +156,19 @@ static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
 	return res;
 }
 
+static int timerfd_check_flags(int flags)
+{
+	/* Check the TFD_* constants for consistency.  */
+	BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
+	BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
+
+	if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
+		return -EINVAL;
+	return 0;
+}
+
 static const struct file_operations timerfd_fops = {
+	.check_flags    = timerfd_check_flags,
 	.release	= timerfd_release,
 	.poll		= timerfd_poll,
 	.read		= timerfd_read,
@@ -182,10 +194,6 @@ SYSCALL_DEFINE2(timerfd_create, int, clockid, int, flags)
 	int ufd;
 	struct timerfd_ctx *ctx;
 
-	/* Check the TFD_* constants for consistency.  */
-	BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
-	BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
-
 	if ((flags & ~TFD_CREATE_FLAGS) ||
 	    (clockid != CLOCK_MONOTONIC &&
 	     clockid != CLOCK_REALTIME))
-- 
1.6.3.3

[RFC][PATCH 3/4] anon_inode fcntl() checks: report failure for fcntl(F_SETFL) on epoll

[Matt Helsley]

Report failure when userspace attempts to set unsupported flags
on epoll files with fcntl().

Signed-off-by: Matt Helsley <matthltc@us.ibm.com>
Cc: Davide Libenzi <davidel@xmailserver.org>
---
 fs/eventpoll.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index bd056a5..ea46b92 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -671,8 +671,19 @@ static unsigned int ep_eventpoll_poll(struct file *file, poll_table *wait)
 	return pollflags != -1 ? pollflags : 0;
 }
 
+static int ep_eventpoll_check_flags(int flags)
+{
+	/* Check the EPOLL_* constant for consistency.  */
+	BUILD_BUG_ON(EPOLL_CLOEXEC != O_CLOEXEC);
+
+	if (flags & ~EPOLL_CLOEXEC)
+		return -EINVAL;
+	return 0;
+}
+
 /* File callbacks that implement the eventpoll file behaviour */
 static const struct file_operations eventpoll_fops = {
+	.check_flags    = ep_eventpoll_check_flags,
 	.release	= ep_eventpoll_release,
 	.poll		= ep_eventpoll_poll
 };
@@ -1190,11 +1201,9 @@ SYSCALL_DEFINE1(epoll_create1, int, flags)
 	int error;
 	struct eventpoll *ep = NULL;
 
-	/* Check the EPOLL_* constant for consistency.  */
-	BUILD_BUG_ON(EPOLL_CLOEXEC != O_CLOEXEC);
-
-	if (flags & ~EPOLL_CLOEXEC)
+	if (ep_eventpoll_check_flags(flags))
 		return -EINVAL;
+
 	/*
 	 * Create the internal data structure ("struct eventpoll").
 	 */
-- 
1.6.3.3

[RFC][PATCH 4/4] anon_inode fcntl() checks: report failure for fcntl(F_SETFL) on eventfd

[Matt Helsley]

Report failure when userspace attempts to set unsupported flags
on eventfd files with fcntl().

Signed-off-by: Matt Helsley <matthltc@us.ibm.com>
Cc: Davide Libenzi <davidel@xmailserver.org>
---
 fs/eventfd.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/eventfd.c b/fs/eventfd.c
index 7758cc3..8976079 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -287,7 +287,19 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c
 	return res;
 }
 
+static int eventfd_check_flags(int flags)
+{
+	/* Check the EFD_* constants for consistency.  */
+	BUILD_BUG_ON(EFD_CLOEXEC != O_CLOEXEC);
+	BUILD_BUG_ON(EFD_NONBLOCK != O_NONBLOCK);
+
+	if (flags & ~EFD_SHARED_FCNTL_FLAGS)
+		return -EINVAL;
+	return 0;
+}
+
 static const struct file_operations eventfd_fops = {
+	.check_flags    = eventfd_check_flags,
 	.release	= eventfd_release,
 	.poll		= eventfd_poll,
 	.read		= eventfd_read,
@@ -381,9 +393,6 @@ struct file *eventfd_file_create(unsigned int count, int flags)
 	struct file *file;
 	struct eventfd_ctx *ctx;
 
-	/* Check the EFD_* constants for consistency.  */
-	BUILD_BUG_ON(EFD_CLOEXEC != O_CLOEXEC);
-	BUILD_BUG_ON(EFD_NONBLOCK != O_NONBLOCK);
 
 	if (flags & ~EFD_FLAGS_SET)
 		return ERR_PTR(-EINVAL);
-- 
1.6.3.3

Re: [RFC][PATCH 0/4] Check O_* flags set with fcntl() on anon_inode files.

[Davide Libenzi]

On Sat, 13 Feb 2010, Matt Helsley wrote:

> anon_inode interfaces often do not support flags that can be set
> by fcntl(). Right now using fcntl() to set these flags falsely
> reports success for things like O_ASYNC yet SIGIO is not delivered.
> 
> I relied on the flags allowed by the syscalls that create
> these files to determine the flags that are allowed to be set by
> fcntl().
> 
> Each patch checks flags for one anonymous inode interface:
> 
> [PATCH 1/4] signalfd
> [PATCH 2/4] timerfd
> [PATCH 3/4] epoll
> [PATCH 4/4] eventfd

The whole serie ...

Acked-by: Davide Libenzi <davidel@xmailserver.org>


- Davide

Re: [RFC][PATCH 0/4] Check O_* flags set with fcntl() on anon_inode files.

[Al Viro]

On Sat, Feb 13, 2010 at 04:27:43PM -0800, Matt Helsley wrote:
> anon_inode interfaces often do not support flags that can be set
> by fcntl(). Right now using fcntl() to set these flags falsely
> reports success for things like O_ASYNC yet SIGIO is not delivered.
> 
> I relied on the flags allowed by the syscalls that create
> these files to determine the flags that are allowed to be set by
> fcntl().
> 
> Each patch checks flags for one anonymous inode interface:
> 
> [PATCH 1/4] signalfd
> [PATCH 2/4] timerfd
> [PATCH 3/4] epoll
> [PATCH 4/4] eventfd
> 
> I did not check the perf, kvm-vm, or kvm-vcpu uses of anon_inodes.

Er...  O_ASYNC is silently ignored for regular files as well, so any
userland code that tries to rely on fcntl() rejecting it is and always
had been badly b0rken.

Re: [RFC][PATCH 0/4] Check O_* flags set with fcntl() on anon_inode files.

[Matt Helsley]

On Sun, Feb 14, 2010 at 03:52:05AM +0000, Al Viro wrote:
> On Sat, Feb 13, 2010 at 04:27:43PM -0800, Matt Helsley wrote:
> > anon_inode interfaces often do not support flags that can be set
> > by fcntl(). Right now using fcntl() to set these flags falsely
> > reports success for things like O_ASYNC yet SIGIO is not delivered.
> > 
> > I relied on the flags allowed by the syscalls that create
> > these files to determine the flags that are allowed to be set by
> > fcntl().
> > 
> > Each patch checks flags for one anonymous inode interface:
> > 
> > [PATCH 1/4] signalfd
> > [PATCH 2/4] timerfd
> > [PATCH 3/4] epoll
> > [PATCH 4/4] eventfd
> > 
> > I did not check the perf, kvm-vm, or kvm-vcpu uses of anon_inodes.
> 
> Er...  O_ASYNC is silently ignored for regular files as well, so any
> userland code that tries to rely on fcntl() rejecting it is and always
> had been badly b0rken.

Of course. Did you mean to imply that the kernel shouldn't bother to
reject these, or were you merely making an observation?

Cheers,
	-Matt Helsley

Re: [RFC][PATCH 0/4] Check O_* flags set with fcntl() on anon_inode files.

[Al Viro]

On Mon, Feb 15, 2010 at 09:26:35AM -0800, Matt Helsley wrote:
> > > [PATCH 1/4] signalfd
> > > [PATCH 2/4] timerfd
> > > [PATCH 3/4] epoll
> > > [PATCH 4/4] eventfd
> > > 
> > > I did not check the perf, kvm-vm, or kvm-vcpu uses of anon_inodes.
> > 
> > Er...  O_ASYNC is silently ignored for regular files as well, so any
> > userland code that tries to rely on fcntl() rejecting it is and always
> > had been badly b0rken.
> 
> Of course. Did you mean to imply that the kernel shouldn't bother to
> reject these, or were you merely making an observation?

I'm wondering why should we start changing that behaviour and what makes
these 4 cases special?

Re: [RFC][PATCH 0/4] Check O_* flags set with fcntl() on anon_inode files.

[Matt Helsley]

On Mon, Feb 15, 2010 at 07:57:28PM +0000, Al Viro wrote:
> On Mon, Feb 15, 2010 at 09:26:35AM -0800, Matt Helsley wrote:
> > > > [PATCH 1/4] signalfd
> > > > [PATCH 2/4] timerfd
> > > > [PATCH 3/4] epoll
> > > > [PATCH 4/4] eventfd
> > > > 
> > > > I did not check the perf, kvm-vm, or kvm-vcpu uses of anon_inodes.
> > > 
> > > Er...  O_ASYNC is silently ignored for regular files as well, so any
> > > userland code that tries to rely on fcntl() rejecting it is and always
> > > had been badly b0rken.
> > 
> > Of course. Did you mean to imply that the kernel shouldn't bother to
> > reject these, or were you merely making an observation?
> 
> I'm wondering why should we start changing that behaviour and what makes
> these 4 cases special?

I'm not saying we should change behavior for regular files. We should check
these because they're already being checked inside the special syscalls that
open these files. Without these patches fcntl(F_SETFL) is a way around those
checks for these interfaces.

Cheers,
	-Matt Helsley