diff mbox series

netfilter: tproxy: Add RCU protection in nf_tproxy_laddr4

Message ID tencent_DE4D2D0FE82F3CA9294AEEB3A949A44F6008@qq.com
State Not Applicable
Headers show
Series netfilter: tproxy: Add RCU protection in nf_tproxy_laddr4 | expand

Commit Message

Jiawei Ye Sept. 4, 2024, 12:18 p.m. UTC 
In the `nf_tproxy_laddr4` function, both the `__in_dev_get_rcu()` call
and the `in_dev_for_each_ifa_rcu()` macro are used to access
RCU-protected data structures. Previously, these accesses were not
enclosed within an RCU read-side critical section, which violates RCU
usage rules and can lead to race conditions, data inconsistencies, and
memory corruption issues.

This possible bug was identified using a static analysis tool developed
by myself, specifically designed to detect RCU-related issues.

To address this, `rcu_read_lock()` and `rcu_read_unlock()` are added
around the RCU-protected operations in the `nf_tproxy_laddr4` function by
acquiring the RCU read lock before calling `__in_dev_get_rcu()` and
iterating with `in_dev_for_each_ifa_rcu()`. This change prevents
potential RCU issues and adheres to proper RCU usage patterns.

Fixes: b8d19572367b ("netfilter: use in_dev_for_each_ifa_rcu")
Signed-off-by: Jiawei Ye <jiawei.ye@foxmail.com>
---
 net/ipv4/netfilter/nf_tproxy_ipv4.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Eric Dumazet Sept. 4, 2024, 12:32 p.m. UTC | #1 
On Wed, Sep 4, 2024 at 2:25 PM Jiawei Ye <jiawei.ye@foxmail.com> wrote:
>
> In the `nf_tproxy_laddr4` function, both the `__in_dev_get_rcu()` call
> and the `in_dev_for_each_ifa_rcu()` macro are used to access
> RCU-protected data structures. Previously, these accesses were not
> enclosed within an RCU read-side critical section, which violates RCU
> usage rules and can lead to race conditions, data inconsistencies, and
> memory corruption issues.
>
> This possible bug was identified using a static analysis tool developed
> by myself, specifically designed to detect RCU-related issues.
>
> To address this, `rcu_read_lock()` and `rcu_read_unlock()` are added
> around the RCU-protected operations in the `nf_tproxy_laddr4` function by
> acquiring the RCU read lock before calling `__in_dev_get_rcu()` and
> iterating with `in_dev_for_each_ifa_rcu()`. This change prevents
> potential RCU issues and adheres to proper RCU usage patterns.

Please share with us the complete  stack trace where you think rcu is not held,
because your static tool is unknown to us.

nf_tproxy_get_sock_v4() would have a similar issue.
Florian Westphal Sept. 4, 2024, 12:48 p.m. UTC | #2 
Eric Dumazet <edumazet@google.com> wrote:
> On Wed, Sep 4, 2024 at 2:25 PM Jiawei Ye <jiawei.ye@foxmail.com> wrote:
> >
> > In the `nf_tproxy_laddr4` function, both the `__in_dev_get_rcu()` call
> > and the `in_dev_for_each_ifa_rcu()` macro are used to access
> > RCU-protected data structures. Previously, these accesses were not
> > enclosed within an RCU read-side critical section, which violates RCU
> > usage rules and can lead to race conditions, data inconsistencies, and
> > memory corruption issues.
> >
> > This possible bug was identified using a static analysis tool developed
> > by myself, specifically designed to detect RCU-related issues.
> >
> > To address this, `rcu_read_lock()` and `rcu_read_unlock()` are added
> > around the RCU-protected operations in the `nf_tproxy_laddr4` function by
> > acquiring the RCU read lock before calling `__in_dev_get_rcu()` and
> > iterating with `in_dev_for_each_ifa_rcu()`. This change prevents
> > potential RCU issues and adheres to proper RCU usage patterns.
> 
> Please share with us the complete  stack trace where you think rcu is not held,
> because your static tool is unknown to us.
> 
> nf_tproxy_get_sock_v4() would have a similar issue.

Right, all netfilter hooks assume rcu read lock is held.

See nf_hook()/nf_hook_slow().
kernel test robot Sept. 5, 2024, 11:53 a.m. UTC | #3 
Hi Jiawei,

kernel test robot noticed the following build warnings:

