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;
Message-ID: <1591134670.16819.18.camel@HansenPartnership.com>
Subject: Re: kobject_init_and_add is easy to misuse
From:   James Bottomley <James.Bottomley@HansenPartnership.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Matthew Wilcox <willy@infradead.org>,
        Wang Hai <wanghai38@huawei.com>, cl@linux.com,
        penberg@kernel.org, rientjes@google.com, iamjoonsoo.kim@lge.com,
        akpm@linux-foundation.org, khlebnikov@yandex-team.ru,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Date:   Tue, 02 Jun 2020 14:51:10 -0700
In-Reply-To: <20200602200756.GA3933938@kroah.com>
References: <20200602115033.1054-1-wanghai38@huawei.com>
         <20200602121035.GL19604@bombadil.infradead.org>
         <1591111514.4253.32.camel@HansenPartnership.com>
         <20200602173603.GB3579519@kroah.com>
         <1591127656.16819.7.camel@HansenPartnership.com>
         <20200602200756.GA3933938@kroah.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 2020-06-02 at 22:07 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 02, 2020 at 12:54:16PM -0700, James Bottomley wrote:
[...]
> > I think the only way we can make the failure semantics consistent
> > is to have the kobject_init() ones (so kfree on failure).  That
> > means for the add part, the function would have to unwind
> > everything it did from init on so kfree() is still an option.  If
> > people agree, then I can produce the patch ... it's just the
> > current drive to transform everyone who's doing kfree() into
> > kobject_put() would become wrong ...
> 
> Everyone should be putting their kfree into the kobject release
> anyway, right?

No, that's the problem ... for a static kobject you can't free it; and
the release path may make assumption which aren't valid depending on
the kobject state.

> Anyway, let's see your patch before I start to object further :)

My first thought was "what?  I got suckered into creating a patch",
thanks ;-)  But now I look, all the error paths do unwind back to the
initial state, so kfree() on error looks to be completely correct.  I
got confused by a bogus patch set like this:

https://lore.kernel.org/linux-scsi/20200528201353.14849-1-wu000273@umn.edu/

But it turns out the person sending the patch didn't understand the
network failure they quote:

b8eb718348b8 net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject

Has the problem precisely because the kobject is static.  The release
path clears it and that allows it to be readded.  I'll just reply to
the sender of the bogus patches.

James