xfrm: Refactor xfrm_state timer management

When clock_was_set is called in case of system wall time change or host resume from suspend state, use this notifier for places where interested in this action.

Signed-off-by Fan Du <[email protected]>

---
  kernel/hrtimer.c |    4 ++++
  1 file changed, 4 insertions(+)

kernel/hrtimer.c +0 -0

index 383319b..b7c62a9 100644

kernel/hrtimer.c +0 -0

  }
 
  /*

Quoted 3 lines · Fan Du

When clock_was_set is called in case of system wall time change or host resume from suspend state, use this notifier for places where interested in this action.

(Only minor commenting on this one ...)

Quoted 10 lines

Signed-off-by Fan Du <[email protected]>

---
kernel/hrtimer.c | 4 ++++

1 file changed, 4 insertions(+)

kernel/hrtimer.c +0 -0

index 383319b..b7c62a9 100644

kernel/hrtimer.c static inline void retrigger_next_event(void *arg) { } · line 759

Quoted 5 lines

kernel/hrtimer.c +2 -0

@@ -755,6 +755,9 @@ static inline void retrigger_next_event(void *arg) { }
 
 #endif /* CONFIG_HIGH_RES_TIMERS */
 
+ATOMIC_NOTIFIER_HEAD(clock_change_notifier_list);
+EXPORT_SYMBOL(clock_change_notifier_list);

This should be static and hidden from other modules, e.g. have a look at netevent_notif_chain (net/core/netevent.c).

Instead, this should be accessed via registration/un-registration handlers for notifier blocks, and those can then be exported as EXPORT_SYMBOL_GPL as this is core area.

kernel/hrtimer.c void clock_was_set(void) · line 779

Quoted 9 lines

kernel/hrtimer.c +2 -0

+
 /*
 * Clock realtime was set
 *

@@ -773,6 +776,7 @@ void clock_was_set(void)
 on_each_cpu(retrigger_next_event, NULL, 1);
 #endif
 timerfd_clock_was_set();
+ atomic_notifier_call_chain(&clock_change_notifier_list, 0, 0);

Also here a small one-line handler call_clock_change_notifiers() would be better.

Quoted 4 lines

kernel/hrtimer.c +0 -0

 }
 
 /*

Quoted 30 lines · Daniel Borkmann

Quoted 3 lines · Fan Du

When clock_was_set is called in case of system wall time change or host resume from suspend state, use this notifier for places where interested in this action.

(Only minor commenting on this one ...)

Quoted 10 lines

Signed-off-by Fan Du <[email protected]>

---
kernel/hrtimer.c | 4 ++++

1 file changed, 4 insertions(+)

kernel/hrtimer.c +0 -0

index 383319b..b7c62a9 100644

kernel/hrtimer.c static inline void retrigger_next_event(void *arg) { } · line 759

Quoted 5 lines

kernel/hrtimer.c +2 -0

@@ -755,6 +755,9 @@ static inline void retrigger_next_event(void *arg) { }
 
 #endif /* CONFIG_HIGH_RES_TIMERS */
 
+ATOMIC_NOTIFIER_HEAD(clock_change_notifier_list);
+EXPORT_SYMBOL(clock_change_notifier_list);

This should be static and hidden from other modules, e.g. have a look at netevent_notif_chain (net/core/netevent.c).

Instead, this should be accessed via registration/un-registration handlers for notifier blocks, and those can then be exported as EXPORT_SYMBOL_GPL as this is core area.

Quoted 12 lines

kernel/hrtimer.c void clock_was_set(void) · line 779

Quoted 9 lines

kernel/hrtimer.c +2 -0

