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 21:11:48 -0700
From:   Dan Williams <dan.j.williams@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: <647abd845bfac_142af8294a0@dwillia2-xfh.jf.intel.com.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?qAnurm/2h7v0/ZNiIh/YUwqVE87P0/T1B4MJ4AQXZgbhq/lErnAMOnBW6k8+?=
 =?us-ascii?Q?yL+LxqBOnrFZGRbyfzg1Ho/S9f/i+lgO87sMfyWPCB/TOSBVBFNO3/LhUkpT?=
 =?us-ascii?Q?5gAQR4/U/AXzCla96cZMdqHpBEcu91JTnttqVY2IjuOWA+qRCylJVjG8IQIW?=
 =?us-ascii?Q?9djsaP7zOo3JwZ8ITH1EGaL1cV6NhQEQ7A6x0avBiOaufrtejLjRdYBGOB3j?=
 =?us-ascii?Q?UVvWot949+wPXZyD98VKXr0yic7lQsN50KWrpM5X0uCRGlVDfWsWUiO0VxJG?=
 =?us-ascii?Q?IDmn6NytpLVhjESmza40IPNDTwKcvMRNJ3I/ctarq/CrLMuKFeKmbJzuohzd?=
 =?us-ascii?Q?PR9grEjvX9+sSJEYV65gww8Uv/ttGANEnxAIh0HgA1aa6hg/Cai/KiMuvkQi?=
 =?us-ascii?Q?BSRxpKcAoaGyaTuwMriMgivrAm+xl+Y/aeqXZ3Iqus+fjtWwjt7xpAKe3FGF?=
 =?us-ascii?Q?HbwqI2yog5itDf/FqJ2HywkjOpABaFWTVLPkEUSZbBV15x0/TJu5h74GGU3C?=
 =?us-ascii?Q?+pEztMEshCou8poMifSu3baUG3cDPalMT+qdw2dQrtqf7caIRUXNHPgGTr5l?=
 =?us-ascii?Q?AaXLoTO4/S2ax2z6LwAzdFdHep3JkxRLPkMGIwZ9p96LfMI1sR1pHhmVbW+w?=
 =?us-ascii?Q?VQsRezn9ZZe0rJahG6h5SOBNCNa5mJZKimAHB9yU4jXYnqX1o1Kr9DuPlMBL?=
 =?us-ascii?Q?puFF+4ErbbVOTWhb1S932nkWmVCIBSuNCnm2dv0VCcel1dpoBoWzwCqWd1R+?=
 =?us-ascii?Q?jSi7fVqAolaTXyV/7fLaMPIp8UFg6nZCdXkVG4rE/t7igpXP4W+EsDptUqcE?=
 =?us-ascii?Q?1BCD2WTEfd+sN9gNUBHHNj/BcHvTqYXQS+XQwgOlALsW44ZC+G8lTU5zya0W?=
 =?us-ascii?Q?4JbRsagvnzYGn2FSyf4QxYIIp3L5344sTyoeR0Ah9RNNJsOHt3xDQZIuKRE1?=
 =?us-ascii?Q?YYyJWaEjMHTu4VAvNG/AI1gkSij/sBaaVfvRde8Z9MROexRCjT5yG2Rjrj1V?=
 =?us-ascii?Q?oDCnTzgbBXczvwRGNg4hkP14CAl/mEO0owaRyX9iyozu8gEN85Fze2oD3vBn?=
 =?us-ascii?Q?vUq0uKF9cWd0FzUaDF21YXcNPGows4buXDC51jyvj7AWGRLS+3KXbwrvtRqY?=
 =?us-ascii?Q?ZLlRAPsYRuEz9tK9ECLOVCt7/jpDfgPv1nqdAMRuz0SaCbaakDyeGfiXePDg?=
 =?us-ascii?Q?CZ6eu5GN/Jg5I+9jwZEjS5+qUtDSB2SPr8wLrz5MDGs11/RnZGdUefor8rSp?=
 =?us-ascii?Q?fPvqHgDDJAe41cZEwDvnrrfOIHlHxheUPuLJTkcn1Wa0OqvVZ8kYQDJSKgsQ?=
 =?us-ascii?Q?106xByUg1V7y40Ck3H3k7hIgKmN1Cxh2LhMx+EqeeD1YIjtLmz5aqH5pTMHp?=
 =?us-ascii?Q?1amF5b8EFJb2NE/DXDgIHeKHfjmfQ7g3voD6ga1WL93n+eTDdA4FBIway0V5?=
 =?us-ascii?Q?knc9f8r2rAWSEbKl6OzQFQUwTa2ZYPEBCP3p3hJsivf6wyfC8J9VlHB5MuhW?=
 =?us-ascii?Q?vTbr1Hz9HFDXq6Ef0YtBrNNwoINHOMi20dcUpVP5SG6+ab0XvjFeUgWtuGOG?=
 =?us-ascii?Q?XJplw1/SNr5+MhVYJaBjxmcoQBLZ1zBLj58eyDUUuoEHupQnmYnsvxOgJMrz?=
 =?us-ascii?Q?cA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ba603c4a-3800-41e6-ba10-08db63e8a817
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jun 2023 04:11:50.9723
 (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: /mEm+MwdAauX3bzzBAcU4KPAYfMff7y5bFIRuYfsiiIr0M29jECOAx7M2F5zpQgbk/UL8B0I0g9WoOrHyqaHakT5nBGpyBDVPgFa65aPhJw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB7045
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,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,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()?
> 
> 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.
> 
> (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.)

Yes, *this* one looks valid. All the other patches in this thread had me
asking, "um, did you try it?".

Now, that said, the games that this init path is playing are hard to
follow and it should not be the case that alloc_dax_region() needs to
hold a reference on behalf of some future code path that might need it.

Lets make this clearer by moving the reference count management closer
to the object that needs it dax_region->ida. Where the extra references
were being taken on behalf of "static" regions just in case they needed
to be reference in dev_dax_release().

I also found a dax_mapping lifetime issue while testing this, so I
appreciate the focus here.