[PATCH net] net: Clear local_df only if crossing namespace.

[PATCH net] net: Clear local_df only if crossing namespace.

[Pravin]

Commit 239c78db9c41a8 (net: clear local_df when passing skb between
namespaces) clears local_df unconditionally.  But upper layer should
be able request local fragmentation within a namespace.  Currently
OVS sets local_df for tunnel packets and then skb_scrub_packe()
resets it which is not correct.

Therefore following patch resets local_df only packet is crossing
namespace.

CC: Templin, Fred L <Fred.L.Templin@boeing.com>
CC: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
 net/core/skbuff.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5976ef0..4ba262f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3905,12 +3905,13 @@ EXPORT_SYMBOL(skb_try_coalesce);
  */
 void skb_scrub_packet(struct sk_buff *skb, bool xnet)
 {
-	if (xnet)
+	if (xnet) {
 		skb_orphan(skb);
+		skb->local_df = 0;
+	}
 	skb->tstamp.tv64 = 0;
 	skb->pkt_type = PACKET_HOST;
 	skb->skb_iif = 0;
-	skb->local_df = 0;
 	skb_dst_drop(skb);
 	skb->mark = 0;
 	secpath_reset(skb);
-- 
1.7.9.5

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Hannes Frederic Sowa]

Hi!

On Fri, Feb 07, 2014 at 02:12:38PM -0800, Pravin wrote:
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3905,12 +3905,13 @@ EXPORT_SYMBOL(skb_try_coalesce);
>   */
>  void skb_scrub_packet(struct sk_buff *skb, bool xnet)
>  {
> -	if (xnet)
> +	if (xnet) {
>  		skb_orphan(skb);
> +		skb->local_df = 0;
> +	}
>  	skb->tstamp.tv64 = 0;
>  	skb->pkt_type = PACKET_HOST;
>  	skb->skb_iif = 0;
> -	skb->local_df = 0;
>  	skb_dst_drop(skb);
>  	skb->mark = 0;
>  	secpath_reset(skb);

I wonder if this should be the right behaviour for tunnels, which should just
do fragmentation based on IP_DF, even if the packet originated locally from a
socket which allowed local fragmentation (inet->pmtudisc < IP_PMTUDISC_DO).

Greetings,

  Hannes

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Pravin Shelar]

On Fri, Feb 7, 2014 at 2:28 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Hi!
>
> On Fri, Feb 07, 2014 at 02:12:38PM -0800, Pravin wrote:
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3905,12 +3905,13 @@ EXPORT_SYMBOL(skb_try_coalesce);
>>   */
>>  void skb_scrub_packet(struct sk_buff *skb, bool xnet)
>>  {
>> -     if (xnet)
>> +     if (xnet) {
>>               skb_orphan(skb);
>> +             skb->local_df = 0;
>> +     }
>>       skb->tstamp.tv64 = 0;
>>       skb->pkt_type = PACKET_HOST;
>>       skb->skb_iif = 0;
>> -     skb->local_df = 0;
>>       skb_dst_drop(skb);
>>       skb->mark = 0;
>>       secpath_reset(skb);
>
> I wonder if this should be the right behaviour for tunnels, which should just
> do fragmentation based on IP_DF, even if the packet originated locally from a
> socket which allowed local fragmentation (inet->pmtudisc < IP_PMTUDISC_DO).
>
This is not about tunneling, skb_scrub_packet() is generic function
which should not reset local_df on all packets.

We can have separate discussion about use of local_df and tunneling in
another thread.

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Hannes Frederic Sowa]

[Cc Nicolas]

