Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 2 Jun 2023 16:34:06 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Paul Cassella <cassella@hpe.com>, Ira Weiny <ira.weiny@intel.com>
CC:     <dan.j.williams@intel.com>,
        Yongqiang Liu <liuyongqiang13@huawei.com>,
        <linux-kernel@vger.kernel.org>, <nvdimm@lists.linux.dev>,
        <vishal.l.verma@intel.com>, <dave.jiang@intel.com>,
        <akpm@linux-foundation.org>, <joao.m.martins@oracle.com>,
        <zhangxiaoxu5@huawei.com>, <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH] dax/hmem: Fix refcount leak in dax_hmem_probe()
Message-ID: <647a7c6e5686e_287a2294dc@iweiny-mobl.notmuch>
References: <20221203095858.612027-1-liuyongqiang13@huawei.com>
 <Y4u2TK4yPU9dfiDr@iweiny-mobl>
 <d0819e2a-f584-3287-61ba-88ea78a9885b@hpe.com>
 <647a384743e5d_c35b294af@iweiny-mobl.notmuch>
 <3cf0890b-4eb0-e70e-cd9c-2ecc3d496263@hpe.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <3cf0890b-4eb0-e70e-cd9c-2ecc3d496263@hpe.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?h7xeo9Yv88pVFjq2Haz1/huY8a2kd1ScX28+YFtrvBgwjPXoy5xwY/FequuF?=
 =?us-ascii?Q?OR08EyGFTCF7d+fxf/vVAn4PLoli21YEnyEeA929dkIJLwJogLDGbYZPdaXk?=
 =?us-ascii?Q?Kt3C6VuUhdz3q15ytGPkptORuoB1W2bRcihYMnkEMFPu7Ox53wglt8+MlBxo?=
 =?us-ascii?Q?PeuBJsCm6pJ8qv40S6CPClrr6OrldIaPPxEOh6pNjSwpF0TdwEiE1YnldzvQ?=
 =?us-ascii?Q?bdLwaLe7r6qpt7aZulZrtYWoRdhJz+GymwTNE4QtoQx949HsL/1y5gKNMySU?=
 =?us-ascii?Q?JNi2u+IAMO4Q0aAQ7y0Gbd1V//DHZMPf9WePD+Xg5nrb2Dqdk98EpRg6Ecz9?=
 =?us-ascii?Q?MCYGfpqQpuWvHg4gSsJ22TYOKvJXWkZHx20K7++fN9u9E22WAUiaYpXDMdCQ?=
 =?us-ascii?Q?V2KiIqYkyrnQcKOTYG8TIYBO7HeBh8DnixL6WU+h5FY14E6UzHADXIOhR34e?=
 =?us-ascii?Q?/mGAkos1XWk4zAfnMppUDovw7SZX3vQev2SrnKJ9SMW+VkfL4jk13NnW6/Uc?=
 =?us-ascii?Q?yakiioMb4X/qzKTmjnaX/K9X908yth14vqANmx/BRej2tMrwPKBeAT8PPYb9?=
 =?us-ascii?Q?XtIimpZU9ghOdOTtAPabABtT5dx7Rs08MlJ31O3sKRuWf2cb0QUhddBYY9Fc?=
 =?us-ascii?Q?GUXJTGcjRLOE8thEUSjBuk2PIvXUpxlW/20+E8nN+/+QU2evmej3CDI3RYBZ?=
 =?us-ascii?Q?q34NKkKesiHWMP5mFb16glhhj7huGMCvv4B1N6Cj3Qh7HY62nr5OJbjn9zTa?=
 =?us-ascii?Q?VTgPWgM83X/JVbg5yrCm1/+LQyvIx2BjYRKwo0NHLjfaX7VsoXHd3Mwf/Fbp?=
 =?us-ascii?Q?E47/T2IXi3ue6APwJayM3PmdAmFkuQ/MKyiBigE0hMBQf1705Pn8OzrACeyd?=
 =?us-ascii?Q?bc4X0ENqlgeJ3otm83ieJnNYnA8TNkbOSdC53tUkeVikePLqgQqqHZerjUYb?=
 =?us-ascii?Q?GBXvK8bJhpUEAzV5+G7Zh3DfZxh/CvgsXBxgWPoRwccuWnlBI7Ljmkl68jre?=
 =?us-ascii?Q?Lp5WPO1ggtxegL77xsfaDzgDas0fdKFGHcizIh6+n4S4mzymV1Mrvg4UKFD+?=
 =?us-ascii?Q?dyhIB2vH7pUWwVlqRcE+BiFTjfDhqXxt+mEjlxyin2gGytvhvlmKIpgSl+vY?=
 =?us-ascii?Q?d2QdT7psXSnMAatF7sKzJ6MWVDGyWva1lDRSLOeWJhY+coT3N+KIlHZJC82g?=
 =?us-ascii?Q?Rgzu26Ltrbm1jjuZ7Ot1OczAUy67Xbbp6etEmSt1Qufk5yGod1DLGZLupwkJ?=
 =?us-ascii?Q?uG0RPi0EpmPBfMA1GfIZPw05FmnP8rlbPn42mGHzwXyAoJGmpNTNmtW4eTGc?=
 =?us-ascii?Q?IPmizEeOopkP5XdeaA5k07HJBJXg4W5RI7Pi3g3IRdVLWqCD+KzTwNODJV/z?=
 =?us-ascii?Q?eO1p+vLayny3RBNv5mmv+FDw4Vc3NUsWtcfv1cyp7LPrP0CrfY7SxPYpnn7U?=
 =?us-ascii?Q?BVhPd7RyeMyFXqpeHq2p5D1biKux9of5TyX7MgxPDEqrVaUZXYctMTmOM5G3?=
 =?us-ascii?Q?9X2krMRg70MhQebIOzyLD7cxu3061AvS8bECXkv0qf3CDTovykxe9d2Y9m8k?=
 =?us-ascii?Q?gGpjaaJkYgKVpDd2uMUiSWw7tKNDNnwweuUvybz7?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f520c22c-eb51-430f-c103-08db63c1dfa4
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Jun 2023 23:34:14.1340
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7FsbgKyhRs5cR7GlJjQZSpzjyGjhf7oJglLqiIfXTI5+PFkI9ynd+vWhT10Sk9aDq0fUwxZLyA8G8aCeccI9Tg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6949
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Paul Cassella wrote:
> On Fri, 2 Jun 2023, Ira Weiny wrote:
> > Paul Cassella wrote:
> > > On Sat, 3 Dec 2022, Ira Weiny wrote:
> > > > On Sat, Dec 03, 2022 at 09:58:58AM +0000, Yongqiang Liu wrote:
> 
> > > > > We should always call dax_region_put() whenever devm_create_dev_dax()
> > > > > succeed or fail to avoid refcount leak of dax_region. Move the return
> > > > > value check after dax_region_put().
> 
> > > > I think dax_region_put is called from dax_region_unregister() automatically on
> > > > tear down.
> 
> > > Note the reference dax_region_unregister() will be putting is the one 
> > > devm_create_dev_dax() takes by kref_get(&dax_region->kref).   I think 
> > > dax_hmem_probe() needs to put its reference in the error case, as in the 
> > > successful case.
> 
> > Looking at this again I'm inclined to agree that something is wrong.  But
> > I'm not sure this patch fixes it. anything.
> 
> > When you say:
> > 
> > > ...   I think 
> > > dax_hmem_probe() needs to put its reference in the error case, as in the 
> > > successful case.
> > 
> > ... which kref_get() is dax_hmem_probe() letting go?
> 
> Isn't it letting go of the initial kref_init() reference from 
> alloc_dax_region()?

Oh wow!  I did not realize that about kref_init()...  :-(

> 
> Sorry, I had gone through the code a little carelessly yesterday.  Now I 
> think that kref_init() reference is the one that dax_hmem_probe() is 
> dropping in the success case, and which still needs to be dropped in the 
> error case.

yea ok...

> 
> (If so, I think the alloc_dax_region() path that returns NULL on 
> devm_add_action_or_reset() failure, releasing the kref_get reference, will 
> leak the kref_init reference and the region.)

I see now.  The ref is taken prior to the add action or reset to ensure
the dax_region does not go away should the platform device go away...

However, in all the call paths currently the device passed to
alloc_dax_region() can't go away prior to the dev_dax taking a reference.
So I don't think this extra reference is required.  :-/

As you say the ref counting right now is not correct on error flows.  But
I'm torn on the correct solution.

I think a small series broken out so it can be backported if needed would
be best.

Ira