Date:   Thu, 9 Feb 2023 11:58:38 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Hangyu Hua <hbh25y@gmail.com>
Cc:     pshelar@ovn.org, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, xiangxia.m.yue@gmail.com,
        echaudro@redhat.com, dev@openvswitch.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [ovs-dev] [PATCH v2] net: openvswitch: fix possible memory leak
 in ovs_meter_cmd_set()
Message-ID: <Y+TR3sB4X5Yt79Tx@corigine.com>
References: <20230209093240.14685-1-hbh25y@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230209093240.14685-1-hbh25y@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?z+sha/Uo3s64Kxk8fGXyvZ3L8nO3PEsgfGORHX9Chd/jDPDso3s6W+cpab6P?=
 =?us-ascii?Q?Vf33PQfHC9Q5Y3GM50rFiHQQ7LXDicV0aNJaVMGqZSpYabaFjmMbc6IZHyFF?=
 =?us-ascii?Q?0ez/2iMFKhgb+TiREfkizBbPYlOlYdwswU4ikS4UD6c1NYSO9zJyr5779RGH?=
 =?us-ascii?Q?1kuT+OtGIuHhWGD9tkFpmNTaDPaISs5G6QJHz6XMMzU7q1I+QAUH5E8yf28/?=
 =?us-ascii?Q?q4ms3HyqnX7LSHMsykXRgin0o5eBHMS+tzlwdVtkL/kU1N1lr9MjUBXFvpaT?=
 =?us-ascii?Q?DyBvrNfPbvXaR+3gNSGMSJROi9zIri/K189C7VWx4CFQp03GImCjH4O3TPFv?=
 =?us-ascii?Q?osf/LQkqBm+jBnfLaN4QY/+oWf/0cQsAjeOCXoAx47XyK1p6ru/eB3m6k4AG?=
 =?us-ascii?Q?5ykd/F/+lOFLydcXSxX9MoN8WNDtUYtDOC/YxXHlXyPnqhLlkvQWIvL2sI3n?=
 =?us-ascii?Q?HgD0+DMd3h5i/+oGYIcmlbqQwI3kViYobtQ/yQe9hmNypG2D5/ew2Igqt3Y6?=
 =?us-ascii?Q?T4RoF2t59YWQpvxA+CGveBwXKvoI1EuKrHF0zGG70q6gzKIPOTLws5i5IaU1?=
 =?us-ascii?Q?WeZftbWvY3P6ZT5V5gtwpZNFvomClk7fX/XfKDY/CflFxBiOzGcL2XlNls1c?=
 =?us-ascii?Q?KLMXA9O4JbzerUl9tL9BeUUhx9djI8BEdK9i5RXVVgYq4lZUmT0EXRgQWFqJ?=
 =?us-ascii?Q?OXWwdcSpPkEjwUhn6TrzZ53HKmldMaFybEF/XsTuIA558QUU5vs20HZs4nOJ?=
 =?us-ascii?Q?92sgFcBECURvpkOEKVBwR24pbQfnExECjWVnYVkEA7DW+lXJlr0EMPc2Zr2p?=
 =?us-ascii?Q?5EI6p9ZA50YIiEHYe2eMWqmS7B5G9F4AwS/xo6APdpRVIUe8VWdBvK8Bgx5F?=
 =?us-ascii?Q?O7Zh3sWE3FOyXucSiq0zB2eO3N3Do6lEJXsiaz7BvQ0tKJF9lolPvaBhcRov?=
 =?us-ascii?Q?Ug2HVRTTzVp+ZQbPgk/UoyJzc5e+Rg2jzvZURnaw/gK3t0aQw4upW1hDGDfN?=
 =?us-ascii?Q?ZIBy6pwucqohW0ztxlkiEV/b8Z1InpsTviedTmTfvn/pc88cUGiydXdBCV0P?=
 =?us-ascii?Q?G5pqArXoCKdiTvXFQAAJJX9hSnqoXnHyIvHLvvAZO+gttTk4h5gCwotWotjL?=
 =?us-ascii?Q?xI0VGMFr/mKPT+FSLRMSGSiJDY1qH8sHdPZn9/Yx+s4Q9GioKh5klxDnbYcf?=
 =?us-ascii?Q?Hh9Wh3M9pPJFkvkiGIsk5VEJTAfHal653LNKAW6s1JLLbc9sYxKDWVOLPdqU?=
 =?us-ascii?Q?WkrscE5sN6a7cxs8+yD2OMiPlekSpOCJJ/Pu+ae0yVz6F3oIvt8TH3FEyo8P?=
 =?us-ascii?Q?XdTpUm1UfOVVH7tgUdh0xVCe+1w0UYMgIDHvkb4uZuoGjz66/TduElEDVW4i?=
 =?us-ascii?Q?aucVWHk1uM94sxbAyjbJ4Kg0hfiEovVSGGy2enDpaq7fHpz8R5ecZ8PW8Yzz?=
 =?us-ascii?Q?fZ+Gy/QSvUkI3UNEGgyjVHLjJFff7gISUl1IN/gqLhvQgdh6TfnDwQXbVp1x?=
 =?us-ascii?Q?UbD3CoQJraOq31E03zmGmAhO6yeo3kuV2aX8xB6+vyp2P/6x3ZJbMLma7GLe?=
 =?us-ascii?Q?2KJeW6LOG/GYiQmAPLP2r2xu5C3Q/tCGAJ8VB9SShu22asCHB50Ffv3VZyeo?=
 =?us-ascii?Q?rqrdKVYIIQ6DtcuSnxVT0yqgwsGNPoeIvY1cw2upsB9l6LXGCJpbO0qGZztZ?=
 =?us-ascii?Q?R3UAng=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5f246024-18b4-4f6f-8779-08db0a8c9ce4
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2023 10:58:44.9009
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: tNKntn4w/ZqVmiW6EnIYUs7+vrhMJxOPkhAKzYQxwfQn/xfLrOaV/rHC0cpBDaQw5sSr52+q87z5YI1PCdvzKtsKYc2tproSElyGO38tk4M=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR13MB4764
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Feb 09, 2023 at 05:32:40PM +0800, Hangyu Hua wrote:
> old_meter needs to be free after it is detached regardless of whether
> the new meter is successfully attached.
> 
> Fixes: c7c4c44c9a95 ("net: openvswitch: expand the meters supported number")
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
> 
> v2: use goto label and free old_meter outside of ovs lock.
> 
>  net/openvswitch/meter.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c
> index 6e38f68f88c2..9b680f0894f1 100644
> --- a/net/openvswitch/meter.c
> +++ b/net/openvswitch/meter.c
> @@ -417,6 +417,7 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info)
>  	int err;
>  	u32 meter_id;
>  	bool failed;
> +	bool locked = true;
>  
>  	if (!a[OVS_METER_ATTR_ID])
>  		return -EINVAL;
> @@ -448,11 +449,13 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info)
>  		goto exit_unlock;
>  
>  	err = attach_meter(meter_tbl, meter);
> -	if (err)
> -		goto exit_unlock;
>  
>  	ovs_unlock();
>  
> +	if (err) {
> +		locked = false;
> +		goto exit_free_old_meter;
> +	}
>  	/* Build response with the meter_id and stats from
>  	 * the old meter, if any.
>  	 */
> @@ -472,8 +475,11 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info)
>  	genlmsg_end(reply, ovs_reply_header);
>  	return genlmsg_reply(reply, info);
>  
> +exit_free_old_meter:
> +	ovs_meter_free(old_meter);
>  exit_unlock:
> -	ovs_unlock();
> +	if (locked)
> +		ovs_unlock();

I see where you are going here, but is the complexity of the
locked variable worth the benefit of freeing old_meter outside
the lock?

>  	nlmsg_free(reply);
>  exit_free_meter:
>  	kfree(meter);
> -- 
> 2.34.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>