Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 8 Sep 2021 14:32:39 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Marion et Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     minyard@acm.org, zweiss@equinix.com, andrew@aj.id.au,
        openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling
 path of 'kcs_bmc_serio_add_device()'
Message-ID: <20210908113239.GP1957@kadam>
References: <ecbfa15e94e64f4b878ecab1541ea46c74807670.1631048724.git.christophe.jaillet@wanadoo.fr>
 <20210908062719.GO1957@kadam>
 <450800047.345.1631083814685.JavaMail.www@wwinf1e33>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <450800047.345.1631083814685.JavaMail.www@wwinf1e33>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?xXzlEBtIBWEgmvXKhvir8spPdVp05V09ZjGjjWKQBrl/AD5TNAEktT9Z8p?=
 =?iso-8859-1?Q?6jFFnrTKmlYicZkrsUPribm1TwkpLf4SkBUSGpod1HxqwiryYM7JVvsYU/?=
 =?iso-8859-1?Q?KcrCP9NmhqIhpwGE9O7S3bYP9KwjhSPIZVd6Yh1kxC13z0lzXS052T1H1r?=
 =?iso-8859-1?Q?RJUBN1kQGNHzGG5c37A3mlkn/vTwDUJfagoe0ohGvXfDC6QJm8OAfHJ5qa?=
 =?iso-8859-1?Q?BTq6Q9CPrl0vCmwXT9+RrpwvRKly49ss5DU76CpFzUmlXSBi2ZBXSi/iHG?=
 =?iso-8859-1?Q?XhP5kqaGDzbWK+twNrrdu0S/MCpK3uMafeZcS4BZNbZif+tkSSSnuikHOd?=
 =?iso-8859-1?Q?8NjLTE4AcLwVr59pR0DexYgqeIKH4PQBuLnQGb1RHQLEvBlrJFHEVsWi1+?=
 =?iso-8859-1?Q?o5wvK+9JupTfGES19J5ID4RjCm73qXsFm/eC+wJ47DP4W31Ls2g77QEV+c?=
 =?iso-8859-1?Q?ehZYJqW98yY2zZyKt51cMh8kNKLKd8Y9dfJ3ocOgQ+IgzqqrWr7EqKKD2y?=
 =?iso-8859-1?Q?iHHMKcfqiYdR7nG2L0iuGtsjvQRCXFOBZsKaIuYQKWhByv8S6echo6aYVU?=
 =?iso-8859-1?Q?jAZJnVMIecayjwDucnyGg5qdnimvGiBKWn4wHHp2ekc9jnq5zj0KQlCtYN?=
 =?iso-8859-1?Q?WPWFtpmZD514Fot8wMcP+gAGLM7vtZPL6tqTxirJrbNlgW/biDZYImtzI/?=
 =?iso-8859-1?Q?Brk/+cxSew5rJBjYN7LCxX90VL+235TM5rjb1gS75FeTyD/BUIwTkRYGPw?=
 =?iso-8859-1?Q?s9RBF/BCQ4BSPELdSEf2DOkQhxs6KBhOCYdRHtAqS7aWNZlOfziVkVTo5m?=
 =?iso-8859-1?Q?ySDVao00CcTfF0Fl8PreMihEQdiHeucjStW7dnK86WX6MlRWJyuYuueFS6?=
 =?iso-8859-1?Q?tRNcalAQUsVhS3dfgYGRe2HVU3HbxIhVBkAY0xX1IHdpDuBwye4sf3RqWe?=
 =?iso-8859-1?Q?knD9hAZs3FHjVyU+qve1/mbi3BAnHOgXNYZFawHsJ+BJWndQH8xajBnb8g?=
 =?iso-8859-1?Q?Ib+UPHRZcEGKOWiCZaU5b9rBhERmI9D8zwNJpr2T9fDlvZXp5NQpT4gTWL?=
 =?iso-8859-1?Q?DIbAmNljRwuQrCmnmSzZzMMEhSUkfNnCJFD2fR5jB/FCDSvikar7X0uYJr?=
 =?iso-8859-1?Q?EmnKBT9q0eX84B1nrJLXYaSVs7vLdlOAQvbMtY6CYnvIATTZwAKcaQNqSX?=
 =?iso-8859-1?Q?lWzwxoOz4QDQL+3+DIKvnfTS9X/YppYVtYPXx7yhP9TXBaucbZJ1FuCIb9?=
 =?iso-8859-1?Q?QtREj+dvZzKTX2WTAXCsp35DPWzR7vtne0yFbsdMqzJYQFKzaeJwihulB2?=
 =?iso-8859-1?Q?q73FKNdyzMY1+h4r2oV4rqRYMZTvsD+HIPm+gE7p8REdC9HmbfnomIhdn3?=
 =?iso-8859-1?Q?NyzrguguiZ?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 11335f85-1859-4bc4-c04a-08d972bc682e
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Sep 2021 11:32:58.1955
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: TtvGrfp4Hddl/bdU0XeH2eP8Uo8K5po4QOt/kyJRHvniNchAcwMmF7ouZ2cVpFJGy4/TB4WlD2YrKvSPVmWdAjgNjCUq3GoF7JkY3GOU4fs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1903
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10100 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0
 malwarescore=0 spamscore=0 mlxscore=0 phishscore=0 mlxlogscore=999
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109030001 definitions=main-2109080073
X-Proofpoint-GUID: obAJbrJpG8OML67luv-o9EQ-s5-YToCa
X-Proofpoint-ORIG-GUID: obAJbrJpG8OML67luv-o9EQ-s5-YToCa
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Sep 08, 2021 at 08:50:14AM +0200, Marion et Christophe JAILLET wrote:
> 
> ?
> 
> > Message du 08/09/21 08:28
> > De : "Dan Carpenter" 
> > A : "Christophe JAILLET" 
> > Copie ? : minyard@acm.org, zweiss@equinix.com, andrew@aj.id.au, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
> > Objet : Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
> > 
> > On Tue, Sep 07, 2021 at 11:06:32PM +0200, Christophe JAILLET wrote:
> > > In the unlikely event where 'devm_kzalloc()' fails and 'kzalloc()'
> > > succeeds, 'port' would be leaking.
> > > 
> > > Test each allocation separately to avoid the leak.
> > > 
> > > Fixes: 3a3d2f6a4c64 ("ipmi: kcs_bmc: Add serio adaptor")
> > > Signed-off-by: Christophe JAILLET 
> > > ---
> > > drivers/char/ipmi/kcs_bmc_serio.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
> > > index 7948cabde50b..7e2067628a6c 100644
> > > --- a/drivers/char/ipmi/kcs_bmc_serio.c
> > > +++ b/drivers/char/ipmi/kcs_bmc_serio.c
> > > @@ -73,10 +73,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
> > > struct serio *port;
> > > 
> > > priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
> > > + if (!priv)
> > > + return -ENOMEM;
> > > 
> > > /* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
> > 
> > The serio_unregister_port() calls serio_destroy_port() which calls
> > put_device(&serio->dev). But I wasn't able to track it further than
> > that to the actual kfree().
> 
> Hi Dan,
> 
> Checking this release path was not the goal of this patch.

Yeah.  I was just curious.

> It was only about the VERRYYYY unlikely memory leak.
> 
> However my understanding is:
> kcs_bmc_serio_add_device
> --> serio_register_port
> --> __serio_register_port
> --> serio_init_port
> --> serio->dev.release = serio_release_port
> 
> And in serio_release_port:
> struct serio *serio = to_serio_port(dev);
> kfree(serio);
> 
> For me, this 'serio' looks to the one allocated by 'kcs_bmc_serio_add_device'.
> I think that the comment is correct.

Thanks.  This really helps me actually.  I could just make a list of
the functions which take a container_of(dev) get a struct serio and then
free it.  Then if there is only one function that matches that, I could
assume it's what put_device() will call.

regards,
dan carpenter