On Fri, Feb 07, 2014 at 02:49:20PM -0800, Pravin Shelar wrote:
> On Fri, Feb 7, 2014 at 2:28 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > Hi!
> >
> > On Fri, Feb 07, 2014 at 02:12:38PM -0800, Pravin wrote:
> >> --- a/net/core/skbuff.c
> >> +++ b/net/core/skbuff.c
> >> @@ -3905,12 +3905,13 @@ EXPORT_SYMBOL(skb_try_coalesce);
> >>   */
> >>  void skb_scrub_packet(struct sk_buff *skb, bool xnet)
> >>  {
> >> -     if (xnet)
> >> +     if (xnet) {
> >>               skb_orphan(skb);
> >> +             skb->local_df = 0;
> >> +     }
> >>       skb->tstamp.tv64 = 0;
> >>       skb->pkt_type = PACKET_HOST;
> >>       skb->skb_iif = 0;
> >> -     skb->local_df = 0;
> >>       skb_dst_drop(skb);
> >>       skb->mark = 0;
> >>       secpath_reset(skb);
> >
> > I wonder if this should be the right behaviour for tunnels, which should just
> > do fragmentation based on IP_DF, even if the packet originated locally from a
> > socket which allowed local fragmentation (inet->pmtudisc < IP_PMTUDISC_DO).
> >
> This is not about tunneling, skb_scrub_packet() is generic function
> which should not reset local_df on all packets.
> 
> We can have separate discussion about use of local_df and tunneling in
> another thread.

This change only affects tunnel code as of current net branch, how do
you not expect a discussion about that in this thread, I really wonder?

May I know because of wich vport, vxlan or gre, you did this change?

I am feeling a bit uncomfortable handling remote and local packets that
differently on lower tunnel output (local_df is mostly set on locally
originating packets).

Thanks,

  Hannes

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Pravin Shelar]

On Fri, Feb 7, 2014 at 4:58 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> [Cc Nicolas]
>
> On Fri, Feb 07, 2014 at 02:49:20PM -0800, Pravin Shelar wrote:
>> On Fri, Feb 7, 2014 at 2:28 PM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>> > Hi!
>> >
>> > On Fri, Feb 07, 2014 at 02:12:38PM -0800, Pravin wrote:
>> >> --- a/net/core/skbuff.c
>> >> +++ b/net/core/skbuff.c
>> >> @@ -3905,12 +3905,13 @@ EXPORT_SYMBOL(skb_try_coalesce);
>> >>   */
>> >>  void skb_scrub_packet(struct sk_buff *skb, bool xnet)
>> >>  {
>> >> -     if (xnet)
>> >> +     if (xnet) {
>> >>               skb_orphan(skb);
>> >> +             skb->local_df = 0;
>> >> +     }
>> >>       skb->tstamp.tv64 = 0;
>> >>       skb->pkt_type = PACKET_HOST;
>> >>       skb->skb_iif = 0;
>> >> -     skb->local_df = 0;
>> >>       skb_dst_drop(skb);
>> >>       skb->mark = 0;
>> >>       secpath_reset(skb);
>> >
>> > I wonder if this should be the right behaviour for tunnels, which should just
>> > do fragmentation based on IP_DF, even if the packet originated locally from a
>> > socket which allowed local fragmentation (inet->pmtudisc < IP_PMTUDISC_DO).
>> >
>> This is not about tunneling, skb_scrub_packet() is generic function
>> which should not reset local_df on all packets.
>>
>> We can have separate discussion about use of local_df and tunneling in
>> another thread.
>
> This change only affects tunnel code as of current net branch, how do
> you not expect a discussion about that in this thread, I really wonder?
>
> May I know because of wich vport, vxlan or gre, you did this change?
>
It affects both gre and vxlan.

> I am feeling a bit uncomfortable handling remote and local packets that
> differently on lower tunnel output (local_df is mostly set on locally
> originating packets).

For ip traffic it make sense to turn on local_df only for local
traffic, since for remote case we can send icmp (frag-needed) back to
source. No such thing exist for OVS tunnels. ICMP packet are not
returned to source for the tunnels. That is why to be on safe side,
local_df is turned on for tunnels in OVS.

Thanks.

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Hannes Frederic Sowa]

On Mon, Feb 10, 2014 at 01:00:14PM -0800, Pravin Shelar wrote:
> On Fri, Feb 7, 2014 at 4:58 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > May I know because of wich vport, vxlan or gre, you did this change?
> >
> It affects both gre and vxlan.

Ok, thanks.

> > I am feeling a bit uncomfortable handling remote and local packets that
> > differently on lower tunnel output (local_df is mostly set on locally
> > originating packets).
> 
> For ip traffic it make sense to turn on local_df only for local
> traffic, since for remote case we can send icmp (frag-needed) back to
> source. No such thing exist for OVS tunnels. ICMP packet are not
> returned to source for the tunnels. That is why to be on safe side,
> local_df is turned on for tunnels in OVS.

I have a proposal:

I don't like it that much because of the many arguments. But I currently
don't see another easy solution. Maybe we should make bool xnet an enum and
test with bitops?

I left the clearing of local_df in skb_scrub_packet as we need it for the
dev_forward_skb case and it should be done that in any case.

This diff is slightly compile tested. ;)

I can test and make proper submit if you agree.

What do you think?

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 026a313..630e72f 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1657,7 +1657,7 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
 		return err;
 
 	return iptunnel_xmit(rt, skb, src, dst, IPPROTO_UDP, tos, ttl, df,
-			     false);
+			     false, false);
 }
 EXPORT_SYMBOL_GPL(vxlan_xmit_skb);
 
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 48ed75c..8863002 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -154,7 +154,8 @@ static inline u8 ip_tunnel_ecn_encap(u8 tos, const struct iphdr *iph,
 int iptunnel_pull_header(struct sk_buff *skb, int hdr_len, __be16 inner_proto);
 int iptunnel_xmit(struct rtable *rt, struct sk_buff *skb,
 		  __be32 src, __be32 dst, __u8 proto,
-		  __u8 tos, __u8 ttl, __be16 df, bool xnet);
+		  __u8 tos, __u8 ttl, __be16 df, bool xnet,
+		  bool clear_local_df);
 
 struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb, bool gre_csum,
 					 int gso_type_mask);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 8f519db..5773681 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3903,12 +3903,13 @@ EXPORT_SYMBOL(skb_try_coalesce);
  */
 void skb_scrub_packet(struct sk_buff *skb, bool xnet)
 {
-	if (xnet)
+	if (xnet) {
 		skb_orphan(skb);
+		skb->local_df = 0;
+	}
 	skb->tstamp.tv64 = 0;
 	skb->pkt_type = PACKET_HOST;
 	skb->skb_iif = 0;
-	skb->local_df = 0;
 	skb_dst_drop(skb);
 	skb->mark = 0;
 	secpath_reset(skb);
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index c0e3cb7..2922ec9 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -721,7 +721,8 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 	}
 
 	err = iptunnel_xmit(rt, skb, fl4.saddr, fl4.daddr, protocol,
-			    tos, ttl, df, !net_eq(tunnel->net, dev_net(dev)));
+			    tos, ttl, df, !net_eq(tunnel->net, dev_net(dev)),
+			    true);
 	iptunnel_xmit_stats(err, &dev->stats, dev->tstats);
 
 	return;
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index 6156f4e..93beb04 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -48,13 +48,16 @@
 
 int iptunnel_xmit(struct rtable *rt, struct sk_buff *skb,
 		  __be32 src, __be32 dst, __u8 proto,
-		  __u8 tos, __u8 ttl, __be16 df, bool xnet)
+		  __u8 tos, __u8 ttl, __be16 df, bool xnet, 
+		  bool clear_df)
 {
 	int pkt_len = skb->len;
 	struct iphdr *iph;
 	int err;
 
 	skb_scrub_packet(skb, xnet);
+	if (clear_df)
+		skb->local_df = 0;
 
 	skb_clear_hash(skb);
 	skb_dst_set(skb, &rt->dst);
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 3dfbcf1..cc0be0e 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -974,7 +974,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 	}
 
 	err = iptunnel_xmit(rt, skb, fl4.saddr, fl4.daddr, IPPROTO_IPV6, tos,
-			    ttl, df, !net_eq(tunnel->net, dev_net(dev)));
+			    ttl, df, !net_eq(tunnel->net, dev_net(dev)), true);
 	iptunnel_xmit_stats(err, &dev->stats, dev->tstats);
 	return NETDEV_TX_OK;
 

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Pravin Shelar]

