Received: by 2002:a25:4158:0:0:0:0:0 with SMTP id o85csp256371yba; Wed, 15 May 2019 23:43:11 -0700 (PDT) X-Google-Smtp-Source: APXvYqyqfrxRk4YBbrhQ8FxIR/pghXQbQV0kZw6jdURSulc7otLSgBeRWuXriWPiI84kEMNes6DN X-Received: by 2002:a63:f10e:: with SMTP id f14mr47912177pgi.226.1557988991436; Wed, 15 May 2019 23:43:11 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1557988991; cv=none; d=google.com; s=arc-20160816; b=JGkHM+20k8EionL9/sYd1EK9BcUABIoOLi+ad7XE6PVQJwtQsF5f1C1ui55EEufryv 7FPqpuNNb4xnu5sRfiklZUDxwTkx/CIOn9rbIJNezQqYk30GhbK2tciwiU2F9XWHOd3l prkPOyiE8wgurMgGx+i/xbJUlyxSydSFQJpSIHvEECJN+uiwMjfYZBNInRjYXOTdAM+p SnfAtzvQ1O1SQWnAo4GouqfU4BbF78B2w0ctF17jIipxf/Z27FGV+LqHFBZhkPl7IA29 d2tbvJoe0kzRN4s8nqLyq6HcWMyq4+YL/yi7ROOkd837eHO2CnFfn3n4OB8sP3f7SRyS Fj/g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=lltmLkntDj73XD1qzMTvTPWXQFPO9ynr6Cg6IYo9Rpw=; b=R81keXJxGNDk8fB4fZGN+JPdNxU02cFX0REOgl8kmGbHN7whqkbsjmo6RuTzmtUDaE H8uDhAtUk3UX4ueEuYDIMiVkr+i8otuIZDj2vGpMVSu36e5OwvB08egGyNTOY0Q7e9O6 yl9eAGJfquh2pad18TI0K2dg8piEYyAWrimtABC4j/L0mObAUoZ9Ol/yZRkoS9pGKygV BKeabH+YN/6A232gNXvg8HKXeVMrxv/JAcGZz5UZ4jS+181D7caK6KDZKqywPWLblGTD m+NY6nOkNcJncoPOkihVk9NkE68U15hVhV1ZLjaxbqQZ7dW38Vah/BxfL7c44n7Lpu8V Dt+g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=hNIqYmst; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id s205si3964127pgs.467.2019.05.15.23.42.56; Wed, 15 May 2019 23:43:11 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=hNIqYmst; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726707AbfEPGkd (ORCPT + 99 others); Thu, 16 May 2019 02:40:33 -0400 Received: from mail.kernel.org ([198.145.29.99]:36542 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726674AbfEPGkc (ORCPT ); Thu, 16 May 2019 02:40:32 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 02C332082E; Thu, 16 May 2019 06:40:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1557988831; bh=lDXJwF8L6Q3G9lM+UuPh8HMotl/xHXPZGZPwBCQSEQg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=hNIqYmstZpb+wXit7EojqiyIsSgJEunDeQWaq6Im3GG0QSpy4UF9UchyzUMGcfr75 OoURW/fXvIvahUMSzzbIdrMDWY5orpuWEZehhqKrbrDjtBZjBy7w8XdRlpF5/Spays Fmtdqha8XUD4kOzSqoVZhdopv8p0dddBIdtF/elA= Date: Thu, 16 May 2019 08:40:29 +0200 From: Greg Kroah-Hartman To: "Tobin C. Harding" Cc: "Rafael J. Wysocki" , linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH] kobject: Clean up allocated memory on failure Message-ID: <20190516064029.GA17068@kroah.com> References: <20190516000716.24249-1-tobin@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190516000716.24249-1-tobin@kernel.org> User-Agent: Mutt/1.11.4 (2019-03-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 16, 2019 at 10:07:16AM +1000, Tobin C. Harding wrote: > Currently kobject_add_varg() calls kobject_set_name_vargs() then returns > the return value of kobject_add_internal(). kobject_set_name_vargs() > allocates memory for the name string. When kobject_add_varg() returns > an error we do not know if memory was allocated or not. If we check the > return value of kobject_add_internal() instead of returning it directly > we can free the allocated memory if kobject_add_internal() fails. Doing > this means that we now know that if kobject_add_varg() fails we do not > have to do any clean up, this benefit goes back up the call chain > meaning that we now do not need to do any cleanup if kobject_del() > fails. Moving further back (in a theoretical kobject user callchain) > this means we now no longer need to call kobject_put() after calling > kobject_init_and_add(), we can just call kfree() on the enclosing > structure. This makes the kobject API better follow the principle of > least surprise. > > Check return value of kobject_add_internal() and free previously > allocated memory on failure. > > Signed-off-by: Tobin C. Harding > --- > > Hi Greg, > > Pretty excited by this one, if this is correct it means that kobject > initialisation code, in the error path, can now use either kobject_put() > (to trigger the release method) OR kfree(). This means most of the > call sites of kobject_init_and_add() will get fixed for free! > > I've been wrong before so I'll state here that this is based on the > assumption that kobject_init() does nothing that causes leaked memory. > This is _not_ what the function docs in kobject.c say but it _is_ what > the code seems to say since kobject_init() does nothing except > initialise kobject data member values? Or have I got the dog by the > tail? I think you are correct here. In looking at the code paths, all should be good and safe. But, if you use your patch, then you have to call kfree, and you can not call kobject_put(), otherwise kfree_const() will be called twice on the same pointer, right? So you will have to audit the kernel and change everything again :) Or, maybe this patch would prevent that: diff --git a/lib/kobject.c b/lib/kobject.c index f2ccdbac8ed9..03cdec1d450a 100644 --- a/lib/kobject.c +++ b/lib/kobject.c @@ -387,7 +387,14 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj, return retval; } kobj->parent = parent; - return kobject_add_internal(kobj); + + retval = kobject_add_internal(kobj); + if (retval && !is_kernel_rodata((unsigned long)(kobj->name))) { + kfree_const(kobj->name); + kobj->name = NULL; + } + + return retval; } /** But that feels like a huge hack to me. I think, to be safe, we should keep the existing lifetime rules, as it mirrors what happens with 'struct device', and that is what people _should_ be using, not "raw" kobjects if at all possible. Yeah, I know filesystems don't do that, my fault, I never thought a filesystem would care about sysfs all those years ago :) thanks, greg k-h