diff mbox series

[v2,net] tcp/dccp: fix other lockdep splats accessing ireq_opt

Message ID 1508800508.30291.104.camel@edumazet-glaptop3.roam.corp.google.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series [v2,net] tcp/dccp: fix other lockdep splats accessing ireq_opt | expand

Commit Message

Eric Dumazet Oct. 23, 2017, 11:15 p.m. UTC 
From: Eric Dumazet <edumazet@google.com>

In my first attempt to fix the lockdep splat, I forgot we could
enter inet_csk_route_req() with a freshly allocated request socket,
for which refcount has not yet been elevated, due to complex
SLAB_TYPESAFE_BY_RCU rules.

We either are in rcu_read_lock() section _or_ we own a refcount on the
request.

Correct RCU verb to use here is rcu_dereference_check(), although it is
not possible to prove we actually own a reference on a shared
refcount :/

In v2, I added ireq_opt_deref() helper and use in three places, to fix other
possible splats.

[   49.844590]  lockdep_rcu_suspicious+0xea/0xf3
[   49.846487]  inet_csk_route_req+0x53/0x14d
[   49.848334]  tcp_v4_route_req+0xe/0x10
[   49.850174]  tcp_conn_request+0x31c/0x6a0
[   49.851992]  ? __lock_acquire+0x614/0x822
[   49.854015]  tcp_v4_conn_request+0x5a/0x79
[   49.855957]  ? tcp_v4_conn_request+0x5a/0x79
[   49.858052]  tcp_rcv_state_process+0x98/0xdcc
[   49.859990]  ? sk_filter_trim_cap+0x2f6/0x307
[   49.862085]  tcp_v4_do_rcv+0xfc/0x145
[   49.864055]  ? tcp_v4_do_rcv+0xfc/0x145
[   49.866173]  tcp_v4_rcv+0x5ab/0xaf9
[   49.868029]  ip_local_deliver_finish+0x1af/0x2e7
[   49.870064]  ip_local_deliver+0x1b2/0x1c5
[   49.871775]  ? inet_del_offload+0x45/0x45
[   49.873916]  ip_rcv_finish+0x3f7/0x471
[   49.875476]  ip_rcv+0x3f1/0x42f
[   49.876991]  ? ip_local_deliver_finish+0x2e7/0x2e7
[   49.878791]  __netif_receive_skb_core+0x6d3/0x950
[   49.880701]  ? process_backlog+0x7e/0x216
[   49.882589]  __netif_receive_skb+0x1d/0x5e
[   49.884122]  process_backlog+0x10c/0x216
[   49.885812]  net_rx_action+0x147/0x3df

Fixes: a6ca7abe53633 ("tcp/dccp: fix lockdep splat in inet_csk_route_req()")
Fixes: c92e8c02fe66 ("tcp/dccp: fix ireq->opt races")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: kernel test robot <fengguang.wu@intel.com>
Reported-by: Maciej Żenczykowski <maze@google.com>
---
 include/net/inet_sock.h         |    6 ++++++
 net/dccp/ipv4.c                 |    2 +-
 net/ipv4/inet_connection_sock.c |    4 ++--
 net/ipv4/tcp_ipv4.c             |    2 +-
 4 files changed, 10 insertions(+), 4 deletions(-)

Comments

David Miller Oct. 24, 2017, 9:56 a.m. UTC | #1 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 23 Oct 2017 16:15:08 -0700

