Date:   Tue, 4 Jan 2022 20:44:34 -0400
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     Li Zhijian <lizhijian@cn.fujitsu.com>
Cc:     zyjzyj2000@gmail.com, linux-rdma@vger.kernel.org,
        linux-kernel@vger.kernel.org, yanjun.zhu@linux.dev,
        rpearsonhpe@gmail.com
Subject: Re: [PATCH v2] RDMA/rxe: Prevent from double freeing rxe_map_set
Message-ID: <20220105004434.GA2712053@nvidia.com>
References: <20211228014406.1033444-1-lizhijian@cn.fujitsu.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211228014406.1033444-1-lizhijian@cn.fujitsu.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8tVWgXEtA+jBYFKeariipyIbQHSWvIvUfGNPoJtymHJeXDBLoyrbApGCbrAx?=
 =?us-ascii?Q?K3ICpWoliy2iA0ucUIIIsIfYIa19vLctg5yuaF7/8SBbhrOCqq9dY6O09TWa?=
 =?us-ascii?Q?935/Q2ZUvqBirxIOuVSCqyGpT0ZuUPdCNNo31Z+j20npvRMUNuiUH8mJotv5?=
 =?us-ascii?Q?yKWBDUKzeKm0Xx7+TxIdVN1ObseaZN1lUEaJMnSoCAVnq2UWTtD847mJliZP?=
 =?us-ascii?Q?SkoWg3HUb2BXCVUni6lELHyMXl0E0alqjQNqGG9dyIC/U9bYPmSu5xIe3hW5?=
 =?us-ascii?Q?GPGGXEdCCD/m7X1S7F6jEve6bJHhPpyJx6SiQw0ipFgFcktYUQjBmINB22Ev?=
 =?us-ascii?Q?IK3OmS9VzcSdHsRwcw4BTeGCwuJJROL7OSiB+1XT3bcQNZQKDkNuvS19lffl?=
 =?us-ascii?Q?zcsY872f98K5KIRZZsCPK4H6Qnf841/kykaA6fEM19NYK3hpyZ0beQK11XX1?=
 =?us-ascii?Q?FhexzyGx39IlARQ1yjwQf5G8CcB1HxKYqLRknAqRMVNLJiQ0vT8t6xxuwIfn?=
 =?us-ascii?Q?atjYz4bvhZJ8iJk6ZJxItu3oORObh0O/eQ4N5hY4C4XTu6d3F2yLA9gLC8IF?=
 =?us-ascii?Q?dxxjbwrA6ra7CPzUmxTMjwhxznGt6N1QZOvX7OxWVbcoB2RlP0/H0XUtcAuI?=
 =?us-ascii?Q?2DH9o/Uh0/wvFvv5Tec2QDyBTKvsllqIH6qTJQAj+VZINHg3dlkxYn0/8rT1?=
 =?us-ascii?Q?diOO2LRQ6C9Sl5q4G5CbOtjBIMeMrtYKaji4UnyeTRNpsyQ0hK17gvRjqo99?=
 =?us-ascii?Q?qj0yLLNvuBu6CAzQFwiU9gLd3396KURaYvekv31xYlme7akGBqSzZcJWfXNt?=
 =?us-ascii?Q?VKK+NNPDkzTSn4guv4p2hhksw9Duh95QGzg1XdSqF0cwYWpG9tlrKnZeJCX1?=
 =?us-ascii?Q?bg7qDsz2uvBYvW9zVQFpoykk0dQho4Z5+bsioFzpclw5yuYYYZ2eBTmHv3yl?=
 =?us-ascii?Q?vxA7bT3CTmHZcQ18ntKNc+6onVltcgTcKQdCq9sJf1K8zWWgczf3A4SI0njG?=
 =?us-ascii?Q?JJnym/ioECtZP3WTsKIyulFOEAGj7SgAc9mxNfIe4w8Xf4g+Cq01OlQyiETb?=
 =?us-ascii?Q?fXfG8duhGin2rCxR7umuyG6saFr81p+XbEn5gAJ9CThHIz9JPMsyuwtResC9?=
 =?us-ascii?Q?ucAIDQp5X4eqF0w4RX1NRizu6zH7FVZRWbVOPxqWdhSp8yuxOeSmRc1TD606?=
 =?us-ascii?Q?OJVxzZVx/RSiuG8tm8S/ptwwWMkHV4ZEPbbttf7m9+7KgyW7JscKnOBJD3CJ?=
 =?us-ascii?Q?vbxJHMrUl0wSD/cMsJ+iUv/0W3aFyD0ozjW3JbwveD5ZZHbzMbaA9E0AVA/z?=
 =?us-ascii?Q?GJQWYM+kUlQIcpq0H/n0L4Z2XhO946db/pa877iG1b+vLGEvxOL9ZZ5zOxJR?=
 =?us-ascii?Q?t5RGnfptpITp7yvnFtN8P9H9Z8IDkYvZwH0SYGeQRgavDLm8Rk4q32IpqWhm?=
 =?us-ascii?Q?SIrtShb85Y7PHamZddpWHCqdp0I6CP84fgaOyeOkxq5/D4j5+h2+c+lwn0af?=
 =?us-ascii?Q?XvlVWptZ76OKN4PTF+ftYhl7ckg4KwxvsxaEpD5+TX88iylNs58U82wi60E0?=
 =?us-ascii?Q?z5w5PtbgWl5Jw4uUnPU=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5307b665-bc5b-49cf-8b48-08d9cfe48c42
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB5520.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jan 2022 00:44:36.4486
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nUS/xyVCsgS4guGRiL4cTrTc6lUVHACrVoT/GeITtL/yFKrEucpyjMZoc2gfdX2F
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR12MB5478
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Dec 28, 2021 at 09:44:06AM +0800, Li Zhijian wrote:
> a same rxe_map_set could be freed twice:
> rxe_reg_user_mr()
>   -> rxe_mr_init_user()
>     -> rxe_mr_free_map_set() # 1st
>   -> rxe_drop_ref()
>    ...
>     -> rxe_mr_cleanup()
>       -> rxe_mr_free_map_set() # 2nd
> 
> By convention, we should cleanup/free resources in the error path in the
> same function where the resources are alloted in. So rxe_mr_init_user()
> doesn't need to free the map_set directly.
> 
> In addition, we have to reset map_set to NULL inside rxe_mr_alloc() if needed
> to prevent from map_set being double freed in rxe_mr_cleanup().
> 
> CC: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com>
> ---
> V2: Fix it by a simpler way by following suggestion from Bob,
> ---
>  drivers/infiniband/sw/rxe/rxe_mr.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Applied to for-rc, but I changed it to keep the goto unwind as is
normal.

@@ -135,19 +135,19 @@ static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf, int both)
 
        ret = rxe_mr_alloc_map_set(num_map, &mr->cur_map_set);
        if (ret)
-               goto err_out;
+               return -ENOMEM;
 
        if (both) {
                ret = rxe_mr_alloc_map_set(num_map, &mr->next_map_set);
-               if (ret) {
-                       rxe_mr_free_map_set(mr->num_map, mr->cur_map_set);
-                       goto err_out;
-               }
+               if (ret)
+                       goto err_free;
        }
 
        return 0;
 
-err_out:
+err_free:
+       rxe_mr_free_map_set(mr->num_map, mr->cur_map_set);
+       mr->cur_map_set = NULL;
        return -ENOMEM;
 }