xfrm_neigh_lookup and dst->path

xfrm_neigh_lookup and dst->path

[Julian Anastasov]

	Hello,

On Fri, 3 Feb 2017, Steffen Klassert wrote:

> On Thu, Feb 02, 2017 at 01:04:34AM +0200, Julian Anastasov wrote:
> > 
> > 	For now I installed common handler, just like
> > xfrm_neigh_lookup. BTW this function has problem from
> > commit f894cbf847c9, it looks like dst is wrongly provided,
> > first arg should be dst->path.
> 
> Yes, this should use dst->path of course. I really wonder why
> nobody noticed this for the last five years.

	IMHO, xfrm_neigh_lookup is not used often. If used,
it should be from a place before skb->dst xforms are popped in
xfrm_output_one, i.e. somewhere before the output processing.
I see that dst_neigh_lookup*() is used mostly in xmit handlers
of tunnel devices, I assume, after xfrm_output_one().
Not sure for br_nf_pre_routing_finish_bridge().

	I'm trying to fix this code but we have the
same problem as in xfrm_confirm_neigh: to find address
of last tunnel. Should we use the same loop?

Regards

Re: xfrm_neigh_lookup and dst->path

[Steffen Klassert]

On Sat, Feb 11, 2017 at 03:12:44PM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Fri, 3 Feb 2017, Steffen Klassert wrote:
> 
> > On Thu, Feb 02, 2017 at 01:04:34AM +0200, Julian Anastasov wrote:
> > > 
> > > 	For now I installed common handler, just like
> > > xfrm_neigh_lookup. BTW this function has problem from
> > > commit f894cbf847c9, it looks like dst is wrongly provided,
> > > first arg should be dst->path.
> > 
> > Yes, this should use dst->path of course. I really wonder why
> > nobody noticed this for the last five years.
> 
> 	IMHO, xfrm_neigh_lookup is not used often.

I really wonder if it is used at all, given that this bug exists
for more than five years now and nobody reported it so far.

> If used,
> it should be from a place before skb->dst xforms are popped in
> xfrm_output_one, i.e. somewhere before the output processing.
> I see that dst_neigh_lookup*() is used mostly in xmit handlers
> of tunnel devices, I assume, after xfrm_output_one().
> Not sure for br_nf_pre_routing_finish_bridge().
> 
> 	I'm trying to fix this code but we have the
> same problem as in xfrm_confirm_neigh: to find address
> of last tunnel. Should we use the same loop?

Yes, I think this is the right thing to do.

Thanks!