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  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, 25 Jan 2007 08:33:40 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	Sridhar Samudrala <sri@...ibm.com>
Cc:	YOSHIFUJI Hideaki / 吉藤英明 
	<yoshfuji@...ux-ipv6.org>, davem@...emloft.net,
	kuznet@....inr.ac.ru, pekkas@...core.fi, jmorris@...ei.org,
	kaber@...eworks.de, netdev@...r.kernel.org
Subject: Re: [PATCH] IPv6: Implement RFC 4429 Optimistic Duplicate Address Detection

On Wed, Jan 24, 2007 at 05:54:47PM -0800, Sridhar Samudrala wrote:
> Sec 2.1 of RFC 4429 says
> 
>    Unless noted otherwise, components of the IPv6 protocol stack should
>    treat addresses in the Optimistic state equivalently to those in the
>    Deprecated state, indicating that the address is available for use
>    but should not be used if another suitable address is available.  For
>    example, Default Address Selection [RFC3484] uses the address state
>    to decide which source address to use for an outgoing packet.
>    Implementations should treat an address in state Optimistic as if it
>    were in state Deprecated.  If address states are recorded as
>    individual flags, this can easily be achieved by also setting
>    'Deprecated' when 'Optimistic' is set.
> 
> So i think DEPRECATED flag also should be set when we mark an address
> as OPTIMISTIC so that we don't use it as source address for new 
> connections if another address is available until DAD is completed.
> 
> Thanks
> Sridhar
> 

Oh, good catch.  Thank you Sri.  However, I'm worried about the next paragraph:

It is important to note that the address lifetime rules of [RFC2462]
   still apply, and so an address may be Deprecated as well as
   Optimistic.  When DAD completes without incident, the address becomes
   either a Preferred or a Deprecated address, as per RFC 2462

Given that, it seems to me that addresses which are flagged as Deprecated may
enter and exit that state independently of the DAD process, which I think gives
rise to the possibility of a race.  I.e. if an address becomes deprecated right
before DAD completes, and then addrconf_dad_complete clears the IFA_F_DEPRECATED
flag, that seems wrong.  Instead I think it would be better if we tested for the
OPTIMISTIC flag in ipv6_dev_get_saddr in parallel with the DEPRECATED flag.  I
may be wrong about this, but I'm going to err on the side of safety.  If you can
ensure that this race is not possible.  Please let me know, and I'll happily
just set the flag.  I'll repost a new patch soon.

Thanks & Regards
Neil

> 
> On Tue, 2007-01-23 at 15:51 -0500, Neil Horman wrote:
> > On Tue, Jan 23, 2007 at 09:18:20AM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > > Hello.
> > <snip>
> > 
> > New patch attached, incorporating Yoshijui and Vlads latest comments.  I didn't
> > follow guidance on the ndisc_recv_ns comment, Yoshifuji, since Vlad had already
> > suggested an alternate solution in a previous post, but from looking at them
> > both, they should be equivalent.
> > 
> > Thanks & Regards
> > Neil
> > 
> > Signed-off-by: Neil Horman <nhorman@...driver.com>
> > 
> > 
> >  include/linux/if_addr.h |    1
> >  include/linux/ipv6.h    |    2 +
> >  include/linux/sysctl.h  |    1
> >  include/net/addrconf.h  |    4 +-
> >  net/ipv6/addrconf.c     |   56 ++++++++++++++++++++++++++++----
> >  net/ipv6/mcast.c        |    4 +-
> >  net/ipv6/ndisc.c        |   82 +++++++++++++++++++++++++++++++++++-------------
> >  7 files changed, 117 insertions(+), 33 deletions(-)
> > 
> > 
> > diff --git a/include/linux/if_addr.h b/include/linux/if_addr.h
> > index d557e4c..43f3bed 100644
> > --- a/include/linux/if_addr.h
> > +++ b/include/linux/if_addr.h
> > @@ -39,6 +39,7 @@ enum
> >  #define IFA_F_TEMPORARY		IFA_F_SECONDARY
> > 
> >  #define	IFA_F_NODAD		0x02
> > +#define IFA_F_OPTIMISTIC	0x04
> >  #define	IFA_F_HOMEADDRESS	0x10
> >  #define IFA_F_DEPRECATED	0x20
> >  #define IFA_F_TENTATIVE		0x40
> > diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
> > index f824113..5d37abf 100644
> > --- a/include/linux/ipv6.h
> > +++ b/include/linux/ipv6.h
> > @@ -177,6 +177,7 @@ struct ipv6_devconf {
> >  #endif
> >  #endif
> >  	__s32		proxy_ndp;
> > +	__s32		optimistic_dad;
> >  	void		*sysctl;
> >  };
> > 
> > @@ -205,6 +206,7 @@ enum {
> >  	DEVCONF_RTR_PROBE_INTERVAL,
> >  	DEVCONF_ACCEPT_RA_RT_INFO_MAX_PLEN,
> >  	DEVCONF_PROXY_NDP,
> > +	DEVCONF_OPTIMISTIC_DAD,
> >  	DEVCONF_MAX
> >  };
> > 
> > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> > index 81480e6..972a33a 100644
> > --- a/include/linux/sysctl.h
> > +++ b/include/linux/sysctl.h
> > @@ -570,6 +570,7 @@ enum {
> >  	NET_IPV6_RTR_PROBE_INTERVAL=21,
> >  	NET_IPV6_ACCEPT_RA_RT_INFO_MAX_PLEN=22,
> >  	NET_IPV6_PROXY_NDP=23,
> > +	NET_IPV6_OPTIMISTIC_DAD=24,
> >  	__NET_IPV6_MAX
> >  };
> > 
> > diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> > index 88df8fc..d248a19 100644
> > --- a/include/net/addrconf.h
> > +++ b/include/net/addrconf.h
> > @@ -73,7 +73,9 @@ extern int			ipv6_get_saddr(struct dst_entry *dst,
> >  extern int			ipv6_dev_get_saddr(struct net_device *dev,
> >  					       struct in6_addr *daddr,
> >  					       struct in6_addr *saddr);
> > -extern int			ipv6_get_lladdr(struct net_device *dev, struct in6_addr *);
> > +extern int			ipv6_get_lladdr(struct net_device *dev,
> > +						struct in6_addr *,
> > +						unsigned char banned_flags);
> >  extern int			ipv6_rcv_saddr_equal(const struct sock *sk,
> >  						      const struct sock *sk2);
> >  extern void			addrconf_join_solict(struct net_device *dev,
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 2a7e461..d2b01ec 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -830,7 +830,8 @@ retry:
> >  	ift = !max_addresses ||
> >  	      ipv6_count_addresses(idev) < max_addresses ?
> >  		ipv6_add_addr(idev, &addr, tmp_plen,
> > -			      ipv6_addr_type(&addr)&IPV6_ADDR_SCOPE_MASK, IFA_F_TEMPORARY) : NULL;
> > +			      ipv6_addr_type(&addr)&IPV6_ADDR_SCOPE_MASK,
> > +			      IFA_F_TEMPORARY|IFA_F_OPTIMISTIC) : NULL;
> >  	if (!ift || IS_ERR(ift)) {
> >  		in6_ifa_put(ifp);
> >  		in6_dev_put(idev);
> > @@ -1174,7 +1175,8 @@ int ipv6_get_saddr(struct dst_entry *dst,
> >  }
> > 
> > 
> > -int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr)
> > +int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
> > +		    unsigned char banned_flags)
> >  {
> >  	struct inet6_dev *idev;
> >  	int err = -EADDRNOTAVAIL;
> > @@ -1185,7 +1187,7 @@ int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr)
> > 
> >  		read_lock_bh(&idev->lock);
> >  		for (ifp=idev->addr_list; ifp; ifp=ifp->if_next) {
> > -			if (ifp->scope == IFA_LINK && !(ifp->flags&IFA_F_TENTATIVE)) {
> > +			if (ifp->scope == IFA_LINK && !(ifp->flags & banned_flags)) {
> >  				ipv6_addr_copy(addr, &ifp->addr);
> >  				err = 0;
> >  				break;
> > @@ -1751,6 +1753,7 @@ ok:
> > 
> >  			update_lft = create = 1;
> >  			ifp->cstamp = jiffies;
> > +			ifp->flags |= IFA_F_OPTIMISTIC;
> >  			addrconf_dad_start(ifp, RTF_ADDRCONF|RTF_PREFIX_RT);
> >  		}
> > 
> > @@ -1945,7 +1948,11 @@ static int inet6_addr_add(int ifindex, struct in6_addr *pfx, int plen,
> >  		ifp->prefered_lft = prefered_lft;
> >  		ifp->tstamp = jiffies;
> >  		spin_unlock_bh(&ifp->lock);
> > -
> > +		/*
> > +		 * Note that section 3.1 of RFC 4429 indicates
> > +		 * That the Optimistic flag should not be set for
> > +		 * manually configured addresses
> > +		 */
> >  		addrconf_dad_start(ifp, 0);
> >  		in6_ifa_put(ifp);
> >  		addrconf_verify(0);
> > @@ -2123,7 +2130,8 @@ static void addrconf_add_linklocal(struct inet6_dev *idev, struct in6_addr *addr
> >  {
> >  	struct inet6_ifaddr * ifp;
> > 
> > -	ifp = ipv6_add_addr(idev, addr, 64, IFA_LINK, IFA_F_PERMANENT);
> > +	ifp = ipv6_add_addr(idev, addr, 64, IFA_LINK,
> > +		            IFA_F_PERMANENT|IFA_F_OPTIMISTIC);
> >  	if (!IS_ERR(ifp)) {
> >  		addrconf_dad_start(ifp, 0);
> >  		in6_ifa_put(ifp);
> > @@ -2190,7 +2198,7 @@ ipv6_inherit_linklocal(struct inet6_dev *idev, struct net_device *link_dev)
> >  {
> >  	struct in6_addr lladdr;
> > 
> > -	if (!ipv6_get_lladdr(link_dev, &lladdr)) {
> > +	if (!ipv6_get_lladdr(link_dev, &lladdr, IFA_F_TENTATIVE)) {
> >  		addrconf_add_linklocal(idev, &lladdr);
> >  		return 0;
> >  	}
> > @@ -2537,8 +2545,18 @@ static void addrconf_dad_start(struct inet6_ifaddr *ifp, u32 flags)
> >  	struct inet6_dev *idev = ifp->idev;
> >  	struct net_device *dev = idev->dev;
> > 
> > +	if (!idev->cnf.optimistic_dad)
> > +		ifp->flags &= ~IFA_F_OPTIMISTIC;
> > +
> >  	addrconf_join_solict(dev, &ifp->addr);
> > 
> > +	/*
> > +	 * Optimistic nodes need to joing the anycast address
> > +	 * right away
> > +	 */
> > +	if (ifp->flags & IFA_F_OPTIMISTIC)
> > +		addrconf_join_anycast(ifp);
> > +
> >  	if (ifp->prefix_len != 128 && (ifp->flags&IFA_F_PERMANENT))
> >  		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, dev, 0,
> >  					flags);
> > @@ -2553,7 +2571,7 @@ static void addrconf_dad_start(struct inet6_ifaddr *ifp, u32 flags)
> >  	if (dev->flags&(IFF_NOARP|IFF_LOOPBACK) ||
> >  	    !(ifp->flags&IFA_F_TENTATIVE) ||
> >  	    ifp->flags & IFA_F_NODAD) {
> > -		ifp->flags &= ~IFA_F_TENTATIVE;
> > +		ifp->flags &= ~(IFA_F_TENTATIVE|IFA_F_OPTIMISTIC);
> >  		spin_unlock_bh(&ifp->lock);
> >  		read_unlock_bh(&idev->lock);
> > 
> > @@ -2573,6 +2591,18 @@ static void addrconf_dad_start(struct inet6_ifaddr *ifp, u32 flags)
> >  		addrconf_dad_stop(ifp);
> >  		return;
> >  	}
> > +
> > +	/*
> > +	 * Forwarding devices (routers) should not use
> > +	 * optimistic addresses
> > +	 * Nor should interfaces that don't know the
> > +	 * Source address for their default gateway
> > +	 * RFC 4429 Sec 3.3
> > +	 */
> > +	if ((ipv6_devconf.forwarding) ||
> > +	   (ifp->rt == NULL))
> > +		ifp->flags &= ~IFA_F_OPTIMISTIC;
> > +
> >  	addrconf_dad_kick(ifp);
> >  	spin_unlock_bh(&ifp->lock);
> >  out:
> > @@ -2597,7 +2627,7 @@ static void addrconf_dad_timer(unsigned long data)
> >  		 * DAD was successful
> >  		 */
> > 
> > -		ifp->flags &= ~IFA_F_TENTATIVE;
> > +		ifp->flags &= ~(IFA_F_TENTATIVE|IFA_F_OPTIMISTIC);
> >  		spin_unlock_bh(&ifp->lock);
> >  		read_unlock_bh(&idev->lock);
> > 
> > @@ -3398,6 +3428,7 @@ static void inline ipv6_store_devconf(struct ipv6_devconf *cnf,
> >  #endif
> >  #endif
> >  	array[DEVCONF_PROXY_NDP] = cnf->proxy_ndp;
> > +	array[DEVCONF_OPTIMISTIC_DAD] = cnf->optimistic_dad;
> >  }
> > 
> >  static inline size_t inet6_if_nlmsg_size(void)
> > @@ -3918,6 +3949,15 @@ static struct addrconf_sysctl_table
> >  			.proc_handler	=	&proc_dointvec,
> >  		},
> >  		{
> > +			.ctl_name	=	NET_IPV6_OPTIMISTIC_DAD,
> > +			.procname       =       "optimistic_dad",
> > +			.data           =       &ipv6_devconf.optimistic_dad,
> > +			.maxlen         =       sizeof(int),
> > +			.mode           =       0644,
> > +			.proc_handler   =       &proc_dointvec,
> > +
> > +		},
> > +		{
> >  			.ctl_name	=	0,	/* sentinel */
> >  		}
> >  	},
> > diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> > index 882cde4..9c5273c 100644
> > --- a/net/ipv6/mcast.c
> > +++ b/net/ipv6/mcast.c
> > @@ -1411,7 +1411,7 @@ static struct sk_buff *mld_newpack(struct net_device *dev, int size)
> > 
> >  	skb_reserve(skb, LL_RESERVED_SPACE(dev));
> > 
> > -	if (ipv6_get_lladdr(dev, &addr_buf)) {
> > +	if (ipv6_get_lladdr(dev, &addr_buf, IFA_F_TENTATIVE)) {
> >  		/* <draft-ietf-magma-mld-source-05.txt>:
> >  		 * use unspecified address as the source address
> >  		 * when a valid link-local address is not available.
> > @@ -1789,7 +1789,7 @@ static void igmp6_send(struct in6_addr *addr, struct net_device *dev, int type)
> > 
> >  	skb_reserve(skb, LL_RESERVED_SPACE(dev));
> > 
> > -	if (ipv6_get_lladdr(dev, &addr_buf)) {
> > +	if (ipv6_get_lladdr(dev, &addr_buf, IFA_F_TENTATIVE)) {
> >  		/* <draft-ietf-magma-mld-source-05.txt>:
> >  		 * use unspecified address as the source address
> >  		 * when a valid link-local address is not available.
> > diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> > index 6a9f616..fcd22e3 100644
> > --- a/net/ipv6/ndisc.c
> > +++ b/net/ipv6/ndisc.c
> > @@ -498,7 +498,21 @@ static void ndisc_send_na(struct net_device *dev, struct neighbour *neigh,
> >          msg->icmph.icmp6_unused = 0;
> >          msg->icmph.icmp6_router    = router;
> >          msg->icmph.icmp6_solicited = solicited;
> > -        msg->icmph.icmp6_override  = override;
> > +	if (!ifp || !(ifp->flags & IFA_F_OPTIMISTIC))
> > +		msg->icmph.icmp6_override  = override;
> > +	else {
> > +		/*
> > +		 * We must clear the override flag on all
> > +		 * neighbor advertisements from source
> > +		 * addresses that are OPTIMISTIC - RFC 4429
> > +		 * section 2.2
> > +		 */
> > +		if (override)
> > +			printk(KERN_WARNING
> > +				"Disallowing override flag for OPTIMISTIC addr\n");
> > +		msg->icmph.icmp6_override = 0;
> > +	}
> > +
> > 
> >          /* Set the target address. */
> >  	ipv6_addr_copy(&msg->target, solicited_addr);
> > @@ -542,7 +556,8 @@ void ndisc_send_ns(struct net_device *dev, struct neighbour *neigh,
> >  	int send_llinfo;
> > 
> >  	if (saddr == NULL) {
> > -		if (ipv6_get_lladdr(dev, &addr_buf))
> > +		if (ipv6_get_lladdr(dev, &addr_buf,
> > +			            (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)))
> >  			return;
> >  		saddr = &addr_buf;
> >  	}
> > @@ -622,9 +637,20 @@ void ndisc_send_rs(struct net_device *dev, struct in6_addr *saddr,
> >          struct sk_buff *skb;
> >          struct icmp6hdr *hdr;
> >  	__u8 * opt;
> > +	struct inet6_ifaddr *ifp;
> >          int len;
> >  	int err;
> > 
> > +	/*
> > +	 * Check the source address.  If its OPTIMISTIC
> > +	 * and addr_len is non-zero (implying the sllao option)
> > +	 * then don't send the RS (RFC 4429, section 2.2)
> > +	 */
> > +	ifp = ipv6_get_ifaddr(saddr, dev, 1);
> > +
> > +	if ((!ifp) || ((ifp->flags & IFA_F_OPTIMISTIC) && dev->addr_len))
> > +		return;
> > +
> >  	ndisc_flow_init(&fl, NDISC_ROUTER_SOLICITATION, saddr, daddr,
> >  			dev->ifindex);
> > 
> > @@ -746,6 +772,7 @@ static void ndisc_recv_ns(struct sk_buff *skb)
> >  	int dad = ipv6_addr_any(saddr);
> >  	int inc;
> >  	int is_router;
> > +	int type;
> > 
> >  	if (ipv6_addr_is_multicast(&msg->target)) {
> >  		ND_PRINTK2(KERN_WARNING
> > @@ -796,28 +823,39 @@ static void ndisc_recv_ns(struct sk_buff *skb)
> >  	inc = ipv6_addr_is_multicast(daddr);
> > 
> >  	if ((ifp = ipv6_get_ifaddr(&msg->target, dev, 1)) != NULL) {
> > -		if (ifp->flags & IFA_F_TENTATIVE) {
> > -			/* Address is tentative. If the source
> > -			   is unspecified address, it is someone
> > -			   does DAD, otherwise we ignore solicitations
> > -			   until DAD timer expires.
> > -			 */
> > -			if (!dad)
> > +
> > +		if (ifp->flags & (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)) {
> > +			if (dad) {
> > +				if (dev->type == ARPHRD_IEEE802_TR) {
> > +					unsigned char *sadr = skb->mac.raw;
> > +					if (((sadr[8] ^ dev->dev_addr[0]) & 0x7f) == 0 &&
> > +					    sadr[9] == dev->dev_addr[1] &&
> > +					    sadr[10] == dev->dev_addr[2] &&
> > +					    sadr[11] == dev->dev_addr[3] &&
> > +					    sadr[12] == dev->dev_addr[4] &&
> > +					    sadr[13] == dev->dev_addr[5]) {
> > +						/* looped-back to us */
> > +						goto out;
> > +					}
> > +				}
> > +
> > +				/*
> > +				 * We are colliding with another node
> > +				 * who is doing DAD
> > +				 * so fail our DAD process
> > +				 */
> > +				addrconf_dad_failure(ifp);
> >  				goto out;
> > -			if (dev->type == ARPHRD_IEEE802_TR) {
> > -				unsigned char *sadr = skb->mac.raw;
> > -				if (((sadr[8] ^ dev->dev_addr[0]) & 0x7f) == 0 &&
> > -				    sadr[9] == dev->dev_addr[1] &&
> > -				    sadr[10] == dev->dev_addr[2] &&
> > -				    sadr[11] == dev->dev_addr[3] &&
> > -				    sadr[12] == dev->dev_addr[4] &&
> > -				    sadr[13] == dev->dev_addr[5]) {
> > -					/* looped-back to us */
> > +			} else {
> > +				/*
> > +				 * This is not a dad solicitation.
> > +				 * If we are an optimistic node,
> > +				 * we should respond.
> > +				 * Otherwise, we should ignore it.
> > +				 */
> > +				if (!(ifp->flags & IFA_F_OPTIMISTIC))
> >  					goto out;
> > -				}
> >  			}
> > -			addrconf_dad_failure(ifp);
> > -			return;
> >  		}
> > 
> >  		idev = ifp->idev;
> > @@ -1406,7 +1444,7 @@ void ndisc_send_redirect(struct sk_buff *skb, struct neighbour *neigh,
> > 
> >  	dev = skb->dev;
> > 
> > -	if (ipv6_get_lladdr(dev, &saddr_buf)) {
> > +	if (ipv6_get_lladdr(dev, &saddr_buf, IFA_F_TENTATIVE)) {
> >  		ND_PRINTK2(KERN_WARNING
> >  			   "ICMPv6 Redirect: no link-local address on %s\n",
> >  			   dev->name);
> > -
> > 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
-
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

Powered by blists - more mailing lists