On 8/23/22 17:12, Andrey Zhadchenko via dev wrote:
> ovs_dp_cmd_new()->ovs_dp_change()->ovs_dp_set_upcall_portids()
> allocates array via kmalloc.
> If for some reason new_vport() fails during ovs_dp_cmd_new()
> dp->upcall_portids must be freed.
> Add missing kfree.
>
> Kmemleak example:
> unreferenced object 0xffff88800c382500 (size 64):
> comm "dump_state", pid 323, jiffies 4294955418 (age 104.347s)
> hex dump (first 32 bytes):
> 5e c2 79 e4 1f 7a 38 c7 09 21 38 0c 80 88 ff ff ^.y..z8..!8.....
> 03 00 00 00 0a 00 00 00 14 00 00 00 28 00 00 00 ............(...
> backtrace:
> [<0000000071bebc9f>] ovs_dp_set_upcall_portids+0x38/0xa0
> [<000000000187d8bd>] ovs_dp_change+0x63/0xe0
> [<000000002397e446>] ovs_dp_cmd_new+0x1f0/0x380
> [<00000000aa06f36e>] genl_family_rcv_msg_doit+0xea/0x150
> [<000000008f583bc4>] genl_rcv_msg+0xdc/0x1e0
> [<00000000fa10e377>] netlink_rcv_skb+0x50/0x100
> [<000000004959cece>] genl_rcv+0x24/0x40
> [<000000004699ac7f>] netlink_unicast+0x23e/0x360
> [<00000000c153573e>] netlink_sendmsg+0x24e/0x4b0
> [<000000006f4aa380>] sock_sendmsg+0x62/0x70
> [<00000000d0068654>] ____sys_sendmsg+0x230/0x270
> [<0000000012dacf7d>] ___sys_sendmsg+0x88/0xd0
> [<0000000011776020>] __sys_sendmsg+0x59/0xa0
> [<000000002e8f2dc1>] do_syscall_64+0x3b/0x90
> [<000000003243e7cb>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Fixes: b83d23a2a38b ("openvswitch: Introduce per-cpu upcall dispatch")
> Acked-by: Aaron Conole <[email protected]>
> Signed-off-by: Andrey Zhadchenko <[email protected]>
> ---
> net/openvswitch/datapath.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 7e8a39a35627..3eb1dc435276 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1802,7 +1802,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
> ovs_dp_reset_user_features(skb, info);
> }
>
> - goto err_unlock_and_destroy_meters;
> + goto err_destroy_pids;
> }
>
> err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
> @@ -1817,6 +1817,9 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
> ovs_notify(&dp_datapath_genl_family, reply, info);
> return 0;
>
> +err_destroy_pids:
> + if (rcu_dereference_raw(dp->upcall_portids))
> + kfree(rcu_dereference_raw(dp->upcall_portids));
kfree() as all typical implementations of free()-like functions
should be perfectly fine to call with a NULL argument.
Also, dereferencing RCU pointer twice is a bad pattern. I know,
it's not a real problem here, since no other threads seen that
pointer yet. But it's probably better to avoid bad patterns
anyway, as someone may copy it in the future to other place where
it will be a problem.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
