Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Sergei Shtylyov]

Hello.

On 09/30/2015 01:39 AM, Joe Stringer wrote:


If ovs_fragment() was unable to fragment the skb due to an L2 header
that exceeds the supported length, skbs would be leaked. Fix the bug.

Fixes: 7f8a436 "openvswitch: Add conntrack action"
Signed-off-by: Joe Stringer 
---
  net/openvswitch/actions.c | 13 +
  1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index e23a61c..e1afbd1 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c

[...]

@@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct 
sk_buff *skb, u16 mru,
WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
  ovs_vport_name(vport), ntohs(ethertype), mru,
  vport->dev->mtu);
-   kfree_skb(skb);
+   goto out;
}
+
+   skb = NULL;


   I'd just return here.


+
+out:
+   if (skb)
+   kfree_skb(skb);


   kfree_skb() checks for NULL.

[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Sergei Shtylyov]

Hello.

On 09/30/2015 01:39 AM, Joe Stringer wrote:


If ovs_fragment() was unable to fragment the skb due to an L2 header
that exceeds the supported length, skbs would be leaked. Fix the bug.

Fixes: 7f8a436 "openvswitch: Add conntrack action"
Signed-off-by: Joe Stringer 
---
  net/openvswitch/actions.c | 13 +
  1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index e23a61c..e1afbd1 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c

[...]

@@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct 
sk_buff *skb, u16 mru,
WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
  ovs_vport_name(vport), ntohs(ethertype), mru,
  vport->dev->mtu);
-   kfree_skb(skb);
+   goto out;
}
+
+   skb = NULL;


   I'd just return here.


+
+out:
+   if (skb)
+   kfree_skb(skb);


   kfree_skb() checks for NULL.

[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Joe Stringer]

On 29 September 2015 at 15:48, Rustad, Mark D  wrote:
>> On Sep 29, 2015, at 3:39 PM, Joe Stringer  wrote:
>>
>> @@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct 
>> sk_buff *skb, u16 mru,
>>   WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
>> ovs_vport_name(vport), ntohs(ethertype), mru,
>> vport->dev->mtu);
>> - kfree_skb(skb);
>> + goto out;
>>   }
>> +
>> + skb = NULL;
>> +
>> +out:
>> + if (skb)
>> + kfree_skb(skb);
>> }
>>
>> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
>
> Wouldn't that hunk be better as:
>
> @@ -728,8 +727,13 @@ static void ovs_fragment(struct vport *vport, struct 
> sk_buff *skb, u16 mru,
> WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, 
> MTU=%d.",
>   ovs_vport_name(vport), ntohs(ethertype), mru,
>   vport->dev->mtu);
> -   kfree_skb(skb);
> +   goto out;
> }
> +
> +   return;
> +
> +out:
> +   kfree_skb(skb);
> }
>
> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
>
> --
> Mark Rustad, Networking Division, Intel Corporation

Sure thing, I'll roll this change in to a v2 when the rest of the
series is reviewed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Rustad, Mark D]

> On Sep 29, 2015, at 3:39 PM, Joe Stringer  wrote:
> 
> @@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct 
> sk_buff *skb, u16 mru,
>   WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
> ovs_vport_name(vport), ntohs(ethertype), mru,
> vport->dev->mtu);
> - kfree_skb(skb);
> + goto out;
>   }
> +
> + skb = NULL;
> +
> +out:
> + if (skb)
> + kfree_skb(skb);
> }
> 
> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,

Wouldn't that hunk be better as:

@@ -728,8 +727,13 @@ static void ovs_fragment(struct vport *vport, struct 
sk_buff *skb, u16 mru,
WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
  ovs_vport_name(vport), ntohs(ethertype), mru,
  vport->dev->mtu);
-   kfree_skb(skb);
+   goto out;
}
+
+   return;
+
+out:
+   kfree_skb(skb);
}

static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Rustad, Mark D]

> On Sep 29, 2015, at 3:39 PM, Joe Stringer  wrote:
> 
> @@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct 
> sk_buff *skb, u16 mru,
>   WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
> ovs_vport_name(vport), ntohs(ethertype), mru,
> vport->dev->mtu);
> - kfree_skb(skb);
> + goto out;
>   }
> +
> + skb = NULL;
> +
> +out:
> + if (skb)
> + kfree_skb(skb);
> }
> 
> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,

Wouldn't that hunk be better as:

@@ -728,8 +727,13 @@ static void ovs_fragment(struct vport *vport, struct 
sk_buff *skb, u16 mru,
WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
  ovs_vport_name(vport), ntohs(ethertype), mru,
  vport->dev->mtu);
-   kfree_skb(skb);
+   goto out;
}
+
+   return;
+
+out:
+   kfree_skb(skb);
}

static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Joe Stringer]

On 29 September 2015 at 15:48, Rustad, Mark D  wrote:
>> On Sep 29, 2015, at 3:39 PM, Joe Stringer  wrote:
>>
>> @@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct 
>> sk_buff *skb, u16 mru,
>>   WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
>> ovs_vport_name(vport), ntohs(ethertype), mru,
>> vport->dev->mtu);
>> - kfree_skb(skb);
>> + goto out;
>>   }
>> +
>> + skb = NULL;
>> +
>> +out:
>> + if (skb)
>> + kfree_skb(skb);
>> }
>>
>> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
>
> Wouldn't that hunk be better as:
>
> @@ -728,8 +727,13 @@ static void ovs_fragment(struct vport *vport, struct 
> sk_buff *skb, u16 mru,
> WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, 
> MTU=%d.",
>   ovs_vport_name(vport), ntohs(ethertype), mru,
>   vport->dev->mtu);
> -   kfree_skb(skb);
> +   goto out;
> }
> +
> +   return;
> +
> +out:
> +   kfree_skb(skb);
> }
>
> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
>
> --
> Mark Rustad, Networking Division, Intel Corporation

Sure thing, I'll roll this change in to a v2 when the rest of the
series is reviewed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/