+
 /*
 * Clock realtime was set
 *

@@ -773,6 +776,7 @@ void clock_was_set(void)
 on_each_cpu(retrigger_next_event, NULL, 1);
 #endif
 timerfd_clock_was_set();
+ atomic_notifier_call_chain(&clock_change_notifier_list, 0, 0);

Also here a small one-line handler call_clock_change_notifiers() would be better.

Thanks for your attention.
After taking a look at netevent_notif_chain, you mean I should do it in below style:

static ATOMIC_NOTIFIER_HEAD(clock_change_notifier_list);

int un/register_clock_change_notifier(struct notifier_block *nb)

{
         int err;

         err = atomic_notifier_chain_un/register(&clock_change_notifier_list, nb);
         return err;

}
EXPORT_SYMBOL_GPL(clock_change_notifier_list);

int call_clock_change_notifiers(unsigned long val, void *v)

{
         return atomic_notifier_call_chain(&clock_change_notifier, val, v);
}

EXPORT_SYMBOL_GPL(call_clock_change_notifiers);

Will do it in next version after others comment rest of patches.

From e3929d4fdfad5b40fd8cad0e217597670d1aef54 Mon Sep 17 00:00:00 2001
From: Fan Du <[email protected]>
Date: Wed, 14 Aug 2013 16:39:23 +0800
Subject: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

When clock_was_set is called in case of system wall time change or host resume from suspend state, use this notifier for places where interested in this action, e.g Ipsec SA lifetime management.

Signed-off-by Fan Du <[email protected]>

v3:
-Beautify notifier with register/unregister API exported for other subsystem.

include/linux/hrtimer.h +3 -0

index d19a5c2..f0404e4 100644
@@ -461,4 +461,7 @@ extern u64 ktime_divns(const ktime_t kt, s64 div);
  /* Show pending timers: */
  extern void sysrq_timer_list_show(void);
 
+extern int register_clock_change_notifier(struct notifier_block *nb);
+extern int unregister_clock_change_notifier(struct notifier_block *nb);
+
  #endif

kernel/hrtimer.c +18 -0

index 383319b..c6e6405 100644
@@ -755,6 +755,24 @@ static inline void retrigger_next_event(void *arg) { }
 
  #endif /* CONFIG_HIGH_RES_TIMERS */
 