> +static inline struct ip_options_rcu *ireq_opt_deref(const struct inet_request_sock *ireq)
> +{
> +	return rcu_dereference_check(ireq->ireq_opt,
> +				     refcount_read(&ireq.req->rsk_refcnt) > 0);
                                                    ^^^^^^^^^

Eric, _please_ build test your changes properly.
kernel test robot Oct. 24, 2017, 12:17 p.m. UTC | #2 
Hi Eric,

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Eric-Dumazet/tcp-dccp-fix-other-lockdep-splats-accessing-ireq_opt/20171024-200038
config: i386-randconfig-x003-201743 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/srcu.h:33:0,
                    from include/linux/notifier.h:15,
                    from include/linux/memory_hotplug.h:6,
                    from include/linux/mmzone.h:779,
                    from include/linux/gfp.h:5,
                    from include/linux/mm.h:9,
                    from net/socket.c:61:
   include/net/inet_sock.h: In function 'ireq_opt_deref':
>> include/net/inet_sock.h:138:29: error: 'ireq' is a pointer; did you mean to use '->'?
             refcount_read(&ireq.req->rsk_refcnt) > 0);
                                ^
            ->
   include/linux/rcupdate.h:292:52: note: in definition of macro 'RCU_LOCKDEP_WARN'
      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
                                                       ^
   include/linux/rcupdate.h:486:2: note: in expansion of macro '__rcu_dereference_check'
     __rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
     ^~~~~~~~~~~~~~~~~~~~~~~
>> include/net/inet_sock.h:137:9: note: in expansion of macro 'rcu_dereference_check'
     return rcu_dereference_check(ireq->ireq_opt,
            ^~~~~~~~~~~~~~~~~~~~~

vim +138 include/net/inet_sock.h

   134	
   135	static inline struct ip_options_rcu *ireq_opt_deref(const struct inet_request_sock *ireq)
   136	{
 > 137		return rcu_dereference_check(ireq->ireq_opt,
 > 138					     refcount_read(&ireq.req->rsk_refcnt) > 0);
   139	}
   140	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot Oct. 24, 2017, 12:27 p.m. UTC | #3 
Hi Eric,

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Eric-Dumazet/tcp-dccp-fix-other-lockdep-splats-accessing-ireq_opt/20171024-200038
config: i386-randconfig-x079-201743 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/list.h:4,
                    from include/linux/module.h:9,
                    from net//8021q/vlan_dev.c:25:
   include/net/inet_sock.h: In function 'ireq_opt_deref':
   include/net/inet_sock.h:138:29: error: 'ireq' is a pointer; did you mean to use '->'?
             refcount_read(&ireq.req->rsk_refcnt) > 0);
                                ^
            ->
   include/linux/compiler.h:156:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
   include/linux/rcupdate.h:292:3: note: in expansion of macro 'if'
      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^~
>> include/linux/rcupdate.h:350:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'
     RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
     ^~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:486:2: note: in expansion of macro '__rcu_dereference_check'
     __rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
     ^~~~~~~~~~~~~~~~~~~~~~~
   include/net/inet_sock.h:137:9: note: in expansion of macro 'rcu_dereference_check'
     return rcu_dereference_check(ireq->ireq_opt,
            ^~~~~~~~~~~~~~~~~~~~~
   include/net/inet_sock.h:138:29: error: 'ireq' is a pointer; did you mean to use '->'?
             refcount_read(&ireq.req->rsk_refcnt) > 0);
                                ^
            ->
   include/linux/compiler.h:156:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
   include/linux/rcupdate.h:292:3: note: in expansion of macro 'if'
      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^~
>> include/linux/rcupdate.h:350:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'
     RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
     ^~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:486:2: note: in expansion of macro '__rcu_dereference_check'
     __rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
     ^~~~~~~~~~~~~~~~~~~~~~~
   include/net/inet_sock.h:137:9: note: in expansion of macro 'rcu_dereference_check'
     return rcu_dereference_check(ireq->ireq_opt,
            ^~~~~~~~~~~~~~~~~~~~~
   include/net/inet_sock.h:138:29: error: 'ireq' is a pointer; did you mean to use '->'?
             refcount_read(&ireq.req->rsk_refcnt) > 0);
                                ^
            ->
   include/linux/compiler.h:167:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
   include/linux/rcupdate.h:292:3: note: in expansion of macro 'if'
      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^~