On Mon, Feb 10, 2014 at 6:11 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Mon, Feb 10, 2014 at 01:00:14PM -0800, Pravin Shelar wrote:
>> On Fri, Feb 7, 2014 at 4:58 PM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>> > May I know because of wich vport, vxlan or gre, you did this change?
>> >
>> It affects both gre and vxlan.
>
> Ok, thanks.
>
>> > I am feeling a bit uncomfortable handling remote and local packets that
>> > differently on lower tunnel output (local_df is mostly set on locally
>> > originating packets).
>>
>> For ip traffic it make sense to turn on local_df only for local
>> traffic, since for remote case we can send icmp (frag-needed) back to
>> source. No such thing exist for OVS tunnels. ICMP packet are not
>> returned to source for the tunnels. That is why to be on safe side,
>> local_df is turned on for tunnels in OVS.
>
> I have a proposal:
>
> I don't like it that much because of the many arguments. But I currently
> don't see another easy solution. Maybe we should make bool xnet an enum and
> test with bitops?
>
> I left the clearing of local_df in skb_scrub_packet as we need it for the
> dev_forward_skb case and it should be done that in any case.
>
> This diff is slightly compile tested. ;)
>
> I can test and make proper submit if you agree.
>
> What do you think?
>

I am not sure why the caller can not just set skb->local_df before
calling iptunnel_xmit() rather than passing extra arg to this
function?
There are not that many caller of this function.

Thanks,
Pravin.

> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 026a313..630e72f 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1657,7 +1657,7 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
>                 return err;
>
>         return iptunnel_xmit(rt, skb, src, dst, IPPROTO_UDP, tos, ttl, df,
> -                            false);
> +                            false, false);
>  }
>  EXPORT_SYMBOL_GPL(vxlan_xmit_skb);
>
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 48ed75c..8863002 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -154,7 +154,8 @@ static inline u8 ip_tunnel_ecn_encap(u8 tos, const struct iphdr *iph,
>  int iptunnel_pull_header(struct sk_buff *skb, int hdr_len, __be16 inner_proto);
>  int iptunnel_xmit(struct rtable *rt, struct sk_buff *skb,
>                   __be32 src, __be32 dst, __u8 proto,
> -                 __u8 tos, __u8 ttl, __be16 df, bool xnet);
> +                 __u8 tos, __u8 ttl, __be16 df, bool xnet,
> +                 bool clear_local_df);
>
>  struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb, bool gre_csum,
>                                          int gso_type_mask);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 8f519db..5773681 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3903,12 +3903,13 @@ EXPORT_SYMBOL(skb_try_coalesce);
>   */
>  void skb_scrub_packet(struct sk_buff *skb, bool xnet)
>  {
> -       if (xnet)
> +       if (xnet) {
>                 skb_orphan(skb);
> +               skb->local_df = 0;
> +       }
>         skb->tstamp.tv64 = 0;
>         skb->pkt_type = PACKET_HOST;
>         skb->skb_iif = 0;
> -       skb->local_df = 0;
>         skb_dst_drop(skb);
>         skb->mark = 0;
>         secpath_reset(skb);
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index c0e3cb7..2922ec9 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -721,7 +721,8 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
>         }
>
>         err = iptunnel_xmit(rt, skb, fl4.saddr, fl4.daddr, protocol,
> -                           tos, ttl, df, !net_eq(tunnel->net, dev_net(dev)));
> +                           tos, ttl, df, !net_eq(tunnel->net, dev_net(dev)),
> +                           true);
>         iptunnel_xmit_stats(err, &dev->stats, dev->tstats);
>
>         return;
> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
> index 6156f4e..93beb04 100644
> --- a/net/ipv4/ip_tunnel_core.c
> +++ b/net/ipv4/ip_tunnel_core.c
> @@ -48,13 +48,16 @@
>
>  int iptunnel_xmit(struct rtable *rt, struct sk_buff *skb,
>                   __be32 src, __be32 dst, __u8 proto,
> -                 __u8 tos, __u8 ttl, __be16 df, bool xnet)
> +                 __u8 tos, __u8 ttl, __be16 df, bool xnet,
> +                 bool clear_df)
>  {
>         int pkt_len = skb->len;
>         struct iphdr *iph;
>         int err;
>
>         skb_scrub_packet(skb, xnet);
> +       if (clear_df)
> +               skb->local_df = 0;
>
>         skb_clear_hash(skb);
>         skb_dst_set(skb, &rt->dst);
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index 3dfbcf1..cc0be0e 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -974,7 +974,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
>         }
>
>         err = iptunnel_xmit(rt, skb, fl4.saddr, fl4.daddr, IPPROTO_IPV6, tos,
> -                           ttl, df, !net_eq(tunnel->net, dev_net(dev)));
> +                           ttl, df, !net_eq(tunnel->net, dev_net(dev)), true);
>         iptunnel_xmit_stats(err, &dev->stats, dev->tstats);
>         return NETDEV_TX_OK;
>

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Nicolas Dichtel]

