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;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Mon, 10 Jul 2023 10:38:46 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Christian Brauner <brauner@kernel.org>
Cc:     Suren Baghdasaryan <surenb@google.com>,
        Greg KH <gregkh@linuxfoundation.org>, peterz@infradead.org,
        lujialin4@huawei.com, lizefan.x@bytedance.com, hannes@cmpxchg.org,
        mingo@redhat.com, ebiggers@kernel.org, oleg@redhat.com,
        akpm@linux-foundation.org, viro@zeniv.linux.org.uk,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        linux-kernel@vger.kernel.org, cgroups@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, kernel-team@android.com
Subject: Re: [PATCH 1/2] kernfs: add kernfs_ops.free operation to free
 resources tied to the file
Message-ID: <ZKxsVuDqdr6IJeyv@slm.duckdns.org>
References: <CAJuCfpF=DjwpWuhugJkVzet2diLkf8eagqxjR8iad39odKdeYQ@mail.gmail.com>
 <20230628-spotten-anzweifeln-e494d16de48a@brauner>
 <ZJx1nkqbQRVCaKgF@slm.duckdns.org>
 <CAJuCfpEFo6WowJ_4XPXH+=D4acFvFqEa4Fuc=+qF8=Jkhn=3pA@mail.gmail.com>
 <2023062845-stabilize-boogieman-1925@gregkh>
 <CAJuCfpFqYytC+5GY9X+jhxiRvhAyyNd27o0=Nbmt_Wc5LFL1Sw@mail.gmail.com>
 <ZJyZWtK4nihRkTME@slm.duckdns.org>
 <CAJuCfpFKjhmti8k6OHoDHAu6dPvqP0jn8FFdSDPqmRfH97bkiQ@mail.gmail.com>
 <CAJuCfpH3JcwADEYPBhzUcunj0dcgYNRo+0sODocdhbuXQsbsUQ@mail.gmail.com>
 <20230630-fegefeuer-urheber-0a25a219520d@brauner>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230630-fegefeuer-urheber-0a25a219520d@brauner>
Precedence: bulk

Hello,

On Fri, Jun 30, 2023 at 10:21:17AM +0200, Christian Brauner wrote:
> What I'm mostly reacting to is that there's a kernfs_ops->release()
> method which mirrors f_op->release() but can be called when there are
> still users which is counterintuitive for release semantics. And that
> ultimately caused this UAF issue which was rather subtle given how long
> it took to track down the root cause.
> 
> A rmdir() isn't triggering a f_op->release() if there are still file
> references but it's apparently triggering a kernfs_ops->release(). It
> feels like this should at least be documented in struct kernfs_ops...

Oh yeah, better documentation would be great. The core part here is that
kernfs is the layer which is implementing the revoke-like semantics
specifically to allow kernfs users (the ones that implement kernfs_ops) can
synchronously abort their involvement at will. So, from those users' POV,
->release is being called when it should be. The problem here was that PSI
was mixing objects from two layers with different lifetime rules, which
obviously causes issues.

As Suren's new fix shows, the fix is just using the matching object whose
lifetime is governed by kernfs. While this shows up in a subtle way for
poll, for all other operations, this is almost completely transprent.

Thanks.

-- 
tejun