diff mbox

[net-next] vrf: plug skb leaks

Message ID 1439953949-23736-1-git-send-email-razor@blackwall.org
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Nikolay Aleksandrov Aug. 19, 2015, 3:12 a.m. UTC 
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Currently whenever a packet different from ETH_P_IP is sent through the
VRF device it is leaked so plug the leaks and properly drop these
packets.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 drivers/net/vrf.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

David Ahern Aug. 19, 2015, 5:13 p.m. UTC | #1 
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

--
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
Nikolay Aleksandrov Aug. 19, 2015, 8:17 p.m. UTC | #2 
> 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.


--
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
David Ahern Aug. 19, 2015, 9 p.m. UTC | #3 
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>


--
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
David Miller Aug. 20, 2015, 7:59 p.m. UTC | #4 
From: Nikolay Aleksandrov <razor@blackwall.org>
Date: Wed, 19 Aug 2015 06:12:29 +0300

> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> 
> Currently whenever a packet different from ETH_P_IP is sent through the
> VRF device it is leaked so plug the leaks and properly drop these
> packets.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Applied, thanks.
--
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 mbox

Patch

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;
 	}
 }