Re: [PATCH net-next] vrf: plug skb leaks

[David Ahern <dsa@cumulusnetworks.com>]

On 8/19/15 1:17 PM, Nikolay Aleksandrov wrote:
>
>> On Aug 19, 2015, at 8:27 PM, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>
>>
>> On Aug 19, 2015 20:13, "David Ahern" <dsa@cumulusnetworks.com> wrote:
>>>
>>> Hi Nikolay:
>>>
>>>
>>> On 8/18/15 8:12 PM, Nikolay Aleksandrov wrote:
>>>>
>>>> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
>>>> index ed208317cbb5..4aa06450fafa 100644
>>>> --- a/drivers/net/vrf.c
>>>> +++ b/drivers/net/vrf.c
>>>> @@ -97,6 +97,12 @@ static bool is_ip_rx_frame(struct sk_buff *skb)
>>>>          return false;
>>>>    }
>>>>
>>>> +static void vrf_tx_error(struct net_device *vrf_dev, struct sk_buff *skb)
>>>> +{
>>>> +       vrf_dev->stats.tx_errors++;
>>>> +       kfree_skb(skb);
>>>> +}
>>>> +
>>>>    /* note: already called with rcu_read_lock */
>>>>    static rx_handler_result_t vrf_handle_frame(struct sk_buff **pskb)
>>>>    {
>>>> @@ -149,7 +155,8 @@ static struct rtnl_link_stats64 *vrf_get_stats64(struct net_device *dev,
>>>>    static netdev_tx_t vrf_process_v6_outbound(struct sk_buff *skb,
>>>>                                             struct net_device *dev)
>>>>    {
>>>> -       return 0;
>>>> +       vrf_tx_error(dev, skb);
>>>> +       return NET_XMIT_DROP;
>>>>    }
>>>>
>>>>    static int vrf_send_v4_prep(struct sk_buff *skb, struct flowi4 *fl4,
>>>> @@ -206,8 +213,7 @@ static netdev_tx_t vrf_process_v4_outbound(struct sk_buff *skb,
>>>>    out:
>>>>          return ret;
>>>>    err:
>>>> -       vrf_dev->stats.tx_errors++;
>>>> -       kfree_skb(skb);
>>>> +       vrf_tx_error(vrf_dev, skb);
>>>>          goto out;
>>>>    }
>>>>
>>>> @@ -219,6 +225,7 @@ static netdev_tx_t is_ip_tx_frame(struct sk_buff *skb, struct net_device *dev)
>>>>          case htons(ETH_P_IPV6):
>>>>                  return vrf_process_v6_outbound(skb, dev);
>>>>          default:
>>>> +               vrf_tx_error(dev, skb);
>>>>                  return NET_XMIT_DROP;
>>>>          }
>>>>    }
>>>>
>>>
>>> Would be simpler to do the vrf_tx_error at the end of is_ip_tx_frame() if ret == NET_XMIT_DROP.
>>>
>>> David
>>>
>>
>> Sure, that will work too.
>>
>
> Actually no, this will not work because ip_local_out() can return NET_XMIT_DROP and the packet
> can already be dropped. I’d prefer to keep these cases separate and explicit as they are in my patch.
>

ok. Then the patch looks good to me.

Acked-by: David Ahern <dsa@cumulusnetworks.com>