Re: [PATCH] skbuff: ensure to reset dev in skb_scrub_packet()

[Nicolas Dichtel <nicolas.dichtel@6wind.com>]

Le 19/07/2013 20:21, Pravin Shelar a écrit :
> On Fri, Jul 19, 2013 at 7:41 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>> Because this function is used to scrub a packet when it cross netns, we must
>> ensure that skb->dev points to the new netns.
>>
>> This was done by eth_type_trans() in dev_forward_skb(), but it's also needed
>> for ip tunnels.
>>
>> I take the opportunity to move the call of skb_scrub_packet() after
>> eth_type_trans(), to be sure that pkt_type is set to PACKET_HOST.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>   include/linux/skbuff.h | 3 ++-
>>   net/core/dev.c         | 6 +++---
>>   net/core/skbuff.c      | 3 ++-
>>   net/ipv4/ip_tunnel.c   | 9 +++++----
>>   net/ipv6/sit.c         | 4 ++--
>>   5 files changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 5afefa01a13c..620ecce0a717 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -2385,7 +2385,8 @@ extern void              skb_split(struct sk_buff *skb,
>>                                   struct sk_buff *skb1, const u32 len);
>>   extern int            skb_shift(struct sk_buff *tgt, struct sk_buff *skb,
>>                                   int shiftlen);
>> -extern void           skb_scrub_packet(struct sk_buff *skb);
>> +extern void           skb_scrub_packet(struct sk_buff *skb,
>> +                                       struct net_device *dev);
>>
>>   extern struct sk_buff *skb_segment(struct sk_buff *skb,
>>                                     netdev_features_t features);
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 26755dd40daa..6f789b99331b 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1691,13 +1691,13 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
>>                  kfree_skb(skb);
>>                  return NET_RX_DROP;
>>          }
>> -       skb_scrub_packet(skb);
>>          skb->protocol = eth_type_trans(skb, dev);
>>
>>          /* eth_type_trans() can set pkt_type.
>> -        * clear pkt_type _after_ calling eth_type_trans()
>> +        * call skb_scrub_packet() after it to clear pkt_type _after_ calling
>> +        * eth_type_trans().
>>           */
>> -       skb->pkt_type = PACKET_HOST;
>> +       skb_scrub_packet(skb, dev);
>>
>>          return netif_rx(skb);
>>   }
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 20e02d2605ec..5f4701f89af8 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3507,13 +3507,14 @@ EXPORT_SYMBOL(skb_try_coalesce);
>>    * another namespace. We have to clear all information in the skb that
>>    * could impact namespace isolation.
>>    */
>> -void skb_scrub_packet(struct sk_buff *skb)
>> +void skb_scrub_packet(struct sk_buff *skb, struct net_device *dev)
>>   {
>>          skb_orphan(skb);
>>          skb->tstamp.tv64 = 0;
>>          skb->pkt_type = PACKET_HOST;
>>          skb->skb_iif = 0;
>>          skb_dst_drop(skb);
>> +       skb->dev = dev;
>>          skb->mark = 0;
>>          secpath_reset(skb);
>>          nf_reset(skb);
>> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
>> index ca1cb2d5f6e2..2e88321c7f23 100644
>> --- a/net/ipv4/ip_tunnel.c
>> +++ b/net/ipv4/ip_tunnel.c
>> @@ -454,15 +454,16 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>>          tstats->rx_bytes += skb->len;
>>          u64_stats_update_end(&tstats->syncp);
>>
>> -       if (tunnel->net != dev_net(tunnel->dev))
>> -               skb_scrub_packet(skb);
>> -
>>          if (tunnel->dev->type == ARPHRD_ETHER) {
>>                  skb->protocol = eth_type_trans(skb, tunnel->dev);
>>                  skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
>>          } else {
>>                  skb->dev = tunnel->dev;
>>          }
>> +
>> +       if (tunnel->net != dev_net(tunnel->dev))
>> +               skb_scrub_packet(skb, tunnel->dev);
>> +
>
> It is done in ip_tunnels right above the statement. Where exactly are
> we missing skb->dev set to tunnel->dev?
On the xmit path, ipip6_tunnel_xmit() for example.

And note also, that skb_scrub_packet() is used for netns crossing, hence this 
function should be complete and must not leave some field with pointer to the 
previous netns.