Steven Rostedt
2022-Nov-03 21:51 UTC
[Bridge] [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
On Sun, 30 Oct 2022 18:22:03 +0100 Paolo Abeni <pabeni at redhat.com> wrote:> On the positive side, I think converting the sk_stop_timer in > inet_csk_clear_xmit_timers() should be safe and should cover the issue > reported by GuenterWould something like this be OK? [ Note, talking with Thomas Gleixner, we agreed that we are changing the name to: time_shutdown_sync() and timer_shutdown() (no wait version). I'll be posting new patches soon. ] -- Steve diff --git a/include/net/sock.h b/include/net/sock.h index 22f8bab583dd..0ef58697d4e5 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -2439,6 +2439,8 @@ void sk_stop_timer(struct sock *sk, struct timer_list *timer); void sk_stop_timer_sync(struct sock *sk, struct timer_list *timer); +void sk_shutdown_timer(struct sock *sk, struct timer_list *timer); + int __sk_queue_drop_skb(struct sock *sk, struct sk_buff_head *sk_queue, struct sk_buff *skb, unsigned int flags, void (*destructor)(struct sock *sk, diff --git a/net/core/sock.c b/net/core/sock.c index a3ba0358c77c..82124862b594 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -3357,6 +3357,13 @@ void sk_stop_timer_sync(struct sock *sk, struct timer_list *timer) } EXPORT_SYMBOL(sk_stop_timer_sync); +void sk_shutdown_timer(struct sock *sk, struct timer_list* timer) +{ + if (timer_shutdown(timer)) + __sock_put(sk); +} +EXPORT_SYMBOL(sk_shutdown_timer); + void sock_init_data(struct socket *sock, struct sock *sk) { sk_init_common(sk); diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index 5e70228c5ae9..71f398f51958 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -722,15 +722,15 @@ void inet_csk_clear_xmit_timers(struct sock *sk) icsk->icsk_pending = icsk->icsk_ack.pending = 0; - sk_stop_timer(sk, &icsk->icsk_retransmit_timer); - sk_stop_timer(sk, &icsk->icsk_delack_timer); - sk_stop_timer(sk, &sk->sk_timer); + sk_shutdown_timer(sk, &icsk->icsk_retransmit_timer); + sk_shutdown_timer(sk, &icsk->icsk_delack_timer); + sk_shutdown_timer(sk, &sk->sk_timer); } EXPORT_SYMBOL(inet_csk_clear_xmit_timers); void inet_csk_delete_keepalive_timer(struct sock *sk) { - sk_stop_timer(sk, &sk->sk_timer); + sk_shutdown_timer(sk, &sk->sk_timer); } EXPORT_SYMBOL(inet_csk_delete_keepalive_timer);
Eric Dumazet
2022-Nov-04 00:00 UTC
[Bridge] [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
On Thu, Nov 3, 2022 at 2:51 PM Steven Rostedt <rostedt at goodmis.org> wrote:> > On Sun, 30 Oct 2022 18:22:03 +0100 > Paolo Abeni <pabeni at redhat.com> wrote: > > > On the positive side, I think converting the sk_stop_timer in > > inet_csk_clear_xmit_timers() should be safe and should cover the issue > > reported by Guenter > > Would something like this be OK? > > [ Note, talking with Thomas Gleixner, we agreed that we are changing the > name to: time_shutdown_sync() and timer_shutdown() (no wait version). > I'll be posting new patches soon. ] > > -- Steve > > diff --git a/include/net/sock.h b/include/net/sock.h > index 22f8bab583dd..0ef58697d4e5 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -2439,6 +2439,8 @@ void sk_stop_timer(struct sock *sk, struct timer_list *timer); > > void sk_stop_timer_sync(struct sock *sk, struct timer_list *timer); > > +void sk_shutdown_timer(struct sock *sk, struct timer_list *timer); > + > int __sk_queue_drop_skb(struct sock *sk, struct sk_buff_head *sk_queue, > struct sk_buff *skb, unsigned int flags, > void (*destructor)(struct sock *sk, > diff --git a/net/core/sock.c b/net/core/sock.c > index a3ba0358c77c..82124862b594 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -3357,6 +3357,13 @@ void sk_stop_timer_sync(struct sock *sk, struct timer_list *timer) > } > EXPORT_SYMBOL(sk_stop_timer_sync); > > +void sk_shutdown_timer(struct sock *sk, struct timer_list* timer) > +{ > + if (timer_shutdown(timer)) > + __sock_put(sk); > +} > +EXPORT_SYMBOL(sk_shutdown_timer); > + > void sock_init_data(struct socket *sock, struct sock *sk) > { > sk_init_common(sk); > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > index 5e70228c5ae9..71f398f51958 100644 > --- a/net/ipv4/inet_connection_sock.c > +++ b/net/ipv4/inet_connection_sock.c > @@ -722,15 +722,15 @@ void inet_csk_clear_xmit_timers(struct sock *sk) > > icsk->icsk_pending = icsk->icsk_ack.pending = 0; > > - sk_stop_timer(sk, &icsk->icsk_retransmit_timer); > - sk_stop_timer(sk, &icsk->icsk_delack_timer); > - sk_stop_timer(sk, &sk->sk_timer); > + sk_shutdown_timer(sk, &icsk->icsk_retransmit_timer); > + sk_shutdown_timer(sk, &icsk->icsk_delack_timer); > + sk_shutdown_timer(sk, &sk->sk_timer); > } > EXPORT_SYMBOL(inet_csk_clear_xmit_timers);inet_csk_clear_xmit_timers() can be called multiple times during TCP socket lifetime. (See tcp_disconnect(), which can be followed by another connect() ... and loop) Maybe add a second parameter, or add a new inet_csk_shutdown_xmit_timers() only called from tcp_v4_destroy_sock() ?> > void inet_csk_delete_keepalive_timer(struct sock *sk) > { > - sk_stop_timer(sk, &sk->sk_timer); > + sk_shutdown_timer(sk, &sk->sk_timer);SO_KEEPALIVE can be called multiple times in a TCP socket lifetime, on/off/on/off/... I suggest leaving sk_stop_timer() here. Eventually inet_csk_clear_xmit_timers( sk, destroy=true) (or inet_csk_shutdown_xmit_timers(()) will be called before the socket is destroyed.