diff mbox series

[net-next,2/3] icmp: move icmp_global.credit and icmp_global.stamp to per netns storage

Message ID 20240828193948.2692476-3-edumazet@google.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series icmp: avoid possible side-channels attacks | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 54 this patch: 55
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 2 maintainers not CCed: lixiaoyan@google.com ncardwell@google.com
netdev/build_clang fail Errors and warnings before: 109 this patch: 110
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 4215 this patch: 4216
netdev/checkpatch warning WARNING: line length of 88 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 1 this patch: 2
netdev/source_inline success Was 0 now: 0

Commit Message

Eric Dumazet Aug. 28, 2024, 7:39 p.m. UTC 
Host wide ICMP ratelimiter should be per netns, to provide better isolation.

Following patch in this series makes the sysctl per netns.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/ip.h         |  4 ++--
 include/net/netns/ipv4.h |  3 ++-
 net/ipv4/icmp.c          | 25 ++++++++++---------------
 net/ipv6/icmp.c          |  4 ++--
 4 files changed, 16 insertions(+), 20 deletions(-)

Comments

David Ahern Aug. 29, 2024, 4:30 a.m. UTC | #1 
On 8/28/24 1:39 PM, Eric Dumazet wrote:
> Host wide ICMP ratelimiter should be per netns, to provide better isolation.
> 
> Following patch in this series makes the sysctl per netns.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/net/ip.h         |  4 ++--
>  include/net/netns/ipv4.h |  3 ++-
>  net/ipv4/icmp.c          | 25 ++++++++++---------------
>  net/ipv6/icmp.c          |  4 ++--
>  4 files changed, 16 insertions(+), 20 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>
Simon Horman Aug. 29, 2024, 1:33 p.m. UTC | #2 
On Wed, Aug 28, 2024 at 07:39:47PM +0000, Eric Dumazet wrote:
> Host wide ICMP ratelimiter should be per netns, to provide better isolation.
> 
> Following patch in this series makes the sysctl per netns.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

...

> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c

...

> @@ -235,7 +230,7 @@ static struct {
>   * Returns false if we reached the limit and can not send another packet.
>   * Works in tandem with icmp_global_consume().
>   */

Hi Eric,

nit: This could be handled in a follow-up, and I'm happy to prepare it
     myself, but net should be added to the Kernel doc above.

> -bool icmp_global_allow(void)
> +bool icmp_global_allow(struct net *net)
>  {
>  	u32 delta, now, oldstamp;
>  	int incr, new, old;
Eric Dumazet Aug. 29, 2024, 1:54 p.m. UTC | #3 
On Thu, Aug 29, 2024 at 3:34 PM Simon Horman <horms@kernel.org> wrote:
>
> On Wed, Aug 28, 2024 at 07:39:47PM +0000, Eric Dumazet wrote:
> > Host wide ICMP ratelimiter should be per netns, to provide better isolation.
> >
> > Following patch in this series makes the sysctl per netns.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> ...
>
> > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
>
> ...
>
> > @@ -235,7 +230,7 @@ static struct {
> >   * Returns false if we reached the limit and can not send another packet.
> >   * Works in tandem with icmp_global_consume().
> >   */
>
> Hi Eric,
>
> nit: This could be handled in a follow-up, and I'm happy to prepare it
>      myself, but net should be added to the Kernel doc above.

Thanks, good point, let me send a V2 real quick.
diff mbox series

Patch

diff --git a/include/net/ip.h b/include/net/ip.h
index 82248813619e3f21e09d52976accbdc76c7668c2..d3bca4e83979f681c4931e9ff62db5941a059c11 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -794,8 +794,8 @@  static inline void ip_cmsg_recv(struct msghdr *msg, struct sk_buff *skb)
 	ip_cmsg_recv_offset(msg, skb->sk, skb, 0, 0);
 }
 
-bool icmp_global_allow(void);
-void icmp_global_consume(void);
+bool icmp_global_allow(struct net *net);
+void icmp_global_consume(struct net *net);
 
 extern int sysctl_icmp_msgs_per_sec;
 extern int sysctl_icmp_msgs_burst;
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 5fcd61ada62289253844be9cbe25387dd92385a5..54fe7c079fffb285b7a8a069f3d57f9440a6655a 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -122,7 +122,8 @@  struct netns_ipv4 {
 	u8 sysctl_icmp_errors_use_inbound_ifaddr;
 	int sysctl_icmp_ratelimit;
 	int sysctl_icmp_ratemask;
-
+	atomic_t icmp_global_credit;
+	u32 icmp_global_stamp;
 	u32 ip_rt_min_pmtu;
 	int ip_rt_mtu_expires;
 	int ip_rt_min_advmss;
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 0078e8fb2e86d0552ef85eb5bf5bef947b0f1c3d..8ad3139a00fb8c5cb8f28f92d125ef83d9e840c3 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -223,11 +223,6 @@  static inline void icmp_xmit_unlock(struct sock *sk)
 int sysctl_icmp_msgs_per_sec __read_mostly = 1000;
 int sysctl_icmp_msgs_burst __read_mostly = 50;
 
-static struct {
-	atomic_t	credit;
-	u32		stamp;
-} icmp_global;
-
 /**
  * icmp_global_allow - Are we allowed to send one more ICMP message ?
  *
@@ -235,7 +230,7 @@  static struct {
  * Returns false if we reached the limit and can not send another packet.
  * Works in tandem with icmp_global_consume().
  */
-bool icmp_global_allow(void)
+bool icmp_global_allow(struct net *net)
 {
 	u32 delta, now, oldstamp;
 	int incr, new, old;
@@ -244,11 +239,11 @@  bool icmp_global_allow(void)
 	 * Then later icmp_global_consume() could consume more credits,
 	 * this is an acceptable race.
 	 */
-	if (atomic_read(&icmp_global.credit) > 0)
+	if (atomic_read(&net->ipv4.icmp_global_credit) > 0)
 		return true;
 
 	now = jiffies;
-	oldstamp = READ_ONCE(icmp_global.stamp);
+	oldstamp = READ_ONCE(net->ipv4.icmp_global_stamp);
 	delta = min_t(u32, now - oldstamp, HZ);
 	if (delta < HZ / 50)
 		return false;
@@ -257,23 +252,23 @@  bool icmp_global_allow(void)
 	if (!incr)
 		return false;
 
-	if (cmpxchg(&icmp_global.stamp, oldstamp, now) == oldstamp) {
-		old = atomic_read(&icmp_global.credit);
+	if (cmpxchg(&net->ipv4.icmp_global_stamp, oldstamp, now) == oldstamp) {
+		old = atomic_read(&net->ipv4.icmp_global_credit);
 		do {
 			new = min(old + incr, READ_ONCE(sysctl_icmp_msgs_burst));
-		} while (!atomic_try_cmpxchg(&icmp_global.credit, &old, new));
+		} while (!atomic_try_cmpxchg(&net->ipv4.icmp_global_credit, &old, new));
 	}
 	return true;
 }
 EXPORT_SYMBOL(icmp_global_allow);
 
-void icmp_global_consume(void)
+void icmp_global_consume(struct net *net)
 {
 	int credits = get_random_u32_below(3);
 
 	/* Note: this might make icmp_global.credit negative. */
 	if (credits)
-		atomic_sub(credits, &icmp_global.credit);
+		atomic_sub(credits, &net->ipv4.icmp_global_credit);
 }
 EXPORT_SYMBOL(icmp_global_consume);
 
@@ -299,7 +294,7 @@  static bool icmpv4_global_allow(struct net *net, int type, int code,
 	if (icmpv4_mask_allow(net, type, code))
 		return true;
 
-	if (icmp_global_allow()) {
+	if (icmp_global_allow(net)) {
 		*apply_ratelimit = true;
 		return true;
 	}
@@ -337,7 +332,7 @@  static bool icmpv4_xrlim_allow(struct net *net, struct rtable *rt,
 	if (!rc)
 		__ICMP_INC_STATS(net, ICMP_MIB_RATELIMITHOST);
 	else
-		icmp_global_consume();
+		icmp_global_consume(net);
 	return rc;
 }
 
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 46f70e4a835139ef7d8925c49440865355048193..071b0bc1179d81b18c340ce415cef21e02a30cd7 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -181,7 +181,7 @@  static bool icmpv6_global_allow(struct net *net, int type,
 	if (icmpv6_mask_allow(net, type))
 		return true;
 
-	if (icmp_global_allow()) {
+	if (icmp_global_allow(net)) {
 		*apply_ratelimit = true;
 		return true;
 	}
@@ -231,7 +231,7 @@  static bool icmpv6_xrlim_allow(struct sock *sk, u8 type,
 		__ICMP6_INC_STATS(net, ip6_dst_idev(dst),
 				  ICMP6_MIB_RATELIMITHOST);
 	else
-		icmp_global_consume();
+		icmp_global_consume(net);
 	dst_release(dst);
 	return res;
 }