| Message ID | 4EB8E0B8.40500@cn.fujitsu.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Li Wei <lw@cn.fujitsu.com> Date: Tue, 08 Nov 2011 15:56:40 +0800 > Since commit 7be799a7 (ipv4: Remove rt->rt_dst reference from > ip_forward_options()) and commit 0374d9ce (ipv4: Kill spurious > write to iph->daddr in ip_forward_options()) we use iph->daddr > for SRR option matching and assume iph->daddr equals to rt->rt_dst, > Unfortunately skb_rtable(skb) has been updated in ip_options_rcv_srr() > for the nexthop in SRR option but iph->daddr *not* updated, > We should use the updated rt->rt_dst for SRR option matching > and update iph->daddr here. > > Signed-off-by: Li Wei <lw@cn.fujitsu.com> Please replace this by whatever logic ip_options_rcv_srr() uses to determine the destination address. I would strongly encourage you, when fixing bugs like this, to use as a hint the intentions of the commit which introduced the bug. And try as hard as possible to retain the goals of the guilty commit. In this case, that means not introducing references to rt->rt_dst back into the code. Thank you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Li Wei <lw@cn.fujitsu.com> > Date: Tue, 08 Nov 2011 15:56:40 +0800 > >> Since commit 7be799a7 (ipv4: Remove rt->rt_dst reference from >> ip_forward_options()) and commit 0374d9ce (ipv4: Kill spurious >> write to iph->daddr in ip_forward_options()) we use iph->daddr >> for SRR option matching and assume iph->daddr equals to rt->rt_dst, >> Unfortunately skb_rtable(skb) has been updated in ip_options_rcv_srr() >> for the nexthop in SRR option but iph->daddr *not* updated, >> We should use the updated rt->rt_dst for SRR option matching >> and update iph->daddr here. >> >> Signed-off-by: Li Wei <lw@cn.fujitsu.com> > > Please replace this by whatever logic ip_options_rcv_srr() uses to > determine the destination address. > > I would strongly encourage you, when fixing bugs like this, to use > as a hint the intentions of the commit which introduced the bug. And > try as hard as possible to retain the goals of the guilty commit. > > In this case, that means not introducing references to rt->rt_dst > back into the code. > > Thank you. > > Thank you for your advice, I reviewed the code again think that as you said in commit def57687, "No matter what kind of header mangling occurs due to IP options processing, rt->rt_dst will always equal iph->daddr in the packet", iph->daddr in ip_options_rcv_srr() should be updated either as skb_rtable(skb) has been updated for 'nexthop'. So we can elide all rt->rt_dst reference in ip_forward() and ip_forward_options(). I will submit another patch to fix this bug. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c index ec93335..8dca67c 100644 --- a/net/ipv4/ip_options.c +++ b/net/ipv4/ip_options.c @@ -568,12 +568,13 @@ void ip_forward_options(struct sk_buff *skb) ) { if (srrptr + 3 > srrspace) break; - if (memcmp(&ip_hdr(skb)->daddr, &optptr[srrptr-1], 4) == 0) + if (memcmp(&rt->rt_dst, &optptr[srrptr-1], 4) == 0) break; } if (srrptr + 3 <= srrspace) { opt->is_changed = 1; ip_rt_get_source(&optptr[srrptr-1], skb, rt); + ip_hdr(skb)->daddr = rt->rt_dst; optptr[2] = srrptr+4; } else if (net_ratelimit()) printk(KERN_CRIT "ip_forward(): Argh! Destination lost!\n");
Since commit 7be799a7 (ipv4: Remove rt->rt_dst reference from ip_forward_options()) and commit 0374d9ce (ipv4: Kill spurious write to iph->daddr in ip_forward_options()) we use iph->daddr for SRR option matching and assume iph->daddr equals to rt->rt_dst, Unfortunately skb_rtable(skb) has been updated in ip_options_rcv_srr() for the nexthop in SRR option but iph->daddr *not* updated, We should use the updated rt->rt_dst for SRR option matching and update iph->daddr here. Signed-off-by: Li Wei <lw@cn.fujitsu.com> --- net/ipv4/ip_options.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)