+static ATOMIC_NOTIFIER_HEAD(clock_change_notifier_list);
+static int call_clock_change_notifiers(void)
+{
+    return atomic_notifier_call_chain(&clock_change_notifier_list, 0, 0);
+}
+
+int register_clock_change_notifier(struct notifier_block *nb)
+{
+    return atomic_notifier_chain_register(&clock_change_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(register_clock_change_notifier);
+
+int unregister_clock_change_notifier(struct notifier_block *nb)
+{
+    return atomic_notifier_chain_unregister(&clock_change_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(unregister_clock_change_notifier);
+
  /*
   * Clock realtime was set
   *

From e3929d4fdfad5b40fd8cad0e217597670d1aef54 Mon Sep 17 00:00:00 2001
From: Fan Du <[email protected]>
Date: Wed, 14 Aug 2013 16:39:23 +0800
Subject: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

When clock_was_set is called in case of system wall time change or host resume from suspend state, use this notifier for places where interested in this action, e.g Ipsec SA lifetime management.

Quoted 9 lines · Fan Du

From e3929d4fdfad5b40fd8cad0e217597670d1aef54 Mon Sep 17 00:00:00 2001
From: Fan Du<[email protected]>
Date: Wed, 14 Aug 2013 16:39:23 +0800
Subject: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

When clock_was_set is called in case of system wall time change or host resume from suspend state, use this notifier for places where interested in this action, e.g Ipsec SA lifetime management.

Sigh. These notifiers have been proposed in the past and we always rejected them. Why do you need this? There should be nothing except the core timekeeping code which needs to know about clock_was_set.

Can you please explain what kind of users you have in mind and WHY they need to know about it.

Thanks,

tglx

Thanks for your patience. Please let me take a few seconds try to explain this.

Current xfrm layers has *one* hrtimer to guard Ipsec keys timeout, The timeout could be measured in either of below two ways:

(1) The timer is started once the keys is created, but this key is not necessary actually used right now. In detail, record the get_seconds() when this key is created.

(2) Starting the timer when this key is actually used, e.g when an IP packet need to be encrypted. In details, recored the get_seconds() when this key is first used.

So in the hrtimer handler, the code get current get_seconds, and check against with what saved in (1)or(2), and notify the timeout up to user land.

So the pitfall is using one hrtimer for two timeout events, most importantly using get_seconds to check timeout, once system clock is changed by user intentionally, the key timeout could misbehave wildly.

A refractor has been proposed to get rid of depending on system wall clock by cleaning up the hrtimer handler. Unfortunately David frowned on it in (3), and suggest once system clock is changed, adjust the timeout of the key.

(3): http://www.spinics.net/lists/netdev/msg245169.html

Quoted 2 lines

(3): http://www.spinics.net/lists/netdev/msg245169.html

Thanks for the explanation so far.

What's still unclear to me is why these timeouts are bound to wall time in the first place.

Is there any real reason why the key life time can't simply be expressed in monotonic time, e.g. N seconds after creation or M seconds after usage? Looking at the relevant RFCs I can't find any requirement for binding the life time to wall time.

A life time of 10 minutes does not change when the wall clock is adjusted for whatever reasons. It's still 10 minutes and not some random result of the wall clock adjustments. But I might be wrong as usual :)

Quoted 13 lines · Thomas Gleixner

Quoted 2 lines

(3): http://www.spinics.net/lists/netdev/msg245169.html

Thanks for the explanation so far.

What's still unclear to me is why these timeouts are bound to wall time in the first place.

Is there any real reason why the key life time can't simply be expressed in monotonic time, e.g. N seconds after creation or M seconds after usage? Looking at the relevant RFCs I can't find any requirement for binding the life time to wall time.

Quoted 4 lines

A life time of 10 minutes does not change when the wall clock is adjusted for whatever reasons. It's still 10 minutes and not some random result of the wall clock adjustments. But I might be wrong as usual :)

Well we started out with straight timers. It was changed because people wanted IPsec SAs to expire after a suspect/resume which

AFAIK does not touch normal timers.

Of course, this brought with it a new set of problems when the system time is stepped which now cause SAs to expire even though they probably shouldn't.

So what about going back to timer_list timers and simply utilize register_pm_notifier(), which will tell you that the system resumed?

From: Thomas Gleixner <[email protected]>
Date: Thu, 12 Sep 2013 16:43:37 +0200 (CEST)

Quoted 2 lines

So what about going back to timer_list timers and simply utilize register_pm_notifier(), which will tell you that the system resumed?

The thing to understand is that there are two timeouts for an IPSEC rule, a soft and a hard timeout.

There is a gap between these two exactly so that we can negotiate a new encapsulation with the IPSEC gateway before communication ceases to be possible over the IPSEC protected path.

So the idea is that the soft timeout triggers the re-negotiation, and after a hard timeout the IPSEC path is no longer usable and all communication will fail.

Simply triggering a re-negoation after every suspend/resume makes no sense at all. Spurious re-negotiations are undesirable.

What we want are real timers. We want that rather than a "we suspended so just assume all timers expired" event which is not very useful for this kind of application.

From: Thomas Gleixner<[email protected]>
Date: Thu, 12 Sep 2013 16:43:37 +0200 (CEST)

Quoted 2 lines

So what about going back to timer_list timers and simply utilize register_pm_notifier(), which will tell you that the system resumed?

The thing to understand is that there are two timeouts for an IPSEC rule, a soft and a hard timeout.

There is a gap between these two exactly so that we can negotiate a new encapsulation with the IPSEC gateway before communication ceases to be possible over the IPSEC protected path.

So the idea is that the soft timeout triggers the re-negotiation, and after a hard timeout the IPSEC path is no longer usable and all communication will fail.

Simply triggering a re-negoation after every suspend/resume makes no sense at all. Spurious re-negotiations are undesirable.

I suspect the thing to do is to have system time changes generate a notifier when clock_was_set() is called.

The XFRM code would walk the rules and pretend that we hit the soft timeout for every rule that we haven't hit the soft timeout yet already.

If a rule hit the soft timeout, force a hard timeout.

When forcing a soft timeout, adjust the hard timeout to be (hard_timeout - soft_timeout) into the future.

What we want are real timers. We want that rather than a "we suspended so just assume all timers expired" event which is not very useful for this kind of application.

(2) What I have been bugging you around here for this long time is really the second
problem, I'm sorry I didn't make it clearly to you and others, which is
below:

Why using wall clock time to calculate soft/hard IPsec events when
xfrm_state timer
out happens in its timeout handler? Because even if xfrm_state using
CLOCK_BOOTTIME,
system wall clock time changing will surely disturb soft/hard IPsec
events, which
you raised your concern about in (*a*).

Quoted 5 lines · Fan Du

(2) What I have been bugging you around here for this long time is really the second
problem, I'm sorry I didn't make it clearly to you and others, which is
below:

Quoted 7 lines

Why using wall clock time to calculate soft/hard IPsec events when
xfrm_state timer
out happens in its timeout handler? Because even if xfrm_state using
CLOCK_BOOTTIME,
system wall clock time changing will surely disturb soft/hard IPsec
events, which
you raised your concern about in (*a*).

No CLOCK_BOOTTIME is not affected by wall clock time changes. It's basically CLOCK_MONOTONIC, it just keeps counting the suspend time as well. So without a suspend/resume cycle CLOCK_MONOTONIC and CLOCK_BOOTTIME are the same. After a suspend/resume cycle CLOCK_BOOTTIME will be N seconds ahead of CLOCK_MONOTONIC, where N is the number of seconds spent in suspend.

Thanks,

tglx

Quoted 16 lines · Thomas Gleixner

Quoted 7 lines · Fan Du

(2) What I have been bugging you around here for this long time is really the
second
problem, I'm sorry I didn't make it clearly to you and others, which
is
below:

Quoted 7 lines

Why using wall clock time to calculate soft/hard IPsec events when
xfrm_state timer
out happens in its timeout handler? Because even if xfrm_state using
CLOCK_BOOTTIME,
system wall clock time changing will surely disturb soft/hard IPsec
events, which
you raised your concern about in (*a*).

Quoted 6 lines

No CLOCK_BOOTTIME is not affected by wall clock time changes. It's basically CLOCK_MONOTONIC, it just keeps counting the suspend time as well. So without a suspend/resume cycle CLOCK_MONOTONIC and CLOCK_BOOTTIME are the same. After a suspend/resume cycle CLOCK_BOOTTIME will be N seconds ahead of CLOCK_MONOTONIC, where N is the number of seconds spent in suspend.

Sorry for the ambiguity. Yes, CLOCK_BOOTTIME is monotonic *and* counting suspend/resume time as well as not affected by wall clock time change.

Using CLOCK_BOOTTIME guarantees b- will timeout in a right manner, i.e., not affected by wall clock time change, as well as keep the timer valid when suspend/resume.

A classic way of using timer is:
a- Arm a timer with specified interval
b- Wait for the timer to timeout
c- After the timeout, notify the event to other place in the timer handler.

IPsec key lifetime timer does its this way:
a- Arm a timer with specified interval
b- Wait for the timer to timeout
c- After timeout, in the timer handler, using wall clock time to calculate
which kind of event to notify user(soft or hard for both key use time, and key created time). So the problem arises at this stage if wall clock time changed.

Quoted 36 lines · Fan Du

Quoted 16 lines · Thomas Gleixner

Quoted 7 lines · Fan Du

(2) What I have been bugging you around here for this long time is really the
second
problem, I'm sorry I didn't make it clearly to you and others, which
is
below:

Quoted 7 lines

Why using wall clock time to calculate soft/hard IPsec events when
xfrm_state timer
out happens in its timeout handler? Because even if xfrm_state using
CLOCK_BOOTTIME,
system wall clock time changing will surely disturb soft/hard IPsec
events, which
you raised your concern about in (*a*).

Quoted 6 lines

No CLOCK_BOOTTIME is not affected by wall clock time changes. It's basically CLOCK_MONOTONIC, it just keeps counting the suspend time as well. So without a suspend/resume cycle CLOCK_MONOTONIC and CLOCK_BOOTTIME are the same. After a suspend/resume cycle CLOCK_BOOTTIME will be N seconds ahead of CLOCK_MONOTONIC, where N is the number of seconds spent in suspend.

Sorry for the ambiguity. Yes, CLOCK_BOOTTIME is monotonic *and* counting suspend/resume time as well as not affected by wall clock time change.

Using CLOCK_BOOTTIME guarantees b- will timeout in a right manner, i.e., not affected by wall clock time change, as well as keep the timer valid when suspend/resume.

A classic way of using timer is:
a- Arm a timer with specified interval
b- Wait for the timer to timeout
c- After the timeout, notify the event to other place in the timer handler.

Quoted 7 lines

IPsec key lifetime timer does its this way:
a- Arm a timer with specified interval
b- Wait for the timer to timeout
c- After timeout, in the timer handler, using wall clock time to calculate
which kind of event to notify user(soft or hard for both key use time, and key created time). So the problem arises at this stage if wall clock time changed.

And why on earth must it use wall clock time for selecting the event type?

Thanks,

tglx

Quoted 13 lines · Thomas Gleixner

Quoted 2 lines

(3): http://www.spinics.net/lists/netdev/msg245169.html

Thanks for the explanation so far.

What's still unclear to me is why these timeouts are bound to wall time in the first place.

Is there any real reason why the key life time can't simply be expressed in monotonic time, e.g. N seconds after creation or M seconds after usage? Looking at the relevant RFCs I can't find any requirement for binding the life time to wall time.

Quoted 4 lines

A life time of 10 minutes does not change when the wall clock is adjusted for whatever reasons. It's still 10 minutes and not some random result of the wall clock adjustments. But I might be wrong as usual :)

Well we started out with straight timers. It was changed because people wanted IPsec SAs to expire after a suspect/resume which AFAIK does not touch normal timers.

Quoted 19 lines · Herbert Xu

Quoted 17 lines · Thomas Gleixner

Quoted 2 lines

(3): http://www.spinics.net/lists/netdev/msg245169.html

Thanks for the explanation so far.

What's still unclear to me is why these timeouts are bound to wall time in the first place.

Is there any real reason why the key life time can't simply be expressed in monotonic time, e.g. N seconds after creation or M seconds after usage? Looking at the relevant RFCs I can't find any requirement for binding the life time to wall time.

A life time of 10 minutes does not change when the wall clock is adjusted for whatever reasons. It's still 10 minutes and not some random result of the wall clock adjustments. But I might be wrong as usual :)

Quoted 3 lines

Well we started out with straight timers. It was changed because people wanted IPsec SAs to expire after a suspect/resume which AFAIK does not touch normal timers.

I'm not sure I've totally groked the specific need here, but if you're wanting a monotonic clockbase that includes suspend time, then you might checkout CLOCK_BOOTTIME.

After clock_was_set called to set new time or host resume from suspend state. Notify IKED with soft timeout for SAs which haven't reach its soft timeout limit. For those dying SAs, arrange them to hard expire.

Signed-off-by Fan Du<[email protected]>

---
  net/xfrm/xfrm_state.c |   40 ++++++++++++++++++++++++++++++++++++++++
  1 file changed, 40 insertions(+)

net/xfrm/xfrm_state.c +5 -0

index 78f66fa..25e808d 100644
@@ -2002,6 +2002,44 @@ int xfrm_init_state(struct xfrm_state *x)
 
  EXPORT_SYMBOL(xfrm_init_state);
 
+extern struct atomic_notifier_head clock_change_notifier_list;
+static int clock_change_callback(struct notifier_block *nb,
+                               unsigned long reason, void *arg)
+{
+	struct xfrm_state_walk *walk;

From bf282199fd172d1bbc6dcd0e38d1b8fc2192e5f5 Mon Sep 17 00:00:00 2001
From: Fan Du <[email protected]>
Date: Wed, 14 Aug 2013 12:59:11 +0800
Subject: [PATCHv3 net-next ] xfrm: Update xfrm_state lifetime expire after clock_was_set

After clock_was_set called to set new time or host resume from suspend state. Notify IKED with soft timeout for SAs which haven't reach its soft timeout limit. For those dying SAs, arrange them to hard expire.

This modification is characterized by SA is sensible to any degree of clock changes while as SA lifetime is marked by second.

Another point is clock_was_set is traversing all net name space to update SA time while holding rtnl_lock, it may not scale very well.

Signed-off-by Fan Du <[email protected]>

v3:
- Fix lockdep complaint about circular locking with trying to acquire

    state->clock while holding xfrm_state_lock.