netdev - Re: [PATCH 2/2] ipv4: Change rt->rt

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:	Tue, 24 Jul 2012 10:41:37 +0300 (EEST)
From:	Julian Anastasov <ja@....bg>
To:	David Miller <davem@...emloft.net>
cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] ipv4: Change rt->rt_iif encoding.


	Hello,

On Mon, 23 Jul 2012, David Miller wrote:

> From: Julian Anastasov <ja@....bg>
> Date: Tue, 24 Jul 2012 03:24:38 +0300 (EEST)
> 
> > On Mon, 23 Jul 2012, David Miller wrote:
> > 
> >> Hmmm, the problem is that when we decapsulate VLAN devices, we're left
> >> with the parent device's index in skb->skb_iif.
> > 
> > 	Not sure if it is a problem with VLANs, can be also
> > with some virtual devices but may be they use dev_forward_skb()
> > where skb_iif is zeroed.
> 
> dev_forward_skb() gives the packet to netif_rx() which will thus send
> the packet down to __netif_receive_skb() which will set the
> skb->skb_iif to the new device's ifindex.  It will not stay at zero
> :-)
> 
> > 	I was also worrying about ip_mc_output looping packets with
> > skb_iif because skb_clone copies the field but may be such loops
> > happen only for locally originated traffic where skb_iif starts with
> > 0.
> 
> Loopback of multicast packets is done in ip_mc_output(), via
> the clone that you mention, via dev_loopback_xmit().
> 
> dev_loopback_xmit() gives the packet to netif_rx_ni() which again
> lands it back at __netif_receive_skb(), which (with my changes)
> will adjust the skb->skb_iif to match whatever sits in skb->dev
> at the time.
> 
> It should be alright.

	Yes, I know that now it is set in all cases, I was
wondering why __netif_receive_skb was prepared to see
non-zero value. Please also check xfrm4_fill_dst, the
possible case where the new skb_iif usage can differ from
rt_iif is for forwarded packets where skb->dst is changed
with new version, for example:

ip_forward -> *xfrm*_policy_check* -> __xfrm_decode_session ->
_decode_session4 memsets flowi4_iif (now to get skb_iif).

ip_forward -> xfrm4_route_forward -> ... xfrm_lookup ->
... -> xfrm4_fill_dst now will use skb_iif ?

	XFRM is still magic for me and I'm not sure if
we need some other device instead of skb_iif. Should we
prefer dst->dev for output routes? For example:

static inline int inet_iif(const struct sk_buff *skb)
{
	struct rtable *rt = skb_rtable(skb);
	int iif = rt->rt_iif;

	if (iif)
		return iif;
	if (rt_is_output_route(rt))
		return rt->dst.dev->ifindex;
	return skb->skb_iif;
}

	but may be it is same if we restore the
rth->rt_iif	= orig_oif ? : dev_out->ifindex;

	code in __mkroute_output? By this way we will have
correct rt_iif at the time of routing call, before the
packet is looped via this device. Then skb_iif will remain
as default only for input routes. By this way we will be
less worried for places like xfrm or when skb->dst is
replaced in forwarding path. IPIP tunnels also replace
skb->dst but this can hurt only code like route4_classify.

Regards

--
Julian Anastasov <ja@....bg>
--
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