Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20230926162228.68666-1-mjguzik@gmail.com> <CAHk-=wjUCLfuKks-VGTG9hrFAORb5cuzqyC0gRXptYGGgL=YYg@mail.gmail.com>
 <CAGudoHGej+gmmv0OOoep2ENkf7hMBib-KL44Fu=Ym46j=r6VEA@mail.gmail.com>
 <20230927-kosmetik-babypuppen-75bee530b9f0@brauner> <CAHk-=whLadznjNKZPYUjxVzAyCH-rRhb24_KaGegKT9E6A86Kg@mail.gmail.com>
 <CAGudoHH2mvfjfKt+nOCEOfvOrQ+o1pqX63tN2r_1+bLZ4OqHNA@mail.gmail.com>
 <CAHk-=wjmgord99A-Gwy3dsiG1YNeXTCbt+z6=3RH_je5PP41Zw@mail.gmail.com> <ZRR1Kc/dvhya7ME4@f>
In-Reply-To: <ZRR1Kc/dvhya7ME4@f>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 27 Sep 2023 13:27:51 -0700
Message-ID: <CAHk-=wibs_xBP2BGG4UHKhiP2B=7KJnx_LL18O0bGK8QkULLHg@mail.gmail.com>
Subject: Re: [PATCH v2] vfs: shave work on failed file open
To:     Mateusz Guzik <mjguzik@gmail.com>
Cc:     Christian Brauner <brauner@kernel.org>, viro@zeniv.linux.org.uk,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Content-Type: multipart/mixed; boundary="000000000000a3607f06065d07b1"
Precedence: bulk

--000000000000a3607f06065d07b1
Content-Type: text/plain; charset="UTF-8"

On Wed, 27 Sept 2023 at 11:32, Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> put_cred showed up in file_free_rcu in d76b0d9b2d87 ("CRED: Use creds in
> file structs"). Commit message does not claim any dependency on this
> being in an rcu callback already and it looks like it was done this way
> because this was the ony spot with kmem_cache_free(filp_cachep, f)

Yes, that looks about right. So the rcu-freeing is almost an accident.

Btw, I think we could get rid of the RCU freeing of 'struct file *' entirely.

The way to fix it is

 (a) make sure all f_count accesses are atomic ops (the one special
case is the "0 -> X" initialization, which is ok)

 (b) make filp_cachep be SLAB_TYPESAFE_BY_RCU

because then get_file_rcu() can do the atomic_long_inc_not_zero()
knowing it's still a 'struct file *' while holding the RCU read lock
even if it was just free'd.

And __fget_files_rcu() will then re-check that it's the *right*
'struct file *' and do a fput() on it and re-try if it isn't. End
result: no need for any RCU freeing.

But the difference is that a *new* 'struct file *' might see a
temporary atomic increment / decrement of the file pointer because
another CPU is going through that __fget_files_rcu() dance.

Which is why "0 -> X" is ok to do as a "atomic_long_set()", but
everything else would need to be done as "atomic_long_inc()" etc.

Which all seems to be the case already, so with the put_cred() not
needing the RCU delay, I thing we really could do this patch (note:
independent of other issues, but makes your patch require that
"atomic_long_cmpxchg()" and the WARN_ON() should probably go away,
because it can actually happen).

That should help the normal file open/close case a bit, in that it
doesn't cause that extra RCU work.

Of course, on some loads it might be advantageous to do a delayed
de-allocation in some other RCU context, so ..

What do you think?

             Linus

PS. And as always: ENTIRELY UNTESTED.

--000000000000a3607f06065d07b1
Content-Type: text/x-patch; charset="US-ASCII"; name="patch.diff"
Content-Disposition: attachment; filename="patch.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_ln277b020>
X-Attachment-Id: f_ln277b020

IGZzL2ZpbGVfdGFibGUuYyB8IDE5ICsrKysrKysrKystLS0tLS0tLS0KIDEgZmlsZSBjaGFuZ2Vk
LCAxMCBpbnNlcnRpb25zKCspLCA5IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2ZzL2ZpbGVf
dGFibGUuYyBiL2ZzL2ZpbGVfdGFibGUuYwppbmRleCBlZTIxYjNkYTlkMDguLjdiMzhmZjczODVj
YyAxMDA2NDQKLS0tIGEvZnMvZmlsZV90YWJsZS5jCisrKyBiL2ZzL2ZpbGVfdGFibGUuYwpAQCAt
NjUsMjEgKzY1LDIxIEBAIHN0YXRpYyB2b2lkIGZpbGVfZnJlZV9yY3Uoc3RydWN0IHJjdV9oZWFk
ICpoZWFkKQogewogCXN0cnVjdCBmaWxlICpmID0gY29udGFpbmVyX29mKGhlYWQsIHN0cnVjdCBm
aWxlLCBmX3JjdWhlYWQpOwogCi0JcHV0X2NyZWQoZi0+Zl9jcmVkKTsKLQlpZiAodW5saWtlbHko
Zi0+Zl9tb2RlICYgRk1PREVfQkFDS0lORykpCi0JCWtmcmVlKGJhY2tpbmdfZmlsZShmKSk7Ci0J
ZWxzZQotCQlrbWVtX2NhY2hlX2ZyZWUoZmlscF9jYWNoZXAsIGYpOworCWtmcmVlKGJhY2tpbmdf
ZmlsZShmKSk7CiB9CiAKIHN0YXRpYyBpbmxpbmUgdm9pZCBmaWxlX2ZyZWUoc3RydWN0IGZpbGUg
KmYpCiB7CiAJc2VjdXJpdHlfZmlsZV9mcmVlKGYpOwotCWlmICh1bmxpa2VseShmLT5mX21vZGUg
JiBGTU9ERV9CQUNLSU5HKSkKLQkJcGF0aF9wdXQoYmFja2luZ19maWxlX3JlYWxfcGF0aChmKSk7
CiAJaWYgKGxpa2VseSghKGYtPmZfbW9kZSAmIEZNT0RFX05PQUNDT1VOVCkpKQogCQlwZXJjcHVf
Y291bnRlcl9kZWMoJm5yX2ZpbGVzKTsKLQljYWxsX3JjdSgmZi0+Zl9yY3VoZWFkLCBmaWxlX2Zy
ZWVfcmN1KTsKKwlwdXRfY3JlZChmLT5mX2NyZWQpOworCWlmICh1bmxpa2VseShmLT5mX21vZGUg
JiBGTU9ERV9CQUNLSU5HKSkgeworCQlwYXRoX3B1dChiYWNraW5nX2ZpbGVfcmVhbF9wYXRoKGYp
KTsKKwkJY2FsbF9yY3UoJmYtPmZfcmN1aGVhZCwgZmlsZV9mcmVlX3JjdSk7CisJfSBlbHNlIHsK
KwkJa21lbV9jYWNoZV9mcmVlKGZpbHBfY2FjaGVwLCBmKTsKKwl9CiB9CiAKIC8qCkBAIC00NzEs
NyArNDcxLDggQEAgRVhQT1JUX1NZTUJPTChfX2ZwdXRfc3luYyk7CiB2b2lkIF9faW5pdCBmaWxl
c19pbml0KHZvaWQpCiB7CiAJZmlscF9jYWNoZXAgPSBrbWVtX2NhY2hlX2NyZWF0ZSgiZmlscCIs
IHNpemVvZihzdHJ1Y3QgZmlsZSksIDAsCi0JCQlTTEFCX0hXQ0FDSEVfQUxJR04gfCBTTEFCX1BB
TklDIHwgU0xBQl9BQ0NPVU5ULCBOVUxMKTsKKwkJCVNMQUJfVFlQRVNBRkVfQllfUkNVIHwgU0xB
Ql9IV0NBQ0hFX0FMSUdOCisJCQl8IFNMQUJfUEFOSUMgfCBTTEFCX0FDQ09VTlQsIE5VTEwpOwog
CXBlcmNwdV9jb3VudGVyX2luaXQoJm5yX2ZpbGVzLCAwLCBHRlBfS0VSTkVMKTsKIH0KIAo=
--000000000000a3607f06065d07b1--