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] [day] [month] [year] [list]

Date:   Fri, 3 Feb 2017 11:08:53 +0100
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Julian Anastasov <ja@....bg>
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

On Fri, Feb 03, 2017 at 11:16:16AM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Fri, 3 Feb 2017, Steffen Klassert wrote:
> > 
> > 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.

Yes, that's better :)

> 
> > 	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?

Yes, it is needed for the lookup. But id.daddr ist the same
as daddr of the packet on transport mode.

> 
> > > 	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?

I don't have a strong opinion on that. Both should work,
choose whatever you prefer.