[auto build test WARNING on netfilter-nf/main]
[also build test WARNING on linus/master v6.11-rc6 next-20240904]
[cannot apply to nf-next/master horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jiawei-Ye/netfilter-tproxy-Add-RCU-protection-in-nf_tproxy_laddr4/20240904-202126
base:   https://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git main
patch link:    https://lore.kernel.org/r/tencent_DE4D2D0FE82F3CA9294AEEB3A949A44F6008%40qq.com
patch subject: [PATCH] netfilter: tproxy: Add RCU protection in nf_tproxy_laddr4
config: arm-randconfig-001-20240905 (https://download.01.org/0day-ci/archive/20240905/202409051900.8A3Mf8i7-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240905/202409051900.8A3Mf8i7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409051900.8A3Mf8i7-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/ipv4/netfilter/nf_tproxy_ipv4.c:64:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
                   return daddr;
                   ^
   net/ipv4/netfilter/nf_tproxy_ipv4.c:62:2: note: previous statement is here
           if (!indev)
           ^
   1 warning generated.


vim +/if +64 net/ipv4/netfilter/nf_tproxy_ipv4.c

45ca4e0cf2734f Máté Eckl        2018-06-01  49  
45ca4e0cf2734f Máté Eckl        2018-06-01  50  __be32 nf_tproxy_laddr4(struct sk_buff *skb, __be32 user_laddr, __be32 daddr)
45ca4e0cf2734f Máté Eckl        2018-06-01  51  {
b8d19572367bb0 Florian Westphal 2019-05-31  52  	const struct in_ifaddr *ifa;
45ca4e0cf2734f Máté Eckl        2018-06-01  53  	struct in_device *indev;
45ca4e0cf2734f Máté Eckl        2018-06-01  54  	__be32 laddr;
45ca4e0cf2734f Máté Eckl        2018-06-01  55  
45ca4e0cf2734f Máté Eckl        2018-06-01  56  	if (user_laddr)
45ca4e0cf2734f Máté Eckl        2018-06-01  57  		return user_laddr;
45ca4e0cf2734f Máté Eckl        2018-06-01  58  
45ca4e0cf2734f Máté Eckl        2018-06-01  59  	laddr = 0;
2729675b33a93a Jiawei Ye        2024-09-04  60  	rcu_read_lock();
45ca4e0cf2734f Máté Eckl        2018-06-01  61  	indev = __in_dev_get_rcu(skb->dev);
21a673bddc8fd4 Florian Westphal 2024-05-13  62  	if (!indev)
2729675b33a93a Jiawei Ye        2024-09-04  63  		rcu_read_unlock();
21a673bddc8fd4 Florian Westphal 2024-05-13 @64  		return daddr;
b8d19572367bb0 Florian Westphal 2019-05-31  65  
b8d19572367bb0 Florian Westphal 2019-05-31  66  	in_dev_for_each_ifa_rcu(ifa, indev) {
b8d19572367bb0 Florian Westphal 2019-05-31  67  		if (ifa->ifa_flags & IFA_F_SECONDARY)
b8d19572367bb0 Florian Westphal 2019-05-31  68  			continue;
b8d19572367bb0 Florian Westphal 2019-05-31  69  
45ca4e0cf2734f Máté Eckl        2018-06-01  70  		laddr = ifa->ifa_local;
45ca4e0cf2734f Máté Eckl        2018-06-01  71  		break;
b8d19572367bb0 Florian Westphal 2019-05-31  72  	}
2729675b33a93a Jiawei Ye        2024-09-04  73  	rcu_read_unlock();
45ca4e0cf2734f Máté Eckl        2018-06-01  74  
45ca4e0cf2734f Máté Eckl        2018-06-01  75  	return laddr ? laddr : daddr;
45ca4e0cf2734f Máté Eckl        2018-06-01  76  }
45ca4e0cf2734f Máté Eckl        2018-06-01  77  EXPORT_SYMBOL_GPL(nf_tproxy_laddr4);
45ca4e0cf2734f Máté Eckl        2018-06-01  78
kernel test robot Sept. 5, 2024, 3:41 p.m. UTC | #4 
Hi Jiawei,

kernel test robot noticed the following build warnings:

[auto build test WARNING on netfilter-nf/main]
[also build test WARNING on linus/master v6.11-rc6 next-20240905]
[cannot apply to nf-next/master horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jiawei-Ye/netfilter-tproxy-Add-RCU-protection-in-nf_tproxy_laddr4/20240904-202126
base:   https://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git main
patch link:    https://lore.kernel.org/r/tencent_DE4D2D0FE82F3CA9294AEEB3A949A44F6008%40qq.com
patch subject: [PATCH] netfilter: tproxy: Add RCU protection in nf_tproxy_laddr4
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240905/202409052334.kT8sZWuh-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240905/202409052334.kT8sZWuh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409052334.kT8sZWuh-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/ipv4/netfilter/nf_tproxy_ipv4.c: In function 'nf_tproxy_laddr4':
>> net/ipv4/netfilter/nf_tproxy_ipv4.c:62:9: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
      62 |         if (!indev)
         |         ^~
   net/ipv4/netfilter/nf_tproxy_ipv4.c:64:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
      64 |                 return daddr;
         |                 ^~~~~~


vim +/if +62 net/ipv4/netfilter/nf_tproxy_ipv4.c

45ca4e0cf2734f Máté Eckl        2018-06-01  49  
45ca4e0cf2734f Máté Eckl        2018-06-01  50  __be32 nf_tproxy_laddr4(struct sk_buff *skb, __be32 user_laddr, __be32 daddr)
45ca4e0cf2734f Máté Eckl        2018-06-01  51  {
b8d19572367bb0 Florian Westphal 2019-05-31  52  	const struct in_ifaddr *ifa;
45ca4e0cf2734f Máté Eckl        2018-06-01  53  	struct in_device *indev;
45ca4e0cf2734f Máté Eckl        2018-06-01  54  	__be32 laddr;
45ca4e0cf2734f Máté Eckl        2018-06-01  55  
45ca4e0cf2734f Máté Eckl        2018-06-01  56  	if (user_laddr)
45ca4e0cf2734f Máté Eckl        2018-06-01  57  		return user_laddr;
45ca4e0cf2734f Máté Eckl        2018-06-01  58  
45ca4e0cf2734f Máté Eckl        2018-06-01  59  	laddr = 0;
2729675b33a93a Jiawei Ye        2024-09-04  60  	rcu_read_lock();
45ca4e0cf2734f Máté Eckl        2018-06-01  61  	indev = __in_dev_get_rcu(skb->dev);
21a673bddc8fd4 Florian Westphal 2024-05-13 @62  	if (!indev)
2729675b33a93a Jiawei Ye        2024-09-04  63  		rcu_read_unlock();
21a673bddc8fd4 Florian Westphal 2024-05-13  64  		return daddr;
b8d19572367bb0 Florian Westphal 2019-05-31  65  
b8d19572367bb0 Florian Westphal 2019-05-31  66  	in_dev_for_each_ifa_rcu(ifa, indev) {
b8d19572367bb0 Florian Westphal 2019-05-31  67  		if (ifa->ifa_flags & IFA_F_SECONDARY)
b8d19572367bb0 Florian Westphal 2019-05-31  68  			continue;
b8d19572367bb0 Florian Westphal 2019-05-31  69  
45ca4e0cf2734f Máté Eckl        2018-06-01  70  		laddr = ifa->ifa_local;
45ca4e0cf2734f Máté Eckl        2018-06-01  71  		break;
b8d19572367bb0 Florian Westphal 2019-05-31  72  	}
2729675b33a93a Jiawei Ye        2024-09-04  73  	rcu_read_unlock();
45ca4e0cf2734f Máté Eckl        2018-06-01  74  
45ca4e0cf2734f Máté Eckl        2018-06-01  75  	return laddr ? laddr : daddr;
45ca4e0cf2734f Máté Eckl        2018-06-01  76  }
45ca4e0cf2734f Máté Eckl        2018-06-01  77  EXPORT_SYMBOL_GPL(nf_tproxy_laddr4);
45ca4e0cf2734f Máté Eckl        2018-06-01  78
diff mbox series

Patch

diff --git a/net/ipv4/netfilter/nf_tproxy_ipv4.c b/net/ipv4/netfilter/nf_tproxy_ipv4.c
index 73e66a088e25..51ff9c337e71 100644
--- a/net/ipv4/netfilter/nf_tproxy_ipv4.c
+++ b/net/ipv4/netfilter/nf_tproxy_ipv4.c
@@ -57,8 +57,10 @@  __be32 nf_tproxy_laddr4(struct sk_buff *skb, __be32 user_laddr, __be32 daddr)
 		return user_laddr;
 
 	laddr = 0;
+	rcu_read_lock();
 	indev = __in_dev_get_rcu(skb->dev);
 	if (!indev)
+		rcu_read_unlock();
 		return daddr;
 
 	in_dev_for_each_ifa_rcu(ifa, indev) {
@@ -68,6 +70,7 @@  __be32 nf_tproxy_laddr4(struct sk_buff *skb, __be32 user_laddr, __be32 daddr)
 		laddr = ifa->ifa_local;
 		break;
 	}
+	rcu_read_unlock();
 
 	return laddr ? laddr : daddr;
 }