netdev - Re: [RFC] net: ipv4 -- Introduce ifa limit per net

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:	Thu, 10 Mar 2016 23:13:51 +0300
From:	Cyrill Gorcunov <gorcunov@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	xiyou.wangcong@...il.com, alexei.starovoitov@...il.com,
	eric.dumazet@...il.com, netdev@...r.kernel.org, solar@...nwall.com,
	vvs@...tuozzo.com, avagin@...tuozzo.com, xemul@...tuozzo.com,
	vdavydov@...tuozzo.com, khorenko@...tuozzo.com,
	pablo@...filter.org, netfilter-devel@...r.kernel.org
Subject: Re: [RFC] net: ipv4 -- Introduce ifa limit per net

On Thu, Mar 10, 2016 at 03:03:11PM -0500, David Miller wrote:
> From: Cyrill Gorcunov <gorcunov@...il.com>
> Date: Thu, 10 Mar 2016 23:01:34 +0300
> 
> > On Thu, Mar 10, 2016 at 02:55:43PM -0500, David Miller wrote:
> >> > 
> >> > Hmm, but inetdev_destroy() is only called when NETDEV_UNREGISTER
> >> > is happening and masq already registers a netdev notifier...
> >> 
> >> Indeed, good catch.  Therefore:
> >> 
> >> 1) Keep the masq netdev notifier.  That will flush the conntrack table
> >>    for the inetdev_destroy event.
> >> 
> >> 2) Make the inetdev notifier only do something if inetdev->dead is
> >>    false.  (ie. we are flushing an individual address)
> >> 
> >> And then we don't need the NETDEV_UNREGISTER thing at all:
> >> 
> >> diff --git a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
> >> index c6eb421..f71841a 100644
> >> --- a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
> >> +++ b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
> >> @@ -108,10 +108,20 @@ static int masq_inet_event(struct notifier_block *this,
> >>  			   unsigned long event,
> >>  			   void *ptr)
> >>  {
> >> -	struct net_device *dev = ((struct in_ifaddr *)ptr)->ifa_dev->dev;
> >>  	struct netdev_notifier_info info;
> >> +	struct in_ifaddr *ifa = ptr;
> >> +	struct in_device *idev;
> >>  
> >> -	netdev_notifier_info_init(&info, dev);
> >> +	/* The masq_dev_notifier will catch the case of the device going
> >> +	 * down.  So if the inetdev is dead and being destroyed we have
> >> +	 * no work to do.  Otherwise this is an individual address removal
> >> +	 * and we have to perform the flush.
> >> +	 */
> >> +	idev = ifa->ifa_dev;
> >> +	if (idev->dead)
> >> +		return NOTIFY_DONE;
> >> +
> >> +	netdev_notifier_info_init(&info, idev->dev);
> >>  	return masq_device_event(this, event, &info);
> >>  }
> > 
> > Guys, I'm lost. Currently masq_device_event calls for conntrack
> > cleanup with device index, so that once device is going down, the
> > appropriate conntracks gonna be dropped off. Now if device is dead
> > nobody will cleanup the conntracks?
> 
> Both notifiers are run in the inetdev_destroy() case.
> 
> Maybe that's what you are missing.

No :) Look, here is what I mean. Previously with your two patches
we've been calling nf-cleanup for every address, so we had to make
code call for cleanup for one time only. Now with the patch above
the code flow is the following

inetdev_destroy
	in_dev->dead = 1;
	...
	inet_del_ifa
		...
		blocking_notifier_call_chain(&inetaddr_chain, NETDEV_DOWN, ifa1);
		...
		masq_inet_event
		 ...
		  masq_device_event
			if (idev->dead)
				return NOTIFY_DONE;

and nobody calls for nf_ct_iterate_cleanup, no?