netdev - Re: [PATCH net-next] icmp: add a global rate limitation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1411139495.26859.18.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Fri, 19 Sep 2014 08:11:35 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Joe Perches <joe@...ches.com>
Cc:	David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] icmp: add a global rate limitation

On Fri, 2014-09-19 at 07:56 -0700, Joe Perches wrote:
> On Fri, 2014-09-19 at 07:38 -0700, Eric Dumazet wrote:
> > Current ICMP rate limiting uses inetpeer cache, which is an RBL tree
> > protected by a lock, meaning that hosts can be stuck hard if all cpus
> > want to check ICMP limits.
> > 
> > When say a DNS or NTP server process is restarted, inetpeer tree grows
> > quick and machine comes to its knees.
> > 
> > iptables can not help because the bottleneck happens before ICMP
> > messages are even cooked and sent.
> > 
> > This patch adds a new global limitation, using a token bucket filter,
> > controlled by two new sysctl :
> > 
> > icmp_msgs_per_sec - INTEGER
> >     Limit maximal number of ICMP packets sent per second from this host.
> >     Only messages whose type matches icmp_ratemask are
> >     controlled by this limit.
> >     Default: 1000
> > 
> > icmp_msgs_burst - INTEGER
> >     icmp_msgs_per_sec controls number of ICMP packets sent per second,
> >     while icmp_msgs_burst controls the burst size of these packets.
> >     Default: 50
> 
> nice.
> 
> > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> []
> > @@ -231,12 +231,62 @@ static inline void icmp_xmit_unlock(struct sock *sk)
> >  	spin_unlock_bh(&sk->sk_lock.slock);
> >  }
> >  
> > +int sysctl_icmp_msgs_per_sec __read_mostly = 1000;
> > +int sysctl_icmp_msgs_burst __read_mostly = 50;
> > +
> > +static struct {
> > +	spinlock_t	lock;
> > +	u32		credit;
> > +	u32		stamp;
> > +} icmp_global = {
> > +	.lock		= __SPIN_LOCK_UNLOCKED(icmp_global.lock),
> > +};
> 
> Is there any real benefit using a u32 stamp
> instead of unsigned long?

This is because the computation and sysctl are 32bit wide.

We clamp the delta to 1 sec anyway, so there is no value having 64bit
wide stamp.

Note that I do not expect anybody trying to overflow the computations,
so I did not bother using u64 to perform the (x * y / HZ) operation.

> 
> The stamp comparisons are to jiffies and now
> have slightly odd (u32) casts.

Thats because I am planning to eventually use a common helper for the
inetpeer token bucket, or the hashed one I mention at the end of
changelog. Keeping u32 would avoid taking too much room.

> 
> > +
> > +/**
> > + * icmp_global_allow - Are we allowed to send one more ICMP message ?
> > + *
> > + * Uses a token bucket to limit our ICMP messages to sysctl_icmp_msgs_per_sec.
> > + * Returns false if we reached the limit and can not send another packet.
> > + * Note: called with BH disabled
> > + */
> > +bool icmp_global_allow(void)
> > +{
> > +	u32 credit, delta, incr = 0, now = (u32)jiffies;
> 
> Doesn't casting jiffies costs a couple cycles
> on a 64 bit machine?

No, thats a word access, instead of a qword.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html