Date:   Wed, 1 Feb 2023 17:54:28 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Fedor Pchelkin <pchelkin@ispras.ru>
Cc:     Pravin B Shelar <pshelar@ovn.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Eelco Chaudron <echaudro@redhat.com>, netdev@vger.kernel.org,
        dev@openvswitch.org, linux-kernel@vger.kernel.org,
        Alexey Khoroshilov <khoroshilov@ispras.ru>,
        lvc-project@linuxtesting.org
Subject: Re: [PATCH] net: openvswitch: fix flow memory leak in
 ovs_flow_cmd_new
Message-ID: <Y9qZRCn7CLhYr5h3@corigine.com>
References: <20230131191939.901288-1-pchelkin@ispras.ru>
 <Y9qI/vBRPlDFwkAh@corigine.com>
 <a0be13d0-22d5-b92b-9fed-4faeed30fdce@ispras.ru>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a0be13d0-22d5-b92b-9fed-4faeed30fdce@ispras.ru>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?3VTd7WGPyLFW9CU8g7XThHmNHgEC14mgZsyD3Ug50IMTiQAERoynhYmb6Hrp?=
 =?us-ascii?Q?jzYLMXGBDK7qt4Kf6UO84vfjIs52YMCC5yjpyGUV0O8QmujC6YwszVDhcK0K?=
 =?us-ascii?Q?OpWIZjN6omKJpxBVtudp1VgLGrS032XYXeFE3pKTgKkCAs2EEiXiwzrPTDtd?=
 =?us-ascii?Q?jEgIFCAOLBZwmO+vjnvKh+oXmxcgjmcBNmPTz4Xj8wt5T1O8hJrIYwRn3YM5?=
 =?us-ascii?Q?8wS19bGctPWJvcZvXjbmaJtkNRHTi49GFZybcjN+GHmLPCdVK7EXCgSBo92b?=
 =?us-ascii?Q?wtL7S8PDtFXtQg4uhzDklAR7nWNvNETTpzkTU3TQFvMyCMqJA7e8Np0ZMjEu?=
 =?us-ascii?Q?PWOCWfaKOgQLTw7gCw9RJzw6QtrCYIMVHnIUOcJLbLmXRHJiieUd0YeO+S1J?=
 =?us-ascii?Q?9j1XkXvipG8i7ern237ArZ1BQ0LIOELBLj//eCkjMw2d68IqvwsxZm4t4jxT?=
 =?us-ascii?Q?2x2HFAl3H3A6tiaLIMK0Ida792li3R2lLBZmQu6BqiNMznx50393XGnhnFGU?=
 =?us-ascii?Q?Hf/IXJX6aRLmOT1NtnkHLSOTQw5szu/jKwkJiIiIK0i0S8AvfL5DIl+lNMxf?=
 =?us-ascii?Q?7J2lX9x7UJn2otZ7NaO82tGGgHfajyhWjZopDIn3Z3ShBVtreoRTH4wzrrgZ?=
 =?us-ascii?Q?DZ0xZRPxAaK9OZYImx0m8dbsJiXW1oIIcJGXtXcgJrg6Qgja6SLR9ve4lpj6?=
 =?us-ascii?Q?5V0DMhBss7nhg07fdLC/JBJm4dFR9mDFM8z4Cc4/5Hepj3/D4viKl+zo5nQ4?=
 =?us-ascii?Q?ZeTa9IopIcTNPJJL8JpiECRsEZ0tCIGvQII2xC8i7fyyWBtIt+G/xYOlAhCk?=
 =?us-ascii?Q?aLLUUMJg2Z6SYuRoQ06Cax9pWazX+6IYlxZiI4t8J/fXTfC0O9dYyGBHQMud?=
 =?us-ascii?Q?OiQ5lhPGzCSUx0LgSmjNAVuGdeBkFPedtheJLiRZx9oWBHy/JotoXlIBw6Of?=
 =?us-ascii?Q?t8paOMPAd1I/9bX+thkGGqzhYyc9t7QkZ7Kwi5CVd8DV+th8XJRcv1HZwVYX?=
 =?us-ascii?Q?gWlD9AIbRUIMHwbnosA8ptbxOm8Jmj4IQdEMvk/LbaaFJ6R8RbLNjDvbR0T5?=
 =?us-ascii?Q?KxHyJ8HwSl/PZmBONIEVxVgqPlIRO6r4QwP5q8vFVf/FXvqf5Plh95Ak00vl?=
 =?us-ascii?Q?VhJOtC+ir05VY5OSb1/qCU2r8KRgLaIx38ER7nD17zphB1yK1lfGJwemq15J?=
 =?us-ascii?Q?Y+xpYxU+cqE2akN5UlfXnwG6XijLdG9d+83ZeQQkH/na858/FHvZ/ELo9G96?=
 =?us-ascii?Q?x8F8KoIwX+CUzWAn3ixaqgSQ8YIdQZeyLvzLhAcl8CpYc/j+rFAw+VUljIIv?=
 =?us-ascii?Q?mLHU2HY6pXp3on5/XpHFlVgUmX1mCVl19vU+/ZsxmAA/5YX1t50cxC8XU756?=
 =?us-ascii?Q?p6L6mMq4TDFjFweNkqep0VWFW3bns1gU0h2uzj20Bgj0pdZFvJAu6msrDIU+?=
 =?us-ascii?Q?vCZq/OAjzUCoJw0VaUGJ9Bq3lskEIw0BhRCQvj023Wn8xfj9Wjr6NtBjrGOo?=
 =?us-ascii?Q?kz7T2afippEMzpay88ApKenejdXasAMzTdEoWvAr6hmZ6x7LkO5BsLC/hhA0?=
 =?us-ascii?Q?1v2yt4QDT6jW6Zk2+L4j0xmZrs8Y2wd03Ych7CuN4CQYUu9aMlvzRFir0ZBu?=
 =?us-ascii?Q?As5iVBnKVvxcheplP3hFjuAFnpj5w2S1rkHMCz0OjxYmkdNvaNKeuXWjzmfv?=
 =?us-ascii?Q?YFWBvw=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fbf79719-ec3b-4ab9-41b1-08db0474fff1
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2023 16:54:36.3056
 (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: vU/CbsRrdAZ8YVwWIaYT8e2/SMN6tBFDzStzadnu+20w1Vovf64N8qMfmHAVmtaQo84dqTQCipCJye00l0gDiGpP+014wz8oBKM/9qDGU0s=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO3PR13MB5719
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.