Le 12/02/2014 05:26, Pravin Shelar a écrit :
> On Mon, Feb 10, 2014 at 6:11 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> On Mon, Feb 10, 2014 at 01:00:14PM -0800, Pravin Shelar wrote:
>>> On Fri, Feb 7, 2014 at 4:58 PM, Hannes Frederic Sowa
>>> <hannes@stressinduktion.org> wrote:
>>>> May I know because of wich vport, vxlan or gre, you did this change?
>>>>
>>> It affects both gre and vxlan.
>>
>> Ok, thanks.
>>
>>>> I am feeling a bit uncomfortable handling remote and local packets that
>>>> differently on lower tunnel output (local_df is mostly set on locally
>>>> originating packets).
>>>
>>> For ip traffic it make sense to turn on local_df only for local
>>> traffic, since for remote case we can send icmp (frag-needed) back to
>>> source. No such thing exist for OVS tunnels. ICMP packet are not
>>> returned to source for the tunnels. That is why to be on safe side,
>>> local_df is turned on for tunnels in OVS.
>>
>> I have a proposal:
>>
>> I don't like it that much because of the many arguments. But I currently
>> don't see another easy solution. Maybe we should make bool xnet an enum and
>> test with bitops?
>>
>> I left the clearing of local_df in skb_scrub_packet as we need it for the
>> dev_forward_skb case and it should be done that in any case.
>>
>> This diff is slightly compile tested. ;)
>>
>> I can test and make proper submit if you agree.
>>
>> What do you think?
>>
>
> I am not sure why the caller can not just set skb->local_df before
> calling iptunnel_xmit() rather than passing extra arg to this
> function?
> There are not that many caller of this function.
The benefit is that it ensures that future callers will think about this point
;-)

Steffen is reworking vti code and will use skb_scrub_packet(), I CC'ed him in
case he has some comment about this change.


Regards,
Nicolas

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Pravin Shelar]

On Wed, Feb 12, 2014 at 1:32 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> Le 12/02/2014 05:26, Pravin Shelar a écrit :
>
>> On Mon, Feb 10, 2014 at 6:11 PM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>>>
>>> On Mon, Feb 10, 2014 at 01:00:14PM -0800, Pravin Shelar wrote:
>>>>
>>>> On Fri, Feb 7, 2014 at 4:58 PM, Hannes Frederic Sowa
>>>> <hannes@stressinduktion.org> wrote:
>>>>>
>>>>> May I know because of wich vport, vxlan or gre, you did this change?
>>>>>
>>>> It affects both gre and vxlan.
>>>
>>>
>>> Ok, thanks.
>>>
>>>>> I am feeling a bit uncomfortable handling remote and local packets that
>>>>> differently on lower tunnel output (local_df is mostly set on locally
>>>>> originating packets).
>>>>
>>>>
>>>> For ip traffic it make sense to turn on local_df only for local
>>>> traffic, since for remote case we can send icmp (frag-needed) back to
>>>> source. No such thing exist for OVS tunnels. ICMP packet are not
>>>> returned to source for the tunnels. That is why to be on safe side,
>>>> local_df is turned on for tunnels in OVS.
>>>
>>>
>>> I have a proposal:
>>>
>>> I don't like it that much because of the many arguments. But I currently
>>> don't see another easy solution. Maybe we should make bool xnet an enum
>>> and
>>> test with bitops?
>>>
>>> I left the clearing of local_df in skb_scrub_packet as we need it for the
>>> dev_forward_skb case and it should be done that in any case.
>>>
>>> This diff is slightly compile tested. ;)
>>>
>>> I can test and make proper submit if you agree.
>>>
>>> What do you think?
>>>
>>
>> I am not sure why the caller can not just set skb->local_df before
>> calling iptunnel_xmit() rather than passing extra arg to this
>> function?
>> There are not that many caller of this function.
>
> The benefit is that it ensures that future callers will think about this
> point
> ;-)
>