>> include/linux/rcupdate.h:350:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'
     RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
     ^~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:486:2: note: in expansion of macro '__rcu_dereference_check'
     __rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
     ^~~~~~~~~~~~~~~~~~~~~~~
   include/net/inet_sock.h:137:9: note: in expansion of macro 'rcu_dereference_check'
     return rcu_dereference_check(ireq->ireq_opt,
            ^~~~~~~~~~~~~~~~~~~~~

vim +/RCU_LOCKDEP_WARN +350 include/linux/rcupdate.h

632ee2001 Paul E. McKenney 2010-02-22  283  
4221a9918 Tetsuo Handa     2010-06-26  284  /**
f78f5b90c Paul E. McKenney 2015-06-18  285   * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met
f78f5b90c Paul E. McKenney 2015-06-18  286   * @c: condition to check
f78f5b90c Paul E. McKenney 2015-06-18  287   * @s: informative message
f78f5b90c Paul E. McKenney 2015-06-18  288   */
f78f5b90c Paul E. McKenney 2015-06-18  289  #define RCU_LOCKDEP_WARN(c, s)						\
f78f5b90c Paul E. McKenney 2015-06-18  290  	do {								\
f78f5b90c Paul E. McKenney 2015-06-18  291  		static bool __section(.data.unlikely) __warned;		\
f78f5b90c Paul E. McKenney 2015-06-18 @292  		if (debug_lockdep_rcu_enabled() && !__warned && (c)) {	\
f78f5b90c Paul E. McKenney 2015-06-18  293  			__warned = true;				\
f78f5b90c Paul E. McKenney 2015-06-18  294  			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\
f78f5b90c Paul E. McKenney 2015-06-18  295  		}							\
f78f5b90c Paul E. McKenney 2015-06-18  296  	} while (0)
f78f5b90c Paul E. McKenney 2015-06-18  297  
50406b98b Paul E. McKenney 2012-01-12  298  #if defined(CONFIG_PROVE_RCU) && !defined(CONFIG_PREEMPT_RCU)
50406b98b Paul E. McKenney 2012-01-12  299  static inline void rcu_preempt_sleep_check(void)
50406b98b Paul E. McKenney 2012-01-12  300  {
f78f5b90c Paul E. McKenney 2015-06-18  301  	RCU_LOCKDEP_WARN(lock_is_held(&rcu_lock_map),
5cf05ad75 Paul E. McKenney 2012-05-17  302  			 "Illegal context switch in RCU read-side critical section");
50406b98b Paul E. McKenney 2012-01-12  303  }
50406b98b Paul E. McKenney 2012-01-12  304  #else /* #ifdef CONFIG_PROVE_RCU */
d0df7a349 Paul E. McKenney 2017-05-03  305  static inline void rcu_preempt_sleep_check(void) { }
50406b98b Paul E. McKenney 2012-01-12  306  #endif /* #else #ifdef CONFIG_PROVE_RCU */
50406b98b Paul E. McKenney 2012-01-12  307  
b3fbab057 Paul E. McKenney 2011-05-24  308  #define rcu_sleep_check()						\
b3fbab057 Paul E. McKenney 2011-05-24  309  	do {								\
50406b98b Paul E. McKenney 2012-01-12  310  		rcu_preempt_sleep_check();				\
f78f5b90c Paul E. McKenney 2015-06-18  311  		RCU_LOCKDEP_WARN(lock_is_held(&rcu_bh_lock_map),	\
41f4abd92 Joe Perches      2013-12-05  312  				 "Illegal context switch in RCU-bh read-side critical section"); \
f78f5b90c Paul E. McKenney 2015-06-18  313  		RCU_LOCKDEP_WARN(lock_is_held(&rcu_sched_lock_map),	\
41f4abd92 Joe Perches      2013-12-05  314  				 "Illegal context switch in RCU-sched read-side critical section"); \
b3fbab057 Paul E. McKenney 2011-05-24  315  	} while (0)
b3fbab057 Paul E. McKenney 2011-05-24  316  
ca5ecddfa Paul E. McKenney 2010-04-28  317  #else /* #ifdef CONFIG_PROVE_RCU */
ca5ecddfa Paul E. McKenney 2010-04-28  318  
f78f5b90c Paul E. McKenney 2015-06-18  319  #define RCU_LOCKDEP_WARN(c, s) do { } while (0)
b3fbab057 Paul E. McKenney 2011-05-24  320  #define rcu_sleep_check() do { } while (0)
ca5ecddfa Paul E. McKenney 2010-04-28  321  
ca5ecddfa Paul E. McKenney 2010-04-28  322  #endif /* #else #ifdef CONFIG_PROVE_RCU */
ca5ecddfa Paul E. McKenney 2010-04-28  323  
ca5ecddfa Paul E. McKenney 2010-04-28  324  /*
ca5ecddfa Paul E. McKenney 2010-04-28  325   * Helper functions for rcu_dereference_check(), rcu_dereference_protected()
ca5ecddfa Paul E. McKenney 2010-04-28  326   * and rcu_assign_pointer().  Some of these could be folded into their
ca5ecddfa Paul E. McKenney 2010-04-28  327   * callers, but they are left separate in order to ease introduction of
ca5ecddfa Paul E. McKenney 2010-04-28  328   * multiple flavors of pointers to match the multiple flavors of RCU
ca5ecddfa Paul E. McKenney 2010-04-28  329   * (e.g., __rcu_bh, * __rcu_sched, and __srcu), should this make sense in
ca5ecddfa Paul E. McKenney 2010-04-28  330   * the future.
ca5ecddfa Paul E. McKenney 2010-04-28  331   */
53ecfba25 Paul E. McKenney 2010-09-13  332  
53ecfba25 Paul E. McKenney 2010-09-13  333  #ifdef __CHECKER__
53ecfba25 Paul E. McKenney 2010-09-13  334  #define rcu_dereference_sparse(p, space) \
53ecfba25 Paul E. McKenney 2010-09-13  335  	((void)(((typeof(*p) space *)p) == p))
53ecfba25 Paul E. McKenney 2010-09-13  336  #else /* #ifdef __CHECKER__ */
53ecfba25 Paul E. McKenney 2010-09-13  337  #define rcu_dereference_sparse(p, space)
53ecfba25 Paul E. McKenney 2010-09-13  338  #endif /* #else #ifdef __CHECKER__ */
53ecfba25 Paul E. McKenney 2010-09-13  339  
ca5ecddfa Paul E. McKenney 2010-04-28  340  #define __rcu_access_pointer(p, space) \
ca5ecddfa Paul E. McKenney 2010-04-28  341  ({ \
7d0ae8086 Paul E. McKenney 2015-03-03  342  	typeof(*p) *_________p1 = (typeof(*p) *__force)READ_ONCE(p); \
53ecfba25 Paul E. McKenney 2010-09-13  343  	rcu_dereference_sparse(p, space); \
ca5ecddfa Paul E. McKenney 2010-04-28  344  	((typeof(*p) __force __kernel *)(_________p1)); \
ca5ecddfa Paul E. McKenney 2010-04-28  345  })
ca5ecddfa Paul E. McKenney 2010-04-28  346  #define __rcu_dereference_check(p, c, space) \
ca5ecddfa Paul E. McKenney 2010-04-28  347  ({ \
ac59853c0 Pranith Kumar    2014-11-13  348  	/* Dependency order vs. p above. */ \
ac59853c0 Pranith Kumar    2014-11-13  349  	typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
f78f5b90c Paul E. McKenney 2015-06-18 @350  	RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
53ecfba25 Paul E. McKenney 2010-09-13  351  	rcu_dereference_sparse(p, space); \
ac59853c0 Pranith Kumar    2014-11-13  352  	((typeof(*p) __force __kernel *)(________p1)); \
ca5ecddfa Paul E. McKenney 2010-04-28  353  })
ca5ecddfa Paul E. McKenney 2010-04-28  354  #define __rcu_dereference_protected(p, c, space) \
ca5ecddfa Paul E. McKenney 2010-04-28  355  ({ \
f78f5b90c Paul E. McKenney 2015-06-18  356  	RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \
53ecfba25 Paul E. McKenney 2010-09-13  357  	rcu_dereference_sparse(p, space); \
ca5ecddfa Paul E. McKenney 2010-04-28  358  	((typeof(*p) __force __kernel *)(p)); \
ca5ecddfa Paul E. McKenney 2010-04-28  359  })
995f14056 Paul E. McKenney 2016-07-01  360  #define rcu_dereference_raw(p) \
995f14056 Paul E. McKenney 2016-07-01  361  ({ \
995f14056 Paul E. McKenney 2016-07-01  362  	/* Dependency order vs. p above. */ \
995f14056 Paul E. McKenney 2016-07-01  363  	typeof(p) ________p1 = lockless_dereference(p); \
995f14056 Paul E. McKenney 2016-07-01  364  	((typeof(*p) __force __kernel *)(________p1)); \
995f14056 Paul E. McKenney 2016-07-01  365  })
ca5ecddfa Paul E. McKenney 2010-04-28  366  

:::::: The code at line 350 was first introduced by commit
:::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN()

:::::: TO: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
:::::: CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Eric Dumazet Oct. 24, 2017, 3:18 p.m. UTC | #4 
On Tue, 2017-10-24 at 18:56 +0900, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 23 Oct 2017 16:15:08 -0700
> 
> > +static inline struct ip_options_rcu *ireq_opt_deref(const struct inet_request_sock *ireq)
> > +{
> > +	return rcu_dereference_check(ireq->ireq_opt,
> > +				     refcount_read(&ireq.req->rsk_refcnt) > 0);
>                                                     ^^^^^^^^^
> 
> Eric, _please_ build test your changes properly.

Ah, you mean the compiler is not fixing this small error for us ? :)

Sorry :/
diff mbox series

Patch

diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index 425752f768d2f1a0efb13964204e07f27609e9db..b9d59d93ec8f1e763f53a4df25366d1f1d576563 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -132,6 +132,12 @@  static inline int inet_request_bound_dev_if(const struct sock *sk,
 	return sk->sk_bound_dev_if;
 }
 
+static inline struct ip_options_rcu *ireq_opt_deref(const struct inet_request_sock *ireq)
+{
+	return rcu_dereference_check(ireq->ireq_opt,
+				     refcount_read(&ireq.req->rsk_refcnt) > 0);
+}
+
 struct inet_cork {
 	unsigned int		flags;
 	__be32			addr;
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index 0490916864f93d5466e87f5b97dc524b3ee57a2e..e65fcb45c3f6c1edc70fc9898ebe6404175b102f 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -495,7 +495,7 @@  static int dccp_v4_send_response(const struct sock *sk, struct request_sock *req
 							      ireq->ir_rmt_addr);
 		err = ip_build_and_send_pkt(skb, sk, ireq->ir_loc_addr,
 					    ireq->ir_rmt_addr,
-					    rcu_dereference(ireq->ireq_opt));
+					    ireq_opt_deref(ireq));
 		err = net_xmit_eval(err);
 	}
 
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 18cd2eae758ff1a9d8a736e143417c7007b99067..b47a59cb3573b3b77aa5cbb9c2739a12ef37a237 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -543,8 +543,8 @@  struct dst_entry *inet_csk_route_req(const struct sock *sk,
 	struct ip_options_rcu *opt;
 	struct rtable *rt;
 
-	opt = rcu_dereference_protected(ireq->ireq_opt,
-					refcount_read(&req->rsk_refcnt) > 0);
+	opt = ireq_opt_deref(ireq);
+
 	flowi4_init_output(fl4, ireq->ir_iif, ireq->ir_mark,
 			   RT_CONN_FLAGS(sk), RT_SCOPE_UNIVERSE,
 			   sk->sk_protocol, inet_sk_flowi_flags(sk),
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 4c43365c374c8bf868fc0b862333244ca26d5016..5b027c69cbc540d4e933189f9de5baab5472eadb 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -877,7 +877,7 @@  static int tcp_v4_send_synack(const struct sock *sk, struct dst_entry *dst,
 
 		err = ip_build_and_send_pkt(skb, sk, ireq->ir_loc_addr,
 					    ireq->ir_rmt_addr,
-					    rcu_dereference(ireq->ireq_opt));
+					    ireq_opt_deref(ireq));
 		err = net_xmit_eval(err);
 	}