LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH] timerfd: Protect the might cancel mechanism proper
@ 2017-01-31 14:24 Thomas Gleixner
  2017-02-01 12:43 ` Dmitry Vyukov
  2017-02-10 10:19 ` [tip:timers/core] " tip-bot for Thomas Gleixner
  0 siblings, 2 replies; 8+ messages in thread
From: Thomas Gleixner @ 2017-01-31 14:24 UTC (permalink / raw)
  To: Dmitry Vyukov; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller

The handling of the might_cancel queueing is not properly protected, so
parallel operations on the file descriptor can race with each other and
lead to list corruptions or use after free.

Protect the context for these operations with a seperate lock.

The wait queue lock cannot be reused for this because that would create a
lock inversion scenario vs. the cancel lock. Replacing might_cancel with an
atomic (atomic_t or atomic bit) does not help either because it still can
race vs. the actual list operation.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 fs/timerfd.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -40,6 +40,7 @@ struct timerfd_ctx {
 	short unsigned settime_flags;	/* to show in fdinfo */
 	struct rcu_head rcu;
 	struct list_head clist;
+	spinlock_t cancel_lock;
 	bool might_cancel;
 };
 
@@ -112,7 +113,7 @@ void timerfd_clock_was_set(void)
 	rcu_read_unlock();
 }
 
