* [PATCH] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new
@ 2023-01-31 19:19 Fedor Pchelkin
2023-02-01 15:45 ` Simon Horman
0 siblings, 1 reply; 8+ messages in thread
From: Fedor Pchelkin @ 2023-01-31 19:19 UTC (permalink / raw)
To: Pravin B Shelar
Cc: Fedor Pchelkin, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Eelco Chaudron, netdev, dev, linux-kernel,
Alexey Khoroshilov, lvc-project
Syzkaller reports a memory leak of new_flow in ovs_flow_cmd_new() as it is
not freed when an allocation of a key fails.
BUG: memory leak
unreferenced object 0xffff888116668000 (size 632):
comm "syz-executor231", pid 1090, jiffies 4294844701 (age 18.871s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<00000000defa3494>] kmem_cache_zalloc include/linux/slab.h:654 [inline]
[<00000000defa3494>] ovs_flow_alloc+0x19/0x180 net/openvswitch/flow_table.c:77
[<00000000c67d8873>] ovs_flow_cmd_new+0x1de/0xd40 net/openvswitch/datapath.c:957
[<0000000010a539a8>] genl_family_rcv_msg_doit+0x22d/0x330 net/netlink/genetlink.c:739
[<00000000dff3302d>] genl_family_rcv_msg net/netlink/genetlink.c:783 [inline]
[<00000000dff3302d>] genl_rcv_msg+0x328/0x590 net/netlink/genetlink.c:800
[<000000000286dd87>] netlink_rcv_skb+0x153/0x430 net/netlink/af_netlink.c:2515
[<0000000061fed410>] genl_rcv+0x24/0x40 net/netlink/genetlink.c:811
[<000000009dc0f111>] netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
[<000000009dc0f111>] netlink_unicast+0x545/0x7f0 net/netlink/af_netlink.c:1339
[<000000004a5ee816>] netlink_sendmsg+0x8e7/0xde0 net/netlink/af_netlink.c:1934
[<00000000482b476f>] sock_sendmsg_nosec net/socket.c:651 [inline]
[<00000000482b476f>] sock_sendmsg+0x152/0x190 net/socket.c:671
[<00000000698574ba>] ____sys_sendmsg+0x70a/0x870 net/socket.c:2356
[<00000000d28d9e11>] ___sys_sendmsg+0xf3/0x170 net/socket.c:2410
[<0000000083ba9120>] __sys_sendmsg+0xe5/0x1b0 net/socket.c:2439
[<00000000c00628f8>] do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46
[<000000004abfdcf4>] entry_SYSCALL_64_after_hwframe+0x61/0xc6
To fix this the patch removes unnecessary err_kfree_key label and adds a
proper goto statement on the key-allocation-error path.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 68bb10101e6b ("openvswitch: Fix flow lookup to use unmasked key")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
net/openvswitch/datapath.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index a71795355aec..3d4b5d83d306 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1004,7 +1004,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
key = kzalloc(sizeof(*key), GFP_KERNEL);
if (!key) {
error = -ENOMEM;
- goto err_kfree_key;
+ goto err_kfree_flow;
}
ovs_match_init(&match, key, false, &mask);
@@ -1128,7 +1128,6 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
ovs_nla_free_flow_actions(acts);
err_kfree_flow:
ovs_flow_free(new_flow, false);
-err_kfree_key:
kfree(key);
error:
return error;
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new
2023-01-31 19:19 [PATCH] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new Fedor Pchelkin
@ 2023-02-01 15:45 ` Simon Horman
2023-02-01 16:28 ` Fedor Pchelkin
0 siblings, 1 reply; 8+ messages in thread
From: Simon Horman @ 2023-02-01 15:45 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: Pravin B Shelar, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Eelco Chaudron, netdev, dev, linux-kernel,
Alexey Khoroshilov, lvc-project
On Tue, Jan 31, 2023 at 10:19:39PM +0300, Fedor Pchelkin wrote:
> Syzkaller reports a memory leak of new_flow in ovs_flow_cmd_new() as it is
> not freed when an allocation of a key fails.
>
> BUG: memory leak
> unreferenced object 0xffff888116668000 (size 632):
> comm "syz-executor231", pid 1090, jiffies 4294844701 (age 18.871s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<00000000defa3494>] kmem_cache_zalloc include/linux/slab.h:654 [inline]
> [<00000000defa3494>] ovs_flow_alloc+0x19/0x180 net/openvswitch/flow_table.c:77
> [<00000000c67d8873>] ovs_flow_cmd_new+0x1de/0xd40 net/openvswitch/datapath.c:957
> [<0000000010a539a8>] genl_family_rcv_msg_doit+0x22d/0x330 net/netlink/genetlink.c:739
> [<00000000dff3302d>] genl_family_rcv_msg net/netlink/genetlink.c:783 [inline]
> [<00000000dff3302d>] genl_rcv_msg+0x328/0x590 net/netlink/genetlink.c:800
> [<000000000286dd87>] netlink_rcv_skb+0x153/0x430 net/netlink/af_netlink.c:2515
> [<0000000061fed410>] genl_rcv+0x24/0x40 net/netlink/genetlink.c:811
> [<000000009dc0f111>] netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
> [<000000009dc0f111>] netlink_unicast+0x545/0x7f0 net/netlink/af_netlink.c:1339
> [<000000004a5ee816>] netlink_sendmsg+0x8e7/0xde0 net/netlink/af_netlink.c:1934
> [<00000000482b476f>] sock_sendmsg_nosec net/socket.c:651 [inline]
> [<00000000482b476f>] sock_sendmsg+0x152/0x190 net/socket.c:671
> [<00000000698574ba>] ____sys_sendmsg+0x70a/0x870 net/socket.c:2356
> [<00000000d28d9e11>] ___sys_sendmsg+0xf3/0x170 net/socket.c:2410
> [<0000000083ba9120>] __sys_sendmsg+0xe5/0x1b0 net/socket.c:2439
> [<00000000c00628f8>] do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46
> [<000000004abfdcf4>] entry_SYSCALL_64_after_hwframe+0x61/0xc6
>
> To fix this the patch removes unnecessary err_kfree_key label and adds a
> proper goto statement on the key-allocation-error path.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 68bb10101e6b ("openvswitch: Fix flow lookup to use unmasked key")
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
> net/openvswitch/datapath.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index a71795355aec..3d4b5d83d306 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1004,7 +1004,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> key = kzalloc(sizeof(*key), GFP_KERNEL);
> if (!key) {
> error = -ENOMEM;
> - goto err_kfree_key;
> + goto err_kfree_flow;
> }
>
> ovs_match_init(&match, key, false, &mask);
> @@ -1128,7 +1128,6 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> ovs_nla_free_flow_actions(acts);
> err_kfree_flow:
> ovs_flow_free(new_flow, false);
> -err_kfree_key:
> kfree(key);
> error:
> return error;
I see this would work by virtue of kfree(key) doing nothing
of key is NULL, the error case in question. And that otherwise key is
non-NULL if this path is hit.
However, the idiomatic approach to error handling is for the error path
to unwind resource allocations in the reverse order that they were made.
And for goto labels to control how far to unwind.
So I think the following would be more in keeping with the intention of the
code. Even if it is a somewhat more verbose change.
*compile tested only!*
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index a71795355aec..fcee6012293b 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1004,14 +1004,14 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
key = kzalloc(sizeof(*key), GFP_KERNEL);
if (!key) {
error = -ENOMEM;
- goto err_kfree_key;
+ goto err_kfree_flow;
}
ovs_match_init(&match, key, false, &mask);
error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
a[OVS_FLOW_ATTR_MASK], log);
if (error)
- goto err_kfree_flow;
+ goto err_kfree_key;
ovs_flow_mask_key(&new_flow->key, key, true, &mask);
@@ -1019,14 +1019,14 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
error = ovs_nla_get_identifier(&new_flow->id, a[OVS_FLOW_ATTR_UFID],
key, log);
if (error)
- goto err_kfree_flow;
+ goto err_kfree_key;
/* Validate actions. */
error = ovs_nla_copy_actions(net, a[OVS_FLOW_ATTR_ACTIONS],
&new_flow->key, &acts, log);
if (error) {
OVS_NLERR(log, "Flow actions may not be safe on all matching packets.");
- goto err_kfree_flow;
+ goto err_kfree_key;
}
reply = ovs_flow_cmd_alloc_info(acts, &new_flow->id, info, false,
@@ -1126,10 +1126,10 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
kfree_skb(reply);
err_kfree_acts:
ovs_nla_free_flow_actions(acts);
-err_kfree_flow:
- ovs_flow_free(new_flow, false);
err_kfree_key:
kfree(key);
+err_kfree_flow:
+ ovs_flow_free(new_flow, false);
error:
return error;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new
2023-02-01 15:45 ` Simon Horman
@ 2023-02-01 16:28 ` Fedor Pchelkin
2023-02-01 16:54 ` Simon Horman
0 siblings, 1 reply; 8+ messages in thread
From: Fedor Pchelkin @ 2023-02-01 16:28 UTC (permalink / raw)
To: Simon Horman
Cc: Pravin B Shelar, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Eelco Chaudron, netdev, dev, linux-kernel,
Alexey Khoroshilov, lvc-project
On 2/1/23 6:45 PM, Simon Horman wrote:
> I see this would work by virtue of kfree(key) doing nothing
> of key is NULL, the error case in question. And that otherwise key is
> non-NULL if this path is hit.
>
> However, the idiomatic approach to error handling is for the error path
> to unwind resource allocations in the reverse order that they were made.
> And for goto labels to control how far to unwind.
>
You are right, thanks. Have to keep 'goto' structured, otherwise there
would be a 'goto' mess.
> So I think the following would be more in keeping with the intention of the
> code. Even if it is a somewhat more verbose change.
>
> *compile tested only!*
I'll test this on error paths and resend the patch.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new
2023-02-01 16:28 ` Fedor Pchelkin
@ 2023-02-01 16:54 ` Simon Horman
2023-02-01 21:02 ` [PATCH v2] " Fedor Pchelkin
0 siblings, 1 reply; 8+ messages in thread
From: Simon Horman @ 2023-02-01 16:54 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: Pravin B Shelar, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Eelco Chaudron, netdev, dev, linux-kernel,
Alexey Khoroshilov, lvc-project
On Wed, Feb 01, 2023 at 07:28:09PM +0300, Fedor Pchelkin wrote:
> On 2/1/23 6:45 PM, Simon Horman wrote:
> > I see this would work by virtue of kfree(key) doing nothing
> > of key is NULL, the error case in question. And that otherwise key is
> > non-NULL if this path is hit.
> >
> > However, the idiomatic approach to error handling is for the error path
> > to unwind resource allocations in the reverse order that they were made.
> > And for goto labels to control how far to unwind.
> >
>
> You are right, thanks. Have to keep 'goto' structured, otherwise there
> would be a 'goto' mess.
>
> > So I think the following would be more in keeping with the intention of the
> > code. Even if it is a somewhat more verbose change.
> >
> > *compile tested only!*
>
> I'll test this on error paths and resend the patch.
Thanks, much appreciated.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new
2023-02-01 16:54 ` Simon Horman
@ 2023-02-01 21:02 ` Fedor Pchelkin
2023-02-02 8:11 ` Eelco Chaudron
2023-02-02 19:40 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 8+ messages in thread
From: Fedor Pchelkin @ 2023-02-01 21:02 UTC (permalink / raw)
To: Pravin B Shelar, Simon Horman
Cc: Fedor Pchelkin, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Eelco Chaudron, netdev, dev, linux-kernel,
Alexey Khoroshilov, lvc-project
Syzkaller reports a memory leak of new_flow in ovs_flow_cmd_new() as it is
not freed when an allocation of a key fails.
BUG: memory leak
unreferenced object 0xffff888116668000 (size 632):
comm "syz-executor231", pid 1090, jiffies 4294844701 (age 18.871s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<00000000defa3494>] kmem_cache_zalloc include/linux/slab.h:654 [inline]
[<00000000defa3494>] ovs_flow_alloc+0x19/0x180 net/openvswitch/flow_table.c:77
[<00000000c67d8873>] ovs_flow_cmd_new+0x1de/0xd40 net/openvswitch/datapath.c:957
[<0000000010a539a8>] genl_family_rcv_msg_doit+0x22d/0x330 net/netlink/genetlink.c:739
[<00000000dff3302d>] genl_family_rcv_msg net/netlink/genetlink.c:783 [inline]
[<00000000dff3302d>] genl_rcv_msg+0x328/0x590 net/netlink/genetlink.c:800
[<000000000286dd87>] netlink_rcv_skb+0x153/0x430 net/netlink/af_netlink.c:2515
[<0000000061fed410>] genl_rcv+0x24/0x40 net/netlink/genetlink.c:811
[<000000009dc0f111>] netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
[<000000009dc0f111>] netlink_unicast+0x545/0x7f0 net/netlink/af_netlink.c:1339
[<000000004a5ee816>] netlink_sendmsg+0x8e7/0xde0 net/netlink/af_netlink.c:1934
[<00000000482b476f>] sock_sendmsg_nosec net/socket.c:651 [inline]
[<00000000482b476f>] sock_sendmsg+0x152/0x190 net/socket.c:671
[<00000000698574ba>] ____sys_sendmsg+0x70a/0x870 net/socket.c:2356
[<00000000d28d9e11>] ___sys_sendmsg+0xf3/0x170 net/socket.c:2410
[<0000000083ba9120>] __sys_sendmsg+0xe5/0x1b0 net/socket.c:2439
[<00000000c00628f8>] do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46
[<000000004abfdcf4>] entry_SYSCALL_64_after_hwframe+0x61/0xc6
To fix this the patch rearranges the goto labels to reflect the order of
object allocations and adds appropriate goto statements on the error
paths.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 68bb10101e6b ("openvswitch: Fix flow lookup to use unmasked key")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
v1->v2: make goto statements structured
net/openvswitch/datapath.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index a71795355aec..fcee6012293b 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1004,14 +1004,14 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
key = kzalloc(sizeof(*key), GFP_KERNEL);
if (!key) {
error = -ENOMEM;
- goto err_kfree_key;
+ goto err_kfree_flow;
}
ovs_match_init(&match, key, false, &mask);
error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
a[OVS_FLOW_ATTR_MASK], log);
if (error)
- goto err_kfree_flow;
+ goto err_kfree_key;
ovs_flow_mask_key(&new_flow->key, key, true, &mask);
@@ -1019,14 +1019,14 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
error = ovs_nla_get_identifier(&new_flow->id, a[OVS_FLOW_ATTR_UFID],
key, log);
if (error)
- goto err_kfree_flow;
+ goto err_kfree_key;
/* Validate actions. */
error = ovs_nla_copy_actions(net, a[OVS_FLOW_ATTR_ACTIONS],
&new_flow->key, &acts, log);
if (error) {
OVS_NLERR(log, "Flow actions may not be safe on all matching packets.");
- goto err_kfree_flow;
+ goto err_kfree_key;
}
reply = ovs_flow_cmd_alloc_info(acts, &new_flow->id, info, false,
@@ -1126,10 +1126,10 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
kfree_skb(reply);
err_kfree_acts:
ovs_nla_free_flow_actions(acts);
-err_kfree_flow:
- ovs_flow_free(new_flow, false);
err_kfree_key:
kfree(key);
+err_kfree_flow:
+ ovs_flow_free(new_flow, false);
error:
return error;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new
2023-02-01 21:02 ` [PATCH v2] " Fedor Pchelkin
@ 2023-02-02 8:11 ` Eelco Chaudron
2023-02-02 9:05 ` Simon Horman
2023-02-02 19:40 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 8+ messages in thread
From: Eelco Chaudron @ 2023-02-02 8:11 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: Pravin B Shelar, Simon Horman, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, dev, linux-kernel,
Alexey Khoroshilov, lvc-project
On 1 Feb 2023, at 22:02, Fedor Pchelkin wrote:
> Syzkaller reports a memory leak of new_flow in ovs_flow_cmd_new() as it is
> not freed when an allocation of a key fails.
>
> BUG: memory leak
> unreferenced object 0xffff888116668000 (size 632):
> comm "syz-executor231", pid 1090, jiffies 4294844701 (age 18.871s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<00000000defa3494>] kmem_cache_zalloc include/linux/slab.h:654 [inline]
> [<00000000defa3494>] ovs_flow_alloc+0x19/0x180 net/openvswitch/flow_table.c:77
> [<00000000c67d8873>] ovs_flow_cmd_new+0x1de/0xd40 net/openvswitch/datapath.c:957
> [<0000000010a539a8>] genl_family_rcv_msg_doit+0x22d/0x330 net/netlink/genetlink.c:739
> [<00000000dff3302d>] genl_family_rcv_msg net/netlink/genetlink.c:783 [inline]
> [<00000000dff3302d>] genl_rcv_msg+0x328/0x590 net/netlink/genetlink.c:800
> [<000000000286dd87>] netlink_rcv_skb+0x153/0x430 net/netlink/af_netlink.c:2515
> [<0000000061fed410>] genl_rcv+0x24/0x40 net/netlink/genetlink.c:811
> [<000000009dc0f111>] netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
> [<000000009dc0f111>] netlink_unicast+0x545/0x7f0 net/netlink/af_netlink.c:1339
> [<000000004a5ee816>] netlink_sendmsg+0x8e7/0xde0 net/netlink/af_netlink.c:1934
> [<00000000482b476f>] sock_sendmsg_nosec net/socket.c:651 [inline]
> [<00000000482b476f>] sock_sendmsg+0x152/0x190 net/socket.c:671
> [<00000000698574ba>] ____sys_sendmsg+0x70a/0x870 net/socket.c:2356
> [<00000000d28d9e11>] ___sys_sendmsg+0xf3/0x170 net/socket.c:2410
> [<0000000083ba9120>] __sys_sendmsg+0xe5/0x1b0 net/socket.c:2439
> [<00000000c00628f8>] do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46
> [<000000004abfdcf4>] entry_SYSCALL_64_after_hwframe+0x61/0xc6
>
> To fix this the patch rearranges the goto labels to reflect the order of
> object allocations and adds appropriate goto statements on the error
> paths.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 68bb10101e6b ("openvswitch: Fix flow lookup to use unmasked key")
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
> v1->v2: make goto statements structured
Thanks for fixing this, the changes look good.
Acked-by: Eelco Chaudron <echaudro@redhat.com>
> net/openvswitch/datapath.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index a71795355aec..fcee6012293b 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1004,14 +1004,14 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> key = kzalloc(sizeof(*key), GFP_KERNEL);
> if (!key) {
> error = -ENOMEM;
> - goto err_kfree_key;
> + goto err_kfree_flow;
> }
>
> ovs_match_init(&match, key, false, &mask);
> error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
> a[OVS_FLOW_ATTR_MASK], log);
> if (error)
> - goto err_kfree_flow;
> + goto err_kfree_key;
>
> ovs_flow_mask_key(&new_flow->key, key, true, &mask);
>
> @@ -1019,14 +1019,14 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> error = ovs_nla_get_identifier(&new_flow->id, a[OVS_FLOW_ATTR_UFID],
> key, log);
> if (error)
> - goto err_kfree_flow;
> + goto err_kfree_key;
>
> /* Validate actions. */
> error = ovs_nla_copy_actions(net, a[OVS_FLOW_ATTR_ACTIONS],
> &new_flow->key, &acts, log);
> if (error) {
> OVS_NLERR(log, "Flow actions may not be safe on all matching packets.");
> - goto err_kfree_flow;
> + goto err_kfree_key;
> }
>
> reply = ovs_flow_cmd_alloc_info(acts, &new_flow->id, info, false,
> @@ -1126,10 +1126,10 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> kfree_skb(reply);
> err_kfree_acts:
> ovs_nla_free_flow_actions(acts);
> -err_kfree_flow:
> - ovs_flow_free(new_flow, false);
> err_kfree_key:
> kfree(key);
> +err_kfree_flow:
> + ovs_flow_free(new_flow, false);
> error:
> return error;
> }
> --
> 2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new
2023-02-02 8:11 ` Eelco Chaudron
@ 2023-02-02 9:05 ` Simon Horman
0 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2023-02-02 9:05 UTC (permalink / raw)
To: Eelco Chaudron
Cc: Fedor Pchelkin, Pravin B Shelar, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, dev, linux-kernel,
Alexey Khoroshilov, lvc-project
On Thu, Feb 02, 2023 at 09:11:04AM +0100, Eelco Chaudron wrote:
>
>
> On 1 Feb 2023, at 22:02, Fedor Pchelkin wrote:
>
> > Syzkaller reports a memory leak of new_flow in ovs_flow_cmd_new() as it is
> > not freed when an allocation of a key fails.
> >
> > BUG: memory leak
> > unreferenced object 0xffff888116668000 (size 632):
> > comm "syz-executor231", pid 1090, jiffies 4294844701 (age 18.871s)
> > hex dump (first 32 bytes):
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > backtrace:
> > [<00000000defa3494>] kmem_cache_zalloc include/linux/slab.h:654 [inline]
> > [<00000000defa3494>] ovs_flow_alloc+0x19/0x180 net/openvswitch/flow_table.c:77
> > [<00000000c67d8873>] ovs_flow_cmd_new+0x1de/0xd40 net/openvswitch/datapath.c:957
> > [<0000000010a539a8>] genl_family_rcv_msg_doit+0x22d/0x330 net/netlink/genetlink.c:739
> > [<00000000dff3302d>] genl_family_rcv_msg net/netlink/genetlink.c:783 [inline]
> > [<00000000dff3302d>] genl_rcv_msg+0x328/0x590 net/netlink/genetlink.c:800
> > [<000000000286dd87>] netlink_rcv_skb+0x153/0x430 net/netlink/af_netlink.c:2515
> > [<0000000061fed410>] genl_rcv+0x24/0x40 net/netlink/genetlink.c:811
> > [<000000009dc0f111>] netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
> > [<000000009dc0f111>] netlink_unicast+0x545/0x7f0 net/netlink/af_netlink.c:1339
> > [<000000004a5ee816>] netlink_sendmsg+0x8e7/0xde0 net/netlink/af_netlink.c:1934
> > [<00000000482b476f>] sock_sendmsg_nosec net/socket.c:651 [inline]
> > [<00000000482b476f>] sock_sendmsg+0x152/0x190 net/socket.c:671
> > [<00000000698574ba>] ____sys_sendmsg+0x70a/0x870 net/socket.c:2356
> > [<00000000d28d9e11>] ___sys_sendmsg+0xf3/0x170 net/socket.c:2410
> > [<0000000083ba9120>] __sys_sendmsg+0xe5/0x1b0 net/socket.c:2439
> > [<00000000c00628f8>] do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46
> > [<000000004abfdcf4>] entry_SYSCALL_64_after_hwframe+0x61/0xc6
> >
> > To fix this the patch rearranges the goto labels to reflect the order of
> > object allocations and adds appropriate goto statements on the error
> > paths.
> >
> > Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> >
> > Fixes: 68bb10101e6b ("openvswitch: Fix flow lookup to use unmasked key")
> > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > ---
> > v1->v2: make goto statements structured
>
> Thanks for fixing this, the changes look good.
>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new
2023-02-01 21:02 ` [PATCH v2] " Fedor Pchelkin
2023-02-02 8:11 ` Eelco Chaudron
@ 2023-02-02 19:40 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-02-02 19:40 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: pshelar, simon.horman, davem, edumazet, kuba, pabeni, echaudro,
netdev, dev, linux-kernel, khoroshilov, lvc-project
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Thu, 2 Feb 2023 00:02:18 +0300 you wrote:
> Syzkaller reports a memory leak of new_flow in ovs_flow_cmd_new() as it is
> not freed when an allocation of a key fails.
>
> BUG: memory leak
> unreferenced object 0xffff888116668000 (size 632):
> comm "syz-executor231", pid 1090, jiffies 4294844701 (age 18.871s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<00000000defa3494>] kmem_cache_zalloc include/linux/slab.h:654 [inline]
> [<00000000defa3494>] ovs_flow_alloc+0x19/0x180 net/openvswitch/flow_table.c:77
> [<00000000c67d8873>] ovs_flow_cmd_new+0x1de/0xd40 net/openvswitch/datapath.c:957
> [<0000000010a539a8>] genl_family_rcv_msg_doit+0x22d/0x330 net/netlink/genetlink.c:739
> [<00000000dff3302d>] genl_family_rcv_msg net/netlink/genetlink.c:783 [inline]
> [<00000000dff3302d>] genl_rcv_msg+0x328/0x590 net/netlink/genetlink.c:800
> [<000000000286dd87>] netlink_rcv_skb+0x153/0x430 net/netlink/af_netlink.c:2515
> [<0000000061fed410>] genl_rcv+0x24/0x40 net/netlink/genetlink.c:811
> [<000000009dc0f111>] netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
> [<000000009dc0f111>] netlink_unicast+0x545/0x7f0 net/netlink/af_netlink.c:1339
> [<000000004a5ee816>] netlink_sendmsg+0x8e7/0xde0 net/netlink/af_netlink.c:1934
> [<00000000482b476f>] sock_sendmsg_nosec net/socket.c:651 [inline]
> [<00000000482b476f>] sock_sendmsg+0x152/0x190 net/socket.c:671
> [<00000000698574ba>] ____sys_sendmsg+0x70a/0x870 net/socket.c:2356
> [<00000000d28d9e11>] ___sys_sendmsg+0xf3/0x170 net/socket.c:2410
> [<0000000083ba9120>] __sys_sendmsg+0xe5/0x1b0 net/socket.c:2439
> [<00000000c00628f8>] do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46
> [<000000004abfdcf4>] entry_SYSCALL_64_after_hwframe+0x61/0xc6
>
> [...]
Here is the summary with links:
- [v2] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new
https://git.kernel.org/netdev/net/c/0c598aed445e
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-02-02 19:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 19:19 [PATCH] net: openvswitch: fix flow memory leak in ovs_flow_cmd_new Fedor Pchelkin
2023-02-01 15:45 ` Simon Horman
2023-02-01 16:28 ` Fedor Pchelkin
2023-02-01 16:54 ` Simon Horman
2023-02-01 21:02 ` [PATCH v2] " Fedor Pchelkin
2023-02-02 8:11 ` Eelco Chaudron
2023-02-02 9:05 ` Simon Horman
2023-02-02 19:40 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.