But that add extra test cases in fast path.
For example OVS we can not reset skb->mark in skb_scrub_packet(). I am
going to send patch for that too. Do you think I should add another
argument for skb-mark clear too ?
We can not make code every piece of code future proof. thats why we
have review process.

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Hannes Frederic Sowa]

On Wed, Feb 12, 2014 at 10:32:49AM +0100, Nicolas Dichtel wrote:
> Le 12/02/2014 05:26, Pravin Shelar a écrit :
> >On Mon, Feb 10, 2014 at 6:11 PM, Hannes Frederic Sowa
> >I am not sure why the caller can not just set skb->local_df before
> >calling iptunnel_xmit() rather than passing extra arg to this
> >function?
> >There are not that many caller of this function.
> The benefit is that it ensures that future callers will think about this 
> point
> ;-)

Exactly, I thought about adding skb->local_df = 0 to all the tunnel code but
wanted to force users of the interface to think about the parameter and its
consequences.

Greetings,

  Hannes

Re: [PATCH net] net: Clear local_df only if crossing namespace.

[Nicolas Dichtel]

Le 12/02/2014 18:05, Pravin Shelar a écrit :
> On Wed, Feb 12, 2014 at 1:32 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>> Le 12/02/2014 05:26, Pravin Shelar a écrit :
>>
>>> On Mon, Feb 10, 2014 at 6:11 PM, Hannes Frederic Sowa
>>> <hannes@stressinduktion.org> wrote:
>>>>
>>>> On Mon, Feb 10, 2014 at 01:00:14PM -0800, Pravin Shelar wrote:
>>>>>
>>>>> On Fri, Feb 7, 2014 at 4:58 PM, Hannes Frederic Sowa
>>>>> <hannes@stressinduktion.org> wrote:
>>>>>>
>>>>>> May I know because of wich vport, vxlan or gre, you did this change?
>>>>>>
>>>>> It affects both gre and vxlan.
>>>>
>>>>
>>>> Ok, thanks.
>>>>
>>>>>> I am feeling a bit uncomfortable handling remote and local packets that
>>>>>> differently on lower tunnel output (local_df is mostly set on locally
>>>>>> originating packets).
>>>>>
>>>>>
>>>>> For ip traffic it make sense to turn on local_df only for local
>>>>> traffic, since for remote case we can send icmp (frag-needed) back to
>>>>> source. No such thing exist for OVS tunnels. ICMP packet are not
>>>>> returned to source for the tunnels. That is why to be on safe side,
>>>>> local_df is turned on for tunnels in OVS.
>>>>
>>>>
>>>> I have a proposal:
>>>>
>>>> I don't like it that much because of the many arguments. But I currently
>>>> don't see another easy solution. Maybe we should make bool xnet an enum
>>>> and
>>>> test with bitops?
>>>>
>>>> I left the clearing of local_df in skb_scrub_packet as we need it for the
>>>> dev_forward_skb case and it should be done that in any case.
>>>>
>>>> This diff is slightly compile tested. ;)
>>>>
>>>> I can test and make proper submit if you agree.
>>>>
>>>> What do you think?
>>>>
>>>
>>> I am not sure why the caller can not just set skb->local_df before
>>> calling iptunnel_xmit() rather than passing extra arg to this
>>> function?
>>> There are not that many caller of this function.
>>
>> The benefit is that it ensures that future callers will think about this
>> point
>> ;-)
>>
>
> But that add extra test cases in fast path.
> For example OVS we can not reset skb->mark in skb_scrub_packet(). I am
> going to send patch for that too. Do you think I should add another
> argument for skb-mark clear too ?
Maybe this will be the same argument than local_df: 'bool ovs' (probably find a
better name ;-))