| Message ID | 1496883476-17445-1-git-send-email-yanhaishuang@cmss.chinamobile.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan <yanhaishuang@cmss.chinamobile.com> wrote: > When ip_tunnel_rcv fails, the tun_dst won't be freed, so move > skb_dst_set to begin and tun_dst would be freed by kfree_skb. > > CC: Pravin B Shelar <pshelar@nicira.com> > Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.") > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e> > --- > net/ipv4/ip_tunnel.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c > index b878ecb..27fc20f 100644 > --- a/net/ipv4/ip_tunnel.c > +++ b/net/ipv4/ip_tunnel.c > @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb, > const struct iphdr *iph = ip_hdr(skb); > int err; > > + if (tun_dst) > + skb_dst_set(skb, (struct dst_entry *)tun_dst); > + If dst is set so early, skb_scrub_packet() would remove the tunnel dst reference. It is better to call skb_dst_drop() from error code path. > #ifdef CONFIG_NET_IPGRE_BROADCAST > if (ipv4_is_multicast(iph->daddr)) { > tunnel->dev->stats.multicast++; > @@ -439,9 +442,6 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb, > skb->dev = tunnel->dev; > } > > - if (tun_dst) > - skb_dst_set(skb, (struct dst_entry *)tun_dst); > - > gro_cells_receive(&tunnel->gro_cells, skb); > return 0; > > -- > 1.8.3.1 > > >
On Thu, 2017-06-08 at 08:57 +0800, Haishuang Yan wrote: > When ip_tunnel_rcv fails, the tun_dst won't be freed, so move > skb_dst_set to begin and tun_dst would be freed by kfree_skb. > > CC: Pravin B Shelar <pshelar@nicira.com> > Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.") > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e> > --- Acked-by: Eric Dumazet <edumazet@google.com> Thanks a lot.
> On 8 Jun 2017, at 10:13 AM, Pravin Shelar <pshelar@ovn.org> wrote: > > On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan > <yanhaishuang@cmss.chinamobile.com> wrote: >> When ip_tunnel_rcv fails, the tun_dst won't be freed, so move >> skb_dst_set to begin and tun_dst would be freed by kfree_skb. >> >> CC: Pravin B Shelar <pshelar@nicira.com> >> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.") >> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e> >> --- >> net/ipv4/ip_tunnel.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c >> index b878ecb..27fc20f 100644 >> --- a/net/ipv4/ip_tunnel.c >> +++ b/net/ipv4/ip_tunnel.c >> @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb, >> const struct iphdr *iph = ip_hdr(skb); >> int err; >> >> + if (tun_dst) >> + skb_dst_set(skb, (struct dst_entry *)tun_dst); >> + > If dst is set so early, skb_scrub_packet() would remove the tunnel dst > reference. > It is better to call skb_dst_drop() from error code path. Yes, I will change it in v3 commit, thanks! > >> #ifdef CONFIG_NET_IPGRE_BROADCAST >> if (ipv4_is_multicast(iph->daddr)) { >> tunnel->dev->stats.multicast++; >> @@ -439,9 +442,6 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb, >> skb->dev = tunnel->dev; >> } >> >> - if (tun_dst) >> - skb_dst_set(skb, (struct dst_entry *)tun_dst); >> - >> gro_cells_receive(&tunnel->gro_cells, skb); >> return 0; >> >> -- >> 1.8.3.1 >> >> >> >
On Wed, 2017-06-07 at 19:13 -0700, Pravin Shelar wrote: > On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan > <yanhaishuang@cmss.chinamobile.com> wrote: > > When ip_tunnel_rcv fails, the tun_dst won't be freed, so move > > skb_dst_set to begin and tun_dst would be freed by kfree_skb. > > > > CC: Pravin B Shelar <pshelar@nicira.com> > > Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.") > > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e> > > --- > > net/ipv4/ip_tunnel.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c > > index b878ecb..27fc20f 100644 > > --- a/net/ipv4/ip_tunnel.c > > +++ b/net/ipv4/ip_tunnel.c > > @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb, > > const struct iphdr *iph = ip_hdr(skb); > > int err; > > > > + if (tun_dst) > > + skb_dst_set(skb, (struct dst_entry *)tun_dst); > > + > If dst is set so early, skb_scrub_packet() would remove the tunnel dst > reference. > It is better to call skb_dst_drop() from error code path. Do we really want to keep a dst from another namespace if skb_scrub_packet() is called with xnet=true ?
On Wed, Jun 7, 2017 at 8:15 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Wed, 2017-06-07 at 19:13 -0700, Pravin Shelar wrote: >> On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan >> <yanhaishuang@cmss.chinamobile.com> wrote: >> > When ip_tunnel_rcv fails, the tun_dst won't be freed, so move >> > skb_dst_set to begin and tun_dst would be freed by kfree_skb. >> > >> > CC: Pravin B Shelar <pshelar@nicira.com> >> > Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.") >> > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e> >> > --- >> > net/ipv4/ip_tunnel.c | 6 +++--- >> > 1 file changed, 3 insertions(+), 3 deletions(-) >> > >> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c >> > index b878ecb..27fc20f 100644 >> > --- a/net/ipv4/ip_tunnel.c >> > +++ b/net/ipv4/ip_tunnel.c >> > @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb, >> > const struct iphdr *iph = ip_hdr(skb); >> > int err; >> > >> > + if (tun_dst) >> > + skb_dst_set(skb, (struct dst_entry *)tun_dst); >> > + >> If dst is set so early, skb_scrub_packet() would remove the tunnel dst >> reference. >> It is better to call skb_dst_drop() from error code path. > > Do we really want to keep a dst from another namespace if > skb_scrub_packet() is called with xnet=true ? > tun_dst is allocated in same namespace. It is required for LWT to work.
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c index b878ecb..27fc20f 100644 --- a/net/ipv4/ip_tunnel.c +++ b/net/ipv4/ip_tunnel.c @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb, const struct iphdr *iph = ip_hdr(skb); int err; + if (tun_dst) + skb_dst_set(skb, (struct dst_entry *)tun_dst); + #ifdef CONFIG_NET_IPGRE_BROADCAST if (ipv4_is_multicast(iph->daddr)) { tunnel->dev->stats.multicast++; @@ -439,9 +442,6 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb, skb->dev = tunnel->dev; } - if (tun_dst) - skb_dst_set(skb, (struct dst_entry *)tun_dst); - gro_cells_receive(&tunnel->gro_cells, skb); return 0;
When ip_tunnel_rcv fails, the tun_dst won't be freed, so move skb_dst_set to begin and tun_dst would be freed by kfree_skb. CC: Pravin B Shelar <pshelar@nicira.com> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.") Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e> --- net/ipv4/ip_tunnel.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)