-static void timerfd_remove_cancel(struct timerfd_ctx *ctx)
+static void __timerfd_remove_cancel(struct timerfd_ctx *ctx)
 {
 	if (ctx->might_cancel) {
 		ctx->might_cancel = false;
@@ -122,6 +123,13 @@ static void timerfd_remove_cancel(struct
 	}
 }
 
+static void timerfd_remove_cancel(struct timerfd_ctx *ctx)
+{
+	spin_lock(&ctx->cancel_lock);
+	__timerfd_remove_cancel(ctx);
+	spin_unlock(&ctx->cancel_lock);
+}
+
 static bool timerfd_canceled(struct timerfd_ctx *ctx)
 {
 	if (!ctx->might_cancel || ctx->moffs != KTIME_MAX)
@@ -132,6 +140,7 @@ static bool timerfd_canceled(struct time
 
 static void timerfd_setup_cancel(struct timerfd_ctx *ctx, int flags)
 {
+	spin_lock(&ctx->cancel_lock);
 	if ((ctx->clockid == CLOCK_REALTIME ||
 	     ctx->clockid == CLOCK_REALTIME_ALARM) &&
 	    (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) {
@@ -141,9 +150,10 @@ static void timerfd_setup_cancel(struct
 			list_add_rcu(&ctx->clist, &cancel_list);
 			spin_unlock(&cancel_lock);
 		}
-	} else if (ctx->might_cancel) {
-		timerfd_remove_cancel(ctx);
+	} else {
+		__timerfd_remove_cancel(ctx);
 	}
+	spin_unlock(&ctx->cancel_lock);
 }
 
 static ktime_t timerfd_get_remaining(struct timerfd_ctx *ctx)
@@ -400,6 +410,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
 		return -ENOMEM;
 
 	init_waitqueue_head(&ctx->wqh);
+	spin_lock_init(&ctx->cancel_lock);
 	ctx->clockid = clockid;
 
 	if (isalarm(ctx))

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] timerfd: Protect the might cancel mechanism proper
  2017-01-31 14:24 [PATCH] timerfd: Protect the might cancel mechanism proper Thomas Gleixner
@ 2017-02-01 12:43 ` Dmitry Vyukov
  2017-02-02 18:54   ` Thomas Gleixner
  2017-02-10 10:19 ` [tip:timers/core] " tip-bot for Thomas Gleixner
  1 sibling, 1 reply; 8+ messages in thread
From: Dmitry Vyukov @ 2017-02-01 12:43 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller

On Tue, Jan 31, 2017 at 3:24 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> The handling of the might_cancel queueing is not properly protected, so
> parallel operations on the file descriptor can race with each other and
> lead to list corruptions or use after free.
>
> Protect the context for these operations with a seperate lock.
>
> The wait queue lock cannot be reused for this because that would create a
> lock inversion scenario vs. the cancel lock. Replacing might_cancel with an
> atomic (atomic_t or atomic bit) does not help either because it still can
> race vs. the actual list operation.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  fs/timerfd.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> --- a/fs/timerfd.c
> +++ b/fs/timerfd.c
> @@ -40,6 +40,7 @@ struct timerfd_ctx {
>         short unsigned settime_flags;   /* to show in fdinfo */
>         struct rcu_head rcu;
>         struct list_head clist;
> +       spinlock_t cancel_lock;
>         bool might_cancel;
>  };
>
> @@ -112,7 +113,7 @@ void timerfd_clock_was_set(void)
>         rcu_read_unlock();
>  }
>
> -static void timerfd_remove_cancel(struct timerfd_ctx *ctx)
> +static void __timerfd_remove_cancel(struct timerfd_ctx *ctx)
>  {
>         if (ctx->might_cancel) {
>                 ctx->might_cancel = false;
> @@ -122,6 +123,13 @@ static void timerfd_remove_cancel(struct
>         }
>  }
>
> +static void timerfd_remove_cancel(struct timerfd_ctx *ctx)
> +{
> +       spin_lock(&ctx->cancel_lock);
> +       __timerfd_remove_cancel(ctx);
> +       spin_unlock(&ctx->cancel_lock);
> +}
> +
>  static bool timerfd_canceled(struct timerfd_ctx *ctx)
>  {
>         if (!ctx->might_cancel || ctx->moffs != KTIME_MAX)
> @@ -132,6 +140,7 @@ static bool timerfd_canceled(struct time
>
>  static void timerfd_setup_cancel(struct timerfd_ctx *ctx, int flags)
>  {
> +       spin_lock(&ctx->cancel_lock);
>         if ((ctx->clockid == CLOCK_REALTIME ||
>              ctx->clockid == CLOCK_REALTIME_ALARM) &&
>             (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) {
> @@ -141,9 +150,10 @@ static void timerfd_setup_cancel(struct
>                         list_add_rcu(&ctx->clist, &cancel_list);
>                         spin_unlock(&cancel_lock);
>                 }
> -       } else if (ctx->might_cancel) {
> -               timerfd_remove_cancel(ctx);
> +       } else {
> +               __timerfd_remove_cancel(ctx);
>         }
> +       spin_unlock(&ctx->cancel_lock);
>  }
>
>  static ktime_t timerfd_get_remaining(struct timerfd_ctx *ctx)
> @@ -400,6 +410,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>                 return -ENOMEM;
>
>         init_waitqueue_head(&ctx->wqh);
> +       spin_lock_init(&ctx->cancel_lock);
>         ctx->clockid = clockid;
>
>         if (isalarm(ctx))


Can't we still end up with an inconsistently setup timer?
do_timerfd_settime executes timerfd_setup_cancel and timerfd_setup as
two separate non-atomic actions. So if there are 2 concurrent
timerfd_settime calls, one that needs cancel and another that does not
need cancel, can't we end up with inconsistent setup? E.g. setup timer
that needs cancel, but it won't be in cancel_list. Or vice versa.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] timerfd: Protect the might cancel mechanism proper
  2017-02-01 12:43 ` Dmitry Vyukov
@ 2017-02-02 18:54   ` Thomas Gleixner
  2017-02-02 19:04     ` Dmitry Vyukov
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2017-02-02 18:54 UTC (permalink / raw)
  To: Dmitry Vyukov; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller

On Wed, 1 Feb 2017, Dmitry Vyukov wrote:
> 
> Can't we still end up with an inconsistently setup timer?
> do_timerfd_settime executes timerfd_setup_cancel and timerfd_setup as
> two separate non-atomic actions. So if there are 2 concurrent
> timerfd_settime calls, one that needs cancel and another that does not
> need cancel, can't we end up with inconsistent setup? E.g. setup timer
> that needs cancel, but it won't be in cancel_list. Or vice versa.

Do we really care? If an application arms the timer with cancel in one
thread and the same timer without cancel in another thread, then it's
probably completely irrelevant whether the state pair timeout/cancel is
correct or not. That's clearly an application bug and I don't want to add
more locking just to make something which is broken by definition pseudo
'atomic'.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] timerfd: Protect the might cancel mechanism proper
  2017-02-02 18:54   ` Thomas Gleixner
@ 2017-02-02 19:04     ` Dmitry Vyukov
  2017-02-10 10:13       ` Thomas Gleixner
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Vyukov @ 2017-02-02 19:04 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller

On Thu, Feb 2, 2017 at 7:54 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 1 Feb 2017, Dmitry Vyukov wrote:
>>
>> Can't we still end up with an inconsistently setup timer?
>> do_timerfd_settime executes timerfd_setup_cancel and timerfd_setup as
>> two separate non-atomic actions. So if there are 2 concurrent
>> timerfd_settime calls, one that needs cancel and another that does not
>> need cancel, can't we end up with inconsistent setup? E.g. setup timer
>> that needs cancel, but it won't be in cancel_list. Or vice versa.
>
> Do we really care? If an application arms the timer with cancel in one
> thread and the same timer without cancel in another thread, then it's
> probably completely irrelevant whether the state pair timeout/cancel is
> correct or not. That's clearly an application bug and I don't want to add
> more locking just to make something which is broken by definition pseudo
> 'atomic'.

I agree that the program is bogus, and don't have to ensure any sane
behavior for it. But I am concerned about potential kernel corruptions
due to this. For example, maybe kernel code will decide to not remove
such timer from the cancel list on destruction because based on
clockid/flags it should not be in the cancel list, but the timer is
actually there and we will end up with a leak or a dangling pointer. I
did not check that this actually happens, such inconsistent state just
looks like a red flag for me.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] timerfd: Protect the might cancel mechanism proper
  2017-02-02 19:04     ` Dmitry Vyukov
@ 2017-02-10 10:13       ` Thomas Gleixner
  2017-02-10 10:26         ` Dmitry Vyukov
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2017-02-10 10:13 UTC (permalink / raw)
  To: Dmitry Vyukov; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller

Dmitry,

On Thu, 2 Feb 2017, Dmitry Vyukov wrote:

> On Thu, Feb 2, 2017 at 7:54 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Wed, 1 Feb 2017, Dmitry Vyukov wrote:
> >>
> >> Can't we still end up with an inconsistently setup timer?
> >> do_timerfd_settime executes timerfd_setup_cancel and timerfd_setup as
> >> two separate non-atomic actions. So if there are 2 concurrent
> >> timerfd_settime calls, one that needs cancel and another that does not
> >> need cancel, can't we end up with inconsistent setup? E.g. setup timer
> >> that needs cancel, but it won't be in cancel_list. Or vice versa.
> >
> > Do we really care? If an application arms the timer with cancel in one
> > thread and the same timer without cancel in another thread, then it's
> > probably completely irrelevant whether the state pair timeout/cancel is
> > correct or not. That's clearly an application bug and I don't want to add
> > more locking just to make something which is broken by definition pseudo
> > 'atomic'.
> 
> I agree that the program is bogus, and don't have to ensure any sane
> behavior for it. But I am concerned about potential kernel corruptions
> due to this. For example, maybe kernel code will decide to not remove
> such timer from the cancel list on destruction because based on
> clockid/flags it should not be in the cancel list, but the timer is
> actually there and we will end up with a leak or a dangling pointer. I
> did not check that this actually happens, such inconsistent state just
> looks like a red flag for me.

That can't happen.

ctx->might_cancel and ctx->clist are always in sync with the new lock and
that's the only interesting thing. On destruction we don't look at clockid
or such, we only care about might_cancel.

What is not guaranteed to be in sync is the timer expiry time and the
cancel stuff, if two threads operate on the same timerfd in
parallel. That's what I do not care about at all.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [tip:timers/core] timerfd: Protect the might cancel mechanism proper
  2017-01-31 14:24 [PATCH] timerfd: Protect the might cancel mechanism proper Thomas Gleixner
  2017-02-01 12:43 ` Dmitry Vyukov
@ 2017-02-10 10:19 ` " tip-bot for Thomas Gleixner
  1 sibling, 0 replies; 8+ messages in thread
From: tip-bot for Thomas Gleixner @ 2017-02-10 10:19 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: hpa, viro, dvyukov, syzkaller, linux-kernel, tglx, mingo

Commit-ID:  1e38da300e1e395a15048b0af1e5305bd91402f6
Gitweb:     http://git.kernel.org/tip/1e38da300e1e395a15048b0af1e5305bd91402f6
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 31 Jan 2017 15:24:03 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 10 Feb 2017 11:15:09 +0100

timerfd: Protect the might cancel mechanism proper

The handling of the might_cancel queueing is not properly protected, so
parallel operations on the file descriptor can race with each other and
lead to list corruptions or use after free.

Protect the context for these operations with a seperate lock.

The wait queue lock cannot be reused for this because that would create a
lock inversion scenario vs. the cancel lock. Replacing might_cancel with an
atomic (atomic_t or atomic bit) does not help either because it still can
race vs. the actual list operation.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: "linux-fsdevel@vger.kernel.org"
Cc: syzkaller <syzkaller@googlegroups.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Link: http://lkml.kernel.org/r/alpine.DEB.2.20.1701311521430.3457@nanos
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 fs/timerfd.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/timerfd.c b/fs/timerfd.c
index c173cc1..384fa75 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -40,6 +40,7 @@ struct timerfd_ctx {
 	short unsigned settime_flags;	/* to show in fdinfo */
 	struct rcu_head rcu;
 	struct list_head clist;
+	spinlock_t cancel_lock;
 	bool might_cancel;
 };
 
@@ -112,7 +113,7 @@ void timerfd_clock_was_set(void)
 	rcu_read_unlock();
 }
 
-static void timerfd_remove_cancel(struct timerfd_ctx *ctx)
+static void __timerfd_remove_cancel(struct timerfd_ctx *ctx)
 {
 	if (ctx->might_cancel) {
 		ctx->might_cancel = false;
@@ -122,6 +123,13 @@ static void timerfd_remove_cancel(struct timerfd_ctx *ctx)
 	}
 }
 
+static void timerfd_remove_cancel(struct timerfd_ctx *ctx)
+{
+	spin_lock(&ctx->cancel_lock);
+	__timerfd_remove_cancel(ctx);
+	spin_unlock(&ctx->cancel_lock);
+}
+
 static bool timerfd_canceled(struct timerfd_ctx *ctx)
 {
 	if (!ctx->might_cancel || ctx->moffs != KTIME_MAX)
@@ -132,6 +140,7 @@ static bool timerfd_canceled(struct timerfd_ctx *ctx)
 
 static void timerfd_setup_cancel(struct timerfd_ctx *ctx, int flags)
 {
+	spin_lock(&ctx->cancel_lock);
 	if ((ctx->clockid == CLOCK_REALTIME ||
 	     ctx->clockid == CLOCK_REALTIME_ALARM) &&
 	    (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) {
@@ -141,9 +150,10 @@ static void timerfd_setup_cancel(struct timerfd_ctx *ctx, int flags)
 			list_add_rcu(&ctx->clist, &cancel_list);
 			spin_unlock(&cancel_lock);
 		}
-	} else if (ctx->might_cancel) {
-		timerfd_remove_cancel(ctx);
+	} else {
+		__timerfd_remove_cancel(ctx);
 	}
+	spin_unlock(&ctx->cancel_lock);
 }
 
 static ktime_t timerfd_get_remaining(struct timerfd_ctx *ctx)
@@ -400,6 +410,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clockid, int, flags)
 		return -ENOMEM;
 
 	init_waitqueue_head(&ctx->wqh);
+	spin_lock_init(&ctx->cancel_lock);
 	ctx->clockid = clockid;
 
 	if (isalarm(ctx))

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] timerfd: Protect the might cancel mechanism proper
  2017-02-10 10:13       ` Thomas Gleixner
@ 2017-02-10 10:26         ` Dmitry Vyukov
  2017-02-10 10:51           ` Thomas Gleixner
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Vyukov @ 2017-02-10 10:26 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller

On Fri, Feb 10, 2017 at 11:13 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Dmitry,
>
> On Thu, 2 Feb 2017, Dmitry Vyukov wrote:
>
>> On Thu, Feb 2, 2017 at 7:54 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Wed, 1 Feb 2017, Dmitry Vyukov wrote:
>> >>
>> >> Can't we still end up with an inconsistently setup timer?
>> >> do_timerfd_settime executes timerfd_setup_cancel and timerfd_setup as
>> >> two separate non-atomic actions. So if there are 2 concurrent
>> >> timerfd_settime calls, one that needs cancel and another that does not
>> >> need cancel, can't we end up with inconsistent setup? E.g. setup timer
>> >> that needs cancel, but it won't be in cancel_list. Or vice versa.
>> >
>> > Do we really care? If an application arms the timer with cancel in one
>> > thread and the same timer without cancel in another thread, then it's
>> > probably completely irrelevant whether the state pair timeout/cancel is
>> > correct or not. That's clearly an application bug and I don't want to add
>> > more locking just to make something which is broken by definition pseudo
>> > 'atomic'.
>>
>> I agree that the program is bogus, and don't have to ensure any sane
>> behavior for it. But I am concerned about potential kernel corruptions
>> due to this. For example, maybe kernel code will decide to not remove
>> such timer from the cancel list on destruction because based on
>> clockid/flags it should not be in the cancel list, but the timer is
>> actually there and we will end up with a leak or a dangling pointer. I
>> did not check that this actually happens, such inconsistent state just
>> looks like a red flag for me.
>
> That can't happen.
>
> ctx->might_cancel and ctx->clist are always in sync with the new lock and
> that's the only interesting thing. On destruction we don't look at clockid
> or such, we only care about might_cancel.
>
> What is not guaranteed to be in sync is the timer expiry time and the
> cancel stuff, if two threads operate on the same timerfd in
> parallel. That's what I do not care about at all.

Ack. Thanks for looking at it bearing with me. Then:

Acked-by: Dmitry Vyukov <dvyukov@google.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] timerfd: Protect the might cancel mechanism proper
  2017-02-10 10:26         ` Dmitry Vyukov
@ 2017-02-10 10:51           ` Thomas Gleixner
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Gleixner @ 2017-02-10 10:51 UTC (permalink / raw)
  To: Dmitry Vyukov; +Cc: Al Viro, linux-fsdevel, LKML, syzkaller

On Fri, 10 Feb 2017, Dmitry Vyukov wrote:
> On Fri, Feb 10, 2017 at 11:13 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > ctx->might_cancel and ctx->clist are always in sync with the new lock and
> > that's the only interesting thing. On destruction we don't look at clockid
> > or such, we only care about might_cancel.
> >
> > What is not guaranteed to be in sync is the timer expiry time and the
> > cancel stuff, if two threads operate on the same timerfd in
> > parallel. That's what I do not care about at all.
> 
> Ack. Thanks for looking at it bearing with me. Then:

Thanks for asking the questions. It's always good if we need to think it
over again.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-31 14:24 [PATCH] timerfd: Protect the might cancel mechanism proper Thomas Gleixner
2017-02-01 12:43 ` Dmitry Vyukov
2017-02-02 18:54   ` Thomas Gleixner
2017-02-02 19:04     ` Dmitry Vyukov
2017-02-10 10:13       ` Thomas Gleixner
2017-02-10 10:26         ` Dmitry Vyukov
2017-02-10 10:51           ` Thomas Gleixner
2017-02-10 10:19 ` [tip:timers/core] " tip-bot for Thomas Gleixner

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox