Re: [BUG] ptraced process waiting on syscall may return kernel internal errnos
From: Rafael J. Wysocki
Date: Thu Jun 14 2007 - 19:28:36 EST
On Thursday, 14 June 2007 14:58, Oleg Nesterov wrote:
> On 06/14, Rafael J. Wysocki wrote:
> >
> > Sorry for being late, I've just realized that you are discussing the freezer
> > here. ;-)
>
> my fault, I was going to cc you but forgot, sorry!
No problem. :-)
> > On Wednesday, 13 June 2007 17:15, Oleg Nesterov wrote:
> >
> > > > @@ -105,7 +105,11 @@ static int recalc_sigpending_tsk(struct
> > > > set_tsk_thread_flag(t, TIF_SIGPENDING);
> > > > return 1;
> > > > }
> > > > - clear_tsk_thread_flag(t, TIF_SIGPENDING);
> > > > + /*
> > > > + * We must never clear the flag in another thread, or in current
> > > > + * when it's possible the current syscall is returning -ERESTART*.
> > > > + * So we don't clear it here, and only callers who know they should do.
> > > > + */
> > > > return 0;
> > > > }
> > >
> > > This breaks cancel_freezing(). Somehow we should clear TIF_SIGPENDING for
> > > kernel threads. Otherwise we may have subtle failures if try_to_freeze_tasks()
> > > fails.
> >
> > Well, the only code path in which we'd want to call cancel_freezing() for kernel
> > threads is when the freezing of kernel threads. However, this only happens if
> > one of the kernel threads declares itself as freezable and the fails to call
> > try_to_freeze(), which is a bug.
>
> But this happens? We know a lot of reasons why try_to_freeze() can fail just
> because some kthread waits for already frozen task.
>
> > Thus I don't think that we need to worry
> > about that case too much.
>
> Well, we can have very subtle problems because a kernel thread may run with
> TIF_SIGPENDING forever. This means in particualar that any wait_event_interruptible()
> can't succeed. I think this is worse than explicit failure (like -ERESSTART... leak),
> because it is hard to reproduce/debug.
>
> > Moreover, I'm not sure that it's a good idea at all to send signals to kernel
> > threads from the freezer, since in fact we only need to wake them up to make
> > them call try_to_freeze() (after we've set TIF_FREEZE for them).
>
> Yes! I completely agree.
Hmm, what about the appended patch, then?
I've tested it a bit on a UP system.
Greetings,
Rafael
---
include/linux/freezer.h | 9 -----
include/linux/sched.h | 12 +++++++
include/linux/wait.h | 6 +--
kernel/power/process.c | 76 ++++++++++++++++++++++++++++++++++--------------
4 files changed, 70 insertions(+), 33 deletions(-)
Index: linux-2.6.22-rc4-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.22-rc4-mm2.orig/kernel/power/process.c 2007-06-15 01:05:33.000000000 +0200
+++ linux-2.6.22-rc4-mm2/kernel/power/process.c 2007-06-15 01:33:52.000000000 +0200
@@ -75,19 +75,67 @@ void refrigerator(void)
__set_current_state(save);
}
-static void freeze_task(struct task_struct *p)
+static void send_fake_signal(struct task_struct *p)
{
unsigned long flags;
+ if (p->state == TASK_STOPPED)
+ force_sig_specific(SIGSTOP, p);
+ spin_lock_irqsave(&p->sighand->siglock, flags);
+ signal_wake_up(p, p->state == TASK_STOPPED);
+ spin_unlock_irqrestore(&p->sighand->siglock, flags);
+}
+
+/**
+ * freeze_user_process - freeze user space process @p.
+ *
+ * Kernel threads should not have TIF_FREEZE set at this point, so we must
+ * ensure that either p->mm is not NULL *and* PF_BORROWED_MM is unset, or
+ * TIF_FRREZE is left unset. The task_lock() is necessary to prevent races
+ * with exit_mm() or use_mm()/unuse_mm() from occuring.
+ */
+static int freeze_user_process(struct task_struct *p)
+{
+ int ret = 1;
+
+ task_lock(p);
+ if (!p->mm || (p->flags & PF_BORROWED_MM)) {
+ ret = 0;
+ } else if (!freezing(p)) {
+ rmb();
+ if (!frozen(p)) {
+ set_freeze_flag(p);
+ task_unlock(p);
+ send_fake_signal(p);
+ return ret;
+ }
+ }
+ task_unlock(p);
+ return ret;
+}
+
+/**
+ * freeze_task - freeze taks @p, regardless of whether or not it is a
+ * user space process.
+ *
+ * The task_lock() is necessary to prevent races with use_mm()/unuse_mm()
+ * from occuring.
+ */
+static void freeze_task(struct task_struct *p)
+{
if (!freezing(p)) {
rmb();
if (!frozen(p)) {
set_freeze_flag(p);
- if (p->state == TASK_STOPPED)
- force_sig_specific(SIGSTOP, p);
- spin_lock_irqsave(&p->sighand->siglock, flags);
- signal_wake_up(p, p->state == TASK_STOPPED);
- spin_unlock_irqrestore(&p->sighand->siglock, flags);
+ task_lock(p);
+ /* We don't want to send signals to kernel threads */
+ if (p->mm && !(p->flags & PF_BORROWED_MM)) {
+ task_unlock(p);
+ send_fake_signal(p);
+ } else {
+ task_unlock(p);
+ wake_up_state(p, TASK_INTERRUPTIBLE);
+ }
}
}
}
@@ -125,22 +173,8 @@ static int try_to_freeze_tasks(int freez
cancel_freezing(p);
continue;
}
- /*
- * Kernel threads should not have TIF_FREEZE set
- * at this point, so we must ensure that either
- * p->mm is not NULL *and* PF_BORROWED_MM is
- * unset, or TIF_FRREZE is left unset.
- * The task_lock() is necessary to prevent races
- * with exit_mm() or use_mm()/unuse_mm() from
- * occuring.
- */
- task_lock(p);
- if (!p->mm || (p->flags & PF_BORROWED_MM)) {
- task_unlock(p);
+ if (!freeze_user_process(p))
continue;
- }
- freeze_task(p);
- task_unlock(p);
} else {
freeze_task(p);
}
Index: linux-2.6.22-rc4-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.22-rc4-mm2.orig/include/linux/sched.h 2007-06-15 01:05:33.000000000 +0200
+++ linux-2.6.22-rc4-mm2/include/linux/sched.h 2007-06-15 01:05:41.000000000 +0200
@@ -1797,6 +1797,18 @@ static inline void inc_syscw(struct task
}
#endif
+#ifdef CONFIG_PM
+static inline int freezing(struct task_struct *p)
+{
+ return unlikely(test_tsk_thread_flag(p, TIF_FREEZE));
+}
+#else
+static inline int freezing(struct task_struct *p)
+{
+ return 0;
+}
+#endif
+
#endif /* __KERNEL__ */
#endif
Index: linux-2.6.22-rc4-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.22-rc4-mm2.orig/include/linux/freezer.h 2007-06-15 01:05:33.000000000 +0200
+++ linux-2.6.22-rc4-mm2/include/linux/freezer.h 2007-06-15 01:05:41.000000000 +0200
@@ -15,14 +15,6 @@ static inline int frozen(struct task_str
}
/*
- * Check if there is a request to freeze a process
- */
-static inline int freezing(struct task_struct *p)
-{
- return test_tsk_thread_flag(p, TIF_FREEZE);
-}
-
-/*
* Request that a process be frozen
*/
static inline void set_freeze_flag(struct task_struct *p)
@@ -128,7 +120,6 @@ static inline void set_freezable(void)
#else
static inline int frozen(struct task_struct *p) { return 0; }
-static inline int freezing(struct task_struct *p) { return 0; }
static inline void set_freeze_flag(struct task_struct *p) {}
static inline void clear_freeze_flag(struct task_struct *p) {}
static inline int thaw_process(struct task_struct *p) { return 1; }
Index: linux-2.6.22-rc4-mm2/include/linux/wait.h
===================================================================
--- linux-2.6.22-rc4-mm2.orig/include/linux/wait.h 2007-06-15 01:05:33.000000000 +0200
+++ linux-2.6.22-rc4-mm2/include/linux/wait.h 2007-06-15 01:05:41.000000000 +0200
@@ -240,7 +240,7 @@ do { \
prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
if (condition) \
break; \
- if (!signal_pending(current)) { \
+ if (!signal_pending(current) && !freezing(current)) { \
schedule(); \
continue; \
} \
@@ -281,7 +281,7 @@ do { \
prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
if (condition) \
break; \
- if (!signal_pending(current)) { \
+ if (!signal_pending(current) && !freezing(current)) { \
ret = schedule_timeout(ret); \
if (!ret) \
break; \
@@ -327,7 +327,7 @@ do { \
TASK_INTERRUPTIBLE); \
if (condition) \
break; \
- if (!signal_pending(current)) { \
+ if (!signal_pending(current) && !freezing(current)) { \
schedule(); \
continue; \
} \
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/