netdev - Re: [net-next PATCH] net: reduce cycles spend on ICMP replies that gets rate limited

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Sat, 7 Jan 2017 10:28:01 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     netdev@...r.kernel.org, brouer@...hat.com
Subject: Re: [net-next PATCH] net: reduce cycles spend on ICMP replies that
 gets rate limited

On Fri, 06 Jan 2017 14:08:06 -0800
Eric Dumazet <eric.dumazet@...il.com> wrote:

> On Fri, 2017-01-06 at 11:40 -0800, Eric Dumazet wrote:
> > On Fri, 2017-01-06 at 18:39 +0100, Jesper Dangaard Brouer wrote:
> > 
> >   
> > > @@ -648,13 +668,17 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
> > >  		}
> > >  	}
> > >  
> > > -	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
> > > -	if (!icmp_param)
> > > -		return;
> > > -
> > >  	sk = icmp_xmit_lock(net);
> > >  	if (!sk)
> > > -		goto out_free;
> > > +		goto out;
> > > +
> > > +	/* Check global sysctl_icmp_msgs_per_sec ratelimit */
> > > +	if (!icmpv4_global_allow(net, type, code))
> > > +		goto out_unlock;
> > > +
> > > +	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
> > > +	if (!icmp_param)
> > > +		goto out_unlock;  
> >   
> 
> You could call icmp_xmit_lock() _after_ checking global limit perhaps. 
> 
> That would remove one atomic op.

The reason I don't do this is, because icmp_xmit_lock() disables BH and
icmp_global_allow() have a comment that it need to run with BH-disabled.
Thus, I'm depending on the BH-disable from icmp_xmit_lock().

I would have to move out the BH-disable part of icmp_xmit_lock, to do
this. Would that be acceptable?

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer