netdev - Re: [PATCHv3 net-next 5/7] net: add confirm_neigh method to dst

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:   Fri, 3 Feb 2017 11:16:16 +0200 (EET)
From:   Julian Anastasov <ja@....bg>
To:     Steffen Klassert <steffen.klassert@...unet.com>
cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-sctp@...r.kernel.org, Neil Horman <nhorman@...driver.com>,
        linux-rdma@...r.kernel.org, YueHaibing <yuehaibing@...wei.com>
Subject: Re: [PATCHv3 net-next 5/7] net: add confirm_neigh method to
 dst_ops


	Hello,

On Fri, 3 Feb 2017, Steffen Klassert wrote:

> On Thu, Feb 02, 2017 at 01:04:34AM +0200, Julian Anastasov wrote:
> > 
> > 	It may sounds good. But only dst->path->ops->confirm_neigh
> > points to real IPv4/IPv6 function. And also, I guess, the
> > family can change while walking the chain, so we should be
> > careful while providing the original daddr (which comes from
> > sendmsg). I had the idea to walk all xforms to get the latest
> > tunnel address but this can be slow. 
> 
> Is this a per packet call or is the information cached somewhere?

	It is for every packet that is sent with both
MSG_CONFIRM and MSG_PROBE, i.e. when nothing is sent
on the wire. It is used by patch 6 just for UDP, RAW, ICMP, L2TP.

> > Something like this?:
> > 
> > static void xfrm_confirm_neigh(const struct dst_entry *dst, const void 
> > *daddr)
> > {
> > 	const struct dst_entry *path = dst->path;
> > 
> > 	/* By default, daddr is from sendmsg() if we have no tunnels */
> > 	for (;dst != path; dst = dst->child) {
> > 		const struct xfrm_state *xfrm = dst->xfrm;
> > 
> > 		/* Use address from last tunnel	*/
> > 		if (xfrm->props.mode != XFRM_MODE_TRANSPORT)
> > 			daddr = &xfrm->id.daddr;
> > 	}
> > 	path->ops->confirm_neigh(path, daddr);
> > }
> 
> I thought about this (completely untested) one:
> 
> static void xfrm_confirm_neigh(const struct dst_entry *dst, const void
> *daddr)
> 
> {
> 	const struct dst_entry *dst = dst->child;

	When starting and dst arg is first xform, the above
assignment skips it. May be both lines should be swapped.

> 	const struct xfrm_state *xfrm = dst->xfrm;
> 
> 	if (xfrm)
> 		daddr = &xfrm->id.daddr;
> 
> 	dst->ops->confirm_neigh(dst, daddr);
> }
> 
> Only the last dst_entry in this call chain (path) sould
> not have dst->xfrm set. So it finally calls path->ops->confirm_neigh
> with the daddr of the last transformation. But your version
> should do the same.

	Above can be fixed but it is risky for the stack
usage when using recursion. In practice, there should not be
many xforms, though. Also, is id.daddr valid for transports?

> > 	This should work as long as path and last tunnel are
> > from same family.
> 
> Yes, the outer mode of the last transformation has the same
> family as path.
> 
> > Also, after checking xfrm_dst_lookup() I'm not
> > sure using just &xfrm->id.daddr is enough. Should we consider
> > more places for daddr value?
> 
> Yes, indeed. We should do it like xfrm_dst_lookup() does it.

	OK, I'll get logic from there. Should I use loop or
recursion?

Regards

--
Julian Anastasov <ja@....bg>