| Message ID | 20110907182217.GB13909@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hello, On Wed, Sep 07, 2011 at 08:22:17PM +0200, Oleg Nesterov wrote: > wait_event_freezable() and wait_event_freezable_timeout() stop > the waiting if try_to_freeze() fails. This is not right, we can > race with __thaw_task() and in this case > > - wait_event_freezable() returns the wrong ERESTARTSYS > > - wait_event_freezable_timeout() can return the positive > value while condition == F Indeed, nice catch. This one actually is pretty dangerous. We probably want to make a separate fix for this and backport it to -stable? > Change the code to always check __retval/condition before return. > > Note: with or without this patch the timeout logic looks strange, > probably we should recalc timeout if try_to_freeze() returns T. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Yeap, with freezable_with_signal gone, this looks correct & simpler to me but it would be nice if you can sprinkle some documentation while at it. :) Thanks.
--- 3.1/include/linux/freezer.h~w_e_f 2011-09-04 20:23:30.000000000 +0200 +++ 3.1/include/linux/freezer.h 2011-09-07 20:00:27.000000000 +0200 @@ -107,32 +107,33 @@ static inline int freezer_should_skip(st * Freezer-friendly wrappers around wait_event_interruptible() and * wait_event_interruptible_timeout(), originally defined in <linux/wait.h> */ - #define wait_event_freezable(wq, condition) \ ({ \ int __retval; \ - do { \ + for (;;) { \ __retval = wait_event_interruptible(wq, \ (condition) || freezing(current)); \ - if (__retval && !freezing(current)) \ + if (__retval || (condition)) \ break; \ - else if (!(condition)) \ - __retval = -ERESTARTSYS; \ - } while (try_to_freeze()); \ + try_to_freeze(); \ + } \ __retval; \ }) - #define wait_event_freezable_timeout(wq, condition, timeout) \ ({ \ long __retval = timeout; \ - do { \ + for (;;) { \ __retval = wait_event_interruptible_timeout(wq, \ (condition) || freezing(current), \ __retval); \ - } while (try_to_freeze()); \ + if (__retval <= 0 || (condition)) \ + break; \ + try_to_freeze(); \ + } \ __retval; \ }) + #else /* !CONFIG_FREEZER */ static inline bool frozen(struct task_struct *p) { return false; } static inline bool freezing(struct task_struct *p) { return false; }
wait_event_freezable() and wait_event_freezable_timeout() stop the waiting if try_to_freeze() fails. This is not right, we can race with __thaw_task() and in this case - wait_event_freezable() returns the wrong ERESTARTSYS - wait_event_freezable_timeout() can return the positive value while condition == F Change the code to always check __retval/condition before return. Note: with or without this patch the timeout logic looks strange, probably we should recalc timeout if try_to_freeze() returns T. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- include/linux